Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit a96a846

Browse filesBrowse files
addaleaxtargos
authored andcommitted
worker: correct (de)initialization order
- Initialize `thread_exit_async_` only once the thread has been started. This is done since it is only triggered from the thread when it is exiting. - Move the final `uv_run` to the `Worker` destructor. This makes sure that it is always run, regardless of whether the thread is actually started or not. - Always dispose the `Isolate` before cleaning up the libuv event loop. This now matches the reverse order of initialization. Fixes: #22736 PR-URL: #22773 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 49b5933 commit a96a846
Copy full SHA for a96a846

File tree

Expand file treeCollapse file tree

2 files changed

+37
-13
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+37
-13
lines changed
Open diff view settings
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+22-13Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,6 @@ Worker::Worker(Environment* env, Local<Object> wrap)
7171
CHECK_NE(isolate_, nullptr);
7272
CHECK_EQ(uv_loop_init(&loop_), 0);
7373

74-
thread_exit_async_.reset(new uv_async_t);
75-
thread_exit_async_->data = this;
76-
CHECK_EQ(uv_async_init(env->event_loop(),
77-
thread_exit_async_.get(),
78-
[](uv_async_t* handle) {
79-
static_cast<Worker*>(handle->data)->OnThreadStopped();
80-
}), 0);
81-
8274
{
8375
// Enter an environment capable of executing code in the child Isolate
8476
// (and only in it).
@@ -242,9 +234,6 @@ void Worker::Run() {
242234

243235
DisposeIsolate();
244236

245-
// Need to run the loop one more time to close the platform's uv_async_t
246-
uv_run(&loop_, UV_RUN_ONCE);
247-
248237
{
249238
Mutex::ScopedLock lock(mutex_);
250239
CHECK(thread_exit_async_);
@@ -256,6 +245,13 @@ void Worker::Run() {
256245
}
257246

258247
void Worker::DisposeIsolate() {
248+
if (env_) {
249+
CHECK_NOT_NULL(isolate_);
250+
Locker locker(isolate_);
251+
Isolate::Scope isolate_scope(isolate_);
252+
env_.reset();
253+
}
254+
259255
if (isolate_ == nullptr)
260256
return;
261257

@@ -331,12 +327,16 @@ Worker::~Worker() {
331327
CHECK(stopped_);
332328
CHECK(thread_joined_);
333329
CHECK_EQ(child_port_, nullptr);
334-
CheckedUvLoopClose(&loop_);
335330

336331
// This has most likely already happened within the worker thread -- this
337332
// is just in case Worker creation failed early.
338333
DisposeIsolate();
339334

335+
// Need to run the loop one more time to close the platform's uv_async_t
336+
uv_run(&loop_, UV_RUN_ONCE);
337+
338+
CheckedUvLoopClose(&loop_);
339+
340340
Debug(this, "Worker %llu destroyed", thread_id_);
341341
}
342342

@@ -360,10 +360,19 @@ void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
360360

361361
w->env()->add_sub_worker_context(w);
362362
w->stopped_ = false;
363+
w->thread_joined_ = false;
364+
365+
w->thread_exit_async_.reset(new uv_async_t);
366+
w->thread_exit_async_->data = w;
367+
CHECK_EQ(uv_async_init(w->env()->event_loop(),
368+
w->thread_exit_async_.get(),
369+
[](uv_async_t* handle) {
370+
static_cast<Worker*>(handle->data)->OnThreadStopped();
371+
}), 0);
372+
363373
CHECK_EQ(uv_thread_create(&w->tid_, [](void* arg) {
364374
static_cast<Worker*>(arg)->Run();
365375
}, static_cast<void*>(w)), 0);
366-
w->thread_joined_ = false;
367376
}
368377

369378
void Worker::StopThread(const FunctionCallbackInfo<Value>& args) {
Collapse file
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Flags: --experimental-worker
2+
'use strict';
3+
require('../common');
4+
const assert = require('assert');
5+
const { Worker } = require('worker_threads');
6+
7+
// This tests verifies that failing to serialize workerData does not keep
8+
// the process alive.
9+
// Refs: https://github.com/nodejs/node/issues/22736
10+
11+
assert.throws(() => {
12+
new Worker('./worker.js', {
13+
workerData: { fn: () => {} }
14+
});
15+
}, /DataCloneError/);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.