worker: spin uv_run twice before closing loop #26138
Conversation
On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006
nodejs-github-bot
added
the
c++
| // Need to run the loop twice more to close the platform's uv_async_t | ||
| // TODO(addaleax): It would be better for the platform itself to provide | ||
| // some kind of notification when it has fully cleaned up. | ||
| uv_run(&loop_, UV_RUN_ONCE); |
There was a problem hiding this comment.
Should this be in a #ifdef Win32 or similar?
There was a problem hiding this comment.
Why not UV_RUN_DEFAULT? My concern is that some platform-specific libuv change may at some point require a different arbitrary number or runs.
There was a problem hiding this comment.
@eugeneo Because we want to not keep spinning the event loop indefinitely, which could happen if e.g. addons still have active handles (which we currently crash for, which is what we want).
I agree that this is hacky – hence the TODO – but I wouldn’t expect libuv to change anything here (and if they do, we’d notice because they run Node.js in the libuv CI).
@Fishrock123 I don’t have strong feelings about that, I can add it if you want.
There was a problem hiding this comment.
Perhaps we should keep the behavior consistent then.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/20802/ (:heavy_check_mark:) |
addaleax
added
the
author ready
|
Landed in 1d51353 |
On Windows, the Platform’s
uv_async_tmay need two iterationsbefore closing when it was previously in use.
Refs: #26089
Refs: #26006
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes