Fix deadlocked nix-daemon zombies on darwin #3294 #6052
Merged
Conversation
This file contains 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
This changes the representation of the interrupt callback list to be safe to use during interrupt handling. Holding a lock while executing arbitrary functions is something to avoid in general, because of the risk of deadlock. Such a deadlock occurs in NixOS#3294 where ~CurlDownloader tries to deregister its interrupt callback. This happens during what seems to be a triggerInterrupt() by the daemon connection's MonitorFdHup thread. This bit I can not confirm based on the stack trace though; it's based on reading the code, so no absolute certainty, but a smoking gun nonetheless.
|
We (me and @flokli) also saw deadlocks on NixOS recently that only got resolved by killing all processes in the systemd service. |
This was referenced Feb 17, 2022
edolstra
approved these changes
Feb 21, 2022
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.
This changes the representation of the interrupt callback list to be safe to use during interrupt handling.
Holding a lock while executing arbitrary functions is something to avoid in general, because of the risk of deadlock.
Such a deadlock occurs in #3294 where
~CurlDownloadertries to deregister its interrupt callback.This happens during what seems to be a
triggerInterrupt()by the daemon connection'sMonitorFdHupthread. This bit I can not confirm based on the stack trace though; it's based on reading the code, so no absolute certainty, but a smoking gun nonetheless.Fixes #3294
At first I've tested this with a simpler solution: to just copy the list of interrupt handlers before running them, however that would still be fragile, as that solution does not satisfy the requirement that inserts/deletions to the interrupt handler list become effective immediately, with disastrous consequences if/when code depends on this for lifetime safety.
It is unclear to me why the deadlock would only occur on darwin, but I can speculate that there might be some nondeterminism involved that just happens to work out fine on Linux, but not on macOS.