Fix Interrupted falling out of thread crash

[roberth]

Motivation

• Fix a problem with Split ignoreException for destructors vs interrupt-safe #11618
• Add implicit checkInterrupt calls to these loops to pick up on shutdowns sooner

Context

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.

[edolstra]

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.

[roberth]

Here's an alternate fix, 31fb9a4

See also my accidental quote reply #11665 (comment)

[edolstra]

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.

[edolstra]

Hm, it's not clear to me where ThreadPool could have an uncaught Interrupted exception. It catches all exceptions from the work item (try { w(); } catch (...) { exc = std::current_exception(); }) and I don't see where else an Interrupted could be thrown.

Same with the daemonThread, though that definitely has an uncaught exception issue if accept() fails.

[roberth]

Hm, it's not clear to me where ThreadPool could have an uncaught Interrupted exception. It catches all exceptions from the work item (try { w(); } catch (...) { exc = std::current_exception(); }) and I don't see where else an Interrupted could be thrown.

Same with the daemonThread, though that definitely has an uncaught exception issue if accept() fails.

It would be thrown by ignoreExceptionExceptInterrupt. I guess that's not so obvious, having two layers of weirdness in the name. I think we should change ignoreException -> reportException, ExceptInterrupt -> RethrowInterrupt.

[roberth]

Also the ExceptInterrupt or RethrowInterrupt part is a bit suggestive of the wrong assumptions, given that it's currently often used in contexts where Interrupt isn't caught. That part - while otherwise harmless - is only relevant when catching any of its superclasses or ..., so realistically: std::exception or nix::BaseError.

So we might prefer to "inline" its behavior into the catch clauses where it matters, and rely on reviews or perhaps linting to keep overly broad catch clauses out of the code. Wdyt?

[lf-]

So we might prefer to "inline" its behavior into the catch clauses where it matters, and rely on reviews or perhaps linting to keep overly broad catch clauses out of the code. Wdyt?

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 __cxa_throw or so hit dozens of false positives.

(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.

[roberth]

I've got rid of the loopUntilInterrupted HOF, which I only ended up using in one case, where it wasn't clear that Interrupted could even be raised.

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.
I'll follow this PR up with renames that clarify the behavior of these so-called ignore functions, but keep this diff small.

[edolstra]

@roberth Looks like you need to add an #include "signals.hh".