diff options
| author | Anna Henningsen <anna@addaleax.net> | 2019-12-12 05:48:47 +0300 |
|---|---|---|
| committer | Myles Borins <mylesborins@google.com> | 2019-12-18 02:20:56 +0300 |
| commit | d456aa0a579fc00a3b80c439af1e3668b617ded6 (patch) | |
| tree | ef627b68e37d17e7bf80dc0b40e7815cb315034a /src | |
| parent | 0c18c49f0efd8296e7cf73680c8df0674ac4131c (diff) | |
src: unregister Isolate with platform before disposing
I previously thought the order of these calls was no longer
relevant. I was wrong.
This commit undoes the changes from 312c02d25e9, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d25e9.
Fixes: https://github.com/nodejs/node/issues/30846
Refs: https://github.com/nodejs/node/pull/30181
PR-URL: https://github.com/nodejs/node/pull/30909
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Diffstat (limited to 'src')
| -rw-r--r-- | src/node.h | 1 | ||||
| -rw-r--r-- | src/node_main_instance.cc | 2 | ||||
| -rw-r--r-- | src/node_worker.cc | 7 |
3 files changed, 8 insertions, 2 deletions
diff --git a/src/node.h b/src/node.h index c80e6266857..5f462082f91 100644 --- a/src/node.h +++ b/src/node.h @@ -286,6 +286,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { // This function may only be called once per `Isolate`, and discard any // pending delayed tasks scheduled for that isolate. + // This needs to be called right before calling `Isolate::Dispose()`. virtual void UnregisterIsolate(v8::Isolate* isolate) = 0; // The platform should call the passed function once all state associated diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index eea84772530..86a857299f6 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -99,8 +99,8 @@ NodeMainInstance::~NodeMainInstance() { if (!owns_isolate_) { return; } - isolate_->Dispose(); platform_->UnregisterIsolate(isolate_); + isolate_->Dispose(); } int NodeMainInstance::Run() { diff --git a/src/node_worker.cc b/src/node_worker.cc index 676b70a1fa9..26f7ddab5aa 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -190,8 +190,13 @@ class WorkerThreadData { *static_cast<bool*>(data) = true; }, &platform_finished); - isolate->Dispose(); + // The order of these calls is important; if the Isolate is first disposed + // and then unregistered, there is a race condition window in which no + // new Isolate at the same address can successfully be registered with + // the platform. + // (Refs: https://github.com/nodejs/node/issues/30846) w_->platform_->UnregisterIsolate(isolate); + isolate->Dispose(); // Wait until the platform has cleaned up all relevant resources. while (!platform_finished) |