threadpool: add panic_handler

[ry]

Motivation

tokio-threadpool currently catches panics that happen inside the thread pool. This can cause silent failures.

Solution

This patch adds a feature no_catch_panic to tokio-threadpool to resume_unwind() after the catch happens. This does not actually forward the panics back to tokio::run(), that will be left for later work.

Ref #495
#209
denoland/deno#1311
denoland/deno#2097

[davidbarsky]

Thanks for opening this PR, and I’m sorry you ran into those issues in Deno! I think this behavior might be better integrated not as a feature flag, but as part of the builder, as a lot of similar configuration is done through there. What do you think?

[ry]

@davidbarsky That does sounds better. I've updated the patch add Builder::catch_panics() and remove the feature flag. I'm working on a test (also in the patch) but having some trouble accessing the pool from the around_worker callback, so that it can be shutdown. Any suggestions?

[davidbarsky]

@ry Thanks for updating the PR! What makes things somewhat tricky is the pool doesn't exist until .build() is called. The way forward I see is:

1. Thread panic::catch_unwind closure through Config (This might require changing Builder::catch_panics to accept a closure that does the panic handling for you).
2. This gives Pool has access to the panic handler.
3. In turn, Runtime also access to the panic handler where the shutdown-on-panic can be implemented.

At that point, forwarding the panics to tokio::run() might come for free.

(Unfortunately, I think the “later work” you mentioned has to happen now if this feature is done through a builder, not a compile-time flag.)

[ry]

@davidbarsky Ok good idea. PTAL - I've updated the patch to have Builder::catch_panics() take a callback which can be used in combination with std::panic::resume_unwind() to crash on panics. A test is added demoing this. However it still does not forward the error to tokio::run().

[carllerche]

Thanks for the PR.

I'm out this week, but I did a quick skim and nothing jumped out.

I assigned @stjepang, if it looks good, feel free to merge. Otherwise, I'll take another look next week and handle it 👍

[ghost]

This looks good to me, thanks!

My only nit is that maybe we should rename catch_panics to panic_handler like in Rayon (see here) for consistency.

[ry]

My only nit is that maybe we should rename catch_panics to panic_handler like in Rayon (see here) for consistency.

Done.

[ghost]

Thanks for the quick commit! :) I'm deferring the PR to @carllerche in case he wants to take one final look at it.

[carllerche]

Thanks 👍 this should get published to crates.io tomorrow.