Repository navigation
Platform thread race: process.exit executed before background threads are ready #23065
Description
Forked issue from #22938 (comment).
This test case:
const assert = require('assert');
const child_process = require('child_process');
const { promisify } = require('util');
const execFile = promisify(child_process.execFile);
const code =
'console.log(42);process.exit(1)';
{
execFile(process.execPath, ['-e', code])
.catch((err) => {
console.log(`child stderr: >>>\n${err.stderr}<<<`);
assert.strictEqual(err.code, 1);
assert.strictEqual(err.stdout, '42\n');
});
}Intermittently crashes in the child process on windows when a small IO delay is introduced in the platform worker thread startup:
static void PlatformWorkerThread(void* data) {
fprintf(stderr, ""); // write an empty string to stderr, just to introduce a delay.
TRACE_EVENT_METADATA1("__metadata", "thread_name", "name",
"PlatformWorkerThread");
TaskQueue<Task>* pending_worker_tasks = static_cast<TaskQueue<Task>*>(data);
while (std::unique_ptr<Task> task = pending_worker_tasks->BlockingPop()) {
task->Run();
pending_worker_tasks->NotifyOfCompletion();
}
}Crash:
C:\workspace\ofrobots\test\common\index.js:662
const crashOnUnhandledRejection = (err) => { throw err; };
^
AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:
�[32m+ actual�[39m �[31m- expected�[39m
�[32m+�[39m 3221225477
�[31m-�[39m 1
at execFile.catch.common.mustCall (C:\workspace\ofrobots\test\parallel\test-child-process-promisified.js:47:14)
at C:\workspace\ofrobots\test\common\index.js:349:15
at process._tickCallback (internal/process/next_tick.js:68:7)
The platform worker thread is trying to do IO while the main thread is already shutting things down in exit.
/cc @nodejs/platform-windows
Activity
ofrobots commentedon Sep 25, 2018
Okay, it seems that Unix platforms are a lot faster in the time between
uv_thread_createis called and the thread loop is actually called. On windows, the main thread callsuv_thread_createand proceed to executeprocess.exit(1)before the threads have actually started running. The threads do eventually start but crash because the process is in the middle of being teared down.This happens on windows but seems to be a general problem with how we are doing background threads in Node.
process.exitdoesn't wait to ensure that process is in a sane state before proceeding (as it rightly should not).I think the right answer would be for the main thread to wait for the background threads to be initialized before executing user code, but I am interested in other opinions. /cc @addaleax @jasnell @nodejs/libuv.
[-]Platform thread race on windows[/-][+]Platform thread race: process.exit executed before background threads are ready[/+]jasnell commentedon Sep 25, 2018
I think you're right. Initializing the background threads should complete before progressing forward with any bootstrap or post-bootstrap execution of any code that may trigger a process.exit
addaleax commentedon Sep 25, 2018
That seems like a good idea, but it sounds like it could affect Windows startup performance significantly?
In the case of libuv, we do wait until all threads in the pool have spawned, and on non-Windows additionally wait for them to
uv_thread_join()on exit.ofrobots commentedon Sep 25, 2018
@addaleax I doubt this will affect startup performance significantly anywhere. I can measure. I think this should be done on all platforms. We have only seen this manifest on windows so far, but there the race exists on all platforms. It would not be safe to give control to any bootstrap code while the background worker threads are still initializing.
ofrobots commentedon Sep 25, 2018
While implementing a fix, I ran into a bug in libuv thread synchronization primitives on mac. This is currently blocked on libuv/libuv#2003.
ofrobots commentedon Oct 1, 2018
@addaleax I did some measurements using this benchmark: https://github.com/proxy/gist.github.com/ofrobots/f686aa7bc33b21e9c09bece53bb8bf52. This runs
node -e 0in a loop 50 times and measures the time taken. 10 such measurements are taken in a single benchmark run. I looked at the mean and the standard deviation on the 10 measurements. I repeated this three times. This experiment was performed on a windows workstation.Baseline (no thread sync on startup):
With a thread sync on startup:
Based on this there is no evidence of noticeable impact on startup. I get similar data on my Mac laptop (interestingly, we are 2x faster starting up on a Mac. This may be something worth chasing down at some point... )
Commits under test:
masterbranch as of this writing.src: ready background workers before bootstrap
Trott commentedon Oct 6, 2018
Fixed in e273abc
src: ready background workers before bootstrap
src: ready background workers before bootstrap
src: ready background workers before bootstrap
src: ready background workers before bootstrap