-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Interrupted
falling out of thread crash
#11665
Fix Interrupted
falling out of thread crash
#11665
Conversation
0f8de62
to
b6d157d
Compare
Interrupted
falling out of thread crash
b6d157d
to
a88068f
Compare
@@ -1504,7 +1505,7 @@ void LocalDerivationGoal::startDaemon() | |||
|
|||
daemonThread = std::thread([this, store]() { | |||
|
|||
while (true) { | |||
loopUntilInterrupted([this, store]() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really clear to me why we don't just call checkInterrupt()
in the while loop.
The issue of having an uncaught exception is orthogonal to the interruption issue. It's better to have an appropriate try { ... } catch (...) { ... }
at top-level in the thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an alternate fix, 31fb9a4
See also my accidental quote reply #11665 (comment)
src/libutil/thread-pool.cc
Outdated
return Continue; | ||
}); | ||
// We've been interrupted or there's no more work to do; either way we | ||
// must return now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this create the possibility that if there is an interrupt, the ThreadPool
will return successfully? I.e. pool.process()
will return without an error, even though not all of the work items have been executed. And then (for instance) the entire Nix command could succeed if the calling thread doesn't check for interrupts.
Hm, it's not clear to me where Same with the |
a88068f
to
7c337e4
Compare
It would be thrown by |
Also the So we might prefer to "inline" its behavior into the |
Commentary from the Lix side: we have nuked the most egregious catch in nix (which is std::exception falling out of commands not being an immediate crash with core dump) and caused it to call std::terminate which then prints a stack trace and the exception type id in a custom handler. In general these catch clauses are evil and severely impede debugging: if you have an unknown exception in the daemon you are just screwed, because there's a pile of other exceptions thrown in normal program operation so breakpoints on (granted printing a stack trace still isn't useful because it's not the stack trace of the throw so it doesn't make the one in main much less bad but it does make every other less broad one less evil; see the library cpptrace for one possible solution to that) You might want to do similarly. |
7c337e4
to
47e3e7a
Compare
I've got rid of the This is a nice and short diff now. I think @lf- is making a good point, and I'd focus on removing/replacing bad catch clauses in future fixes and cleanups. |
@roberth Looks like you need to add an |
Otherwise, if checkInterrupt() in any of the supported store operations would catch onto a user interrupt, the exception would bubble to the thread start and be handled by std::terminate(): a crash.
Introduced in 8f6b347 without explanation. Throwing anything that's not that is a programming mistake that we don't want to ignore silently. A crash would be ok, because that means we/they can fix the offending throw.
7c337e4
to
fd8a4a8
Compare
Motivation
checkInterrupt
calls to these loops to pick up on shutdowns soonerContext
What I missed is that threads will crash the process if they end in an uncaught exception.
Thank you @lf- for pinging me about this bug!
This does not need a backport unless 2.25 is released first somehow.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.