daemon: handle event queueing and shutdown issues #692
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use STM to short-circuit reading from the event queue if the latch is triggered.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the daemon’s shutdown mechanism by replacing the old MVar‑based exit latch with a new TVar‑based ShutdownLatch and updating the event loop and signal handling accordingly.
- Replace ExitLatch with ShutdownLatch in the event loop types.
- Integrate STM‑based ShutdownLatch into the event loop logic for handling shutdown and event processing.
- Update signal handlers in the daemon to support immediate versus graceful shutdown.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cachix/src/Cachix/Daemon/Types/EventLoop.hs | Removed ExitLatch and updated the data type to use ShutdownLatch. |
| cachix/src/Cachix/Daemon/Types/Daemon.hs | Removed the unused daemonShutdownLatch field. |
| cachix/src/Cachix/Daemon/Types.hs | Added exports/imports for the new event loop error types. |
| cachix/src/Cachix/Daemon/ShutdownLatch.hs | Reimplemented the shutdown latch using TVar with STM support. |
| cachix/src/Cachix/Daemon/EventLoop.hs | Updated event loop logic to integrate with ShutdownLatch for shutdown signaling. |
| cachix/src/Cachix/Daemon.hs | Modified signal handler installation to use the updated shutdown logic. |
Comments suppressed due to low confidence (1)
cachix/src/Cachix/Daemon.hs:183
- [nitpick] The variable 'isSecondInterrupt' holds the previous value of interruptRef; consider renaming it (for example, to 'wasAlreadyInterrupted') to more clearly indicate that a 'True' value means a prior interrupt was already recorded.
isSecondInterrupt <- liftIO $ atomicModifyIORef' interruptRef (True,)
Re-use the event loop for shutdown, since that's the main run loop of the daemon anyway. Remove the unused daemon shutdown latch. We now also gracefully handle sigINT.
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses graceful shutdown and event queue handling issues in the daemon. Key changes include:
- Updating signal handling to distinguish between immediate and graceful shutdown for sigTERM and sigINT.
- Refactoring event loop shutdown logic to use a new STM‐based ShutdownLatch and increasing the queue capacity.
- Adjusting daemon signal handler integration and removing unused shutdown latch fields.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cachix/src/Cachix/Deploy/Agent.hs | Updated signal handler to track and act upon repeated SIGINT events |
| cachix/src/Cachix/Daemon/Types/EventLoop.hs | Replaced exit latch with an STM-based shutdownLatch and related documentation |
| cachix/src/Cachix/Daemon/Types/Daemon.hs | Removed the daemonShutdownLatch field as the shutdown now occurs via the event loop |
| cachix/src/Cachix/Daemon/Types.hs | Updated module imports to include new event loop error definitions |
| cachix/src/Cachix/Daemon/ShutdownLatch.hs | Refactored latch implementation from MVar to TVar with STM functions |
| cachix/src/Cachix/Daemon/EventLoop.hs | Enhanced event loop handling to check for shutdown signals and adjust queue size |
| cachix/src/Cachix/Daemon.hs | Adjusted signal handlers using runInIO for proper Daemon action integration |
| startExitTimer :: ThreadId -> Daemon () | ||
| startExitTimer mainThreadId = do | ||
| void $ liftIO $ forkIO $ do | ||
| threadDelay (15 * 1000 * 1000) -- 15 seconds |
There was a problem hiding this comment.
Consider extracting the magic number for the exit timer delay into a named constant to improve maintainability.
Suggested change
| threadDelay (15 * 1000 * 1000) -- 15 seconds | |
| threadDelay exitTimerDelayMicroseconds |
Copilot uses AI. Check for mistakes.
| queue <- liftIO $ newTBMQueueIO 100 | ||
| return $ EventLoop {queue, exitLatch} | ||
| shutdownLatch <- ShutdownLatch.newShutdownLatch | ||
| queue <- liftIO $ newTBMQueueIO 100_000 |
There was a problem hiding this comment.
[nitpick] The increased queue capacity to 100_000 may have performance implications under high load; consider reviewing resource usage and potential memory constraints.
Suggested change
| queue <- liftIO $ newTBMQueueIO 100_000 | |
| let defaultQueueCapacity = 10_000 -- Default capacity for the event loop queue | |
| queue <- liftIO $ newTBMQueueIO defaultQueueCapacity |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sigTERMtriggers the immediate shutdown of the daemon.sigINTattempts to gracefully shut down the daemon. Exits immediately if a second signal is sent.