fix(cli): Set a maximum time to wait for spawn_blocking tasks
#18436
Conversation
Tokio's `spawn_blocking` allows running blocking operations, which in Deno include file I/O and "non-blocking" FFI, in a thread pool separate from the thread running the JS event loop. However, by default, when the tokio runtime is dropped, it will block on the main thread until all such blocking operations have completed. This is a problem, since a non-blocking FFI call might never actually finish, which would keep the Deno process running forever, even if it would otherwise have finished earlier because of an uncaught exception. This change, therefore, sets a time limit of 0.5 seconds to wait for those blocking tasks to finish. Closes denoland#18131.
There was a problem hiding this comment.
The exact timeout duration might maybe be better as eg. 1 sec, and a parameter for nonblocking FFI calls might be beneficial, but over all this looks good to me.
I'd like a review from @littledivy for the duration related specifics.
| }, | ||
| }); | ||
|
|
||
| const ONE_HOUR = 1000 * 60 * 60; |
There was a problem hiding this comment.
Don’t you think this is too time-consuming for cases when the test might fail?
There was a problem hiding this comment.
Agreed, 1 minute should be plenty.
|
I’ll be using my local Deno build until then. |
|
I haven't looked into the root cause much, Why do the FFI uncaught exceptions not let the event loop quit gently? |
This is not about uncaught exceptions coming from FFI at all – notice how the test I added has an exception being thrown from the main module, completely separate from the FFI call. This is also not at all about the Deno event loop, which AFAIK deals with FFI just fine, but about the tokio event loop. When a tokio runtime is dropped, it will usually wait for any blocking spawned tasks to finish. But for FFI non-blocking calls, this might literally take forever. If it's not possible to kill a thread without its cooperation, there's really nothing we can do to kill a non-blocking FFI call. But I don't know the specifics of thread-killing across OS's. |
| // and non-blocking FFI, would usually keep the runtime from shutting down | ||
| // until they finish. With FFI this can keep the process from ever shutting | ||
| // down, so instead we set a maximum waiting time of half a second. | ||
| rt.shutdown_timeout(Duration::from_secs_f64(0.5)); |
There was a problem hiding this comment.
This will affect other things that we might want to wait more than half a second for. For example: #18401 (comment)
|
Should people be calling |
|
Do you mean should people be replacing all the thrown errors with |
call_some_nonblocking_function();
console.debug("Reaches here.");
throw new Error(); // not thrownWho wants this behavior anyway? |
|
@roj1512 no, I meant people using I believe a workaround to your bug is to do the following for now: try {
main();
} catch (err) {
console.error(err);
Deno.exit(1);
} |
|
It seems to be possible to get the same issue with reading stdin: #14333 |
Tokio's
spawn_blockingallows running blocking operations, which in Deno include file I/O and "non-blocking" FFI, in a thread pool separate from the thread running the JS event loop. However, by default, when the tokio runtime is dropped, it will block on the main thread until all such blocking operations have completed.This is a problem, since a non-blocking FFI call might never actually finish, which would keep the Deno process running forever, even if it would otherwise have finished earlier because of an uncaught exception. This change, therefore, sets a time limit of 0.5 seconds to wait for those blocking tasks to finish.
Closes #18131.