src: unregister Isolate with platform before disposing · nodejs/node@65e5a8a · GitHub

nodejs / node Public

• Notifications You must be signed in to change notification settings
• Fork 36.6k
• Star 120k

• Code
• Issues 635
• Pull requests 755
• Actions
• Projects
• Security and quality
• Insights

Additional navigation options

Commit 65e5a8a

Browse filesBrowse the repository at this point in the historyBrowse files

addaleax

authored and

BethGriggs

committed

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 312c02d, 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 312c02d. Fixes: #30846 Refs: #30181 PR-URL: #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>

1 parent 322912a commit 65e5a8a
Copy full SHA for 65e5a8a

4 files changed

+9-3Lines changed: 9 additions & 3 deletions

File tree

Expand file treeCollapse file tree

Open diff view settings

Filter options

• src
  • node.h
  • node_main_instance.cc
  • node_worker.cc
• test/cctest
  • node_test_fixture.h

Expand file treeCollapse file tree

Open diff view settings

Collapse file

‎src/node.h‎

Copy file name to clipboardExpand all lines: src/node.h

+1Lines changed: 1 addition & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -280,6 +280,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
280	280
281	281	// This function may only be called once per `Isolate`, and discard any
282	282	// pending delayed tasks scheduled for that isolate.
	283	+ // This needs to be called right before calling `Isolate::Dispose()`.
283	284	virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
284	285
285	286	// The platform should call the passed function once all state associated

Collapse file

‎src/node_main_instance.cc‎

Copy file name to clipboardExpand all lines: src/node_main_instance.cc

+1-1Lines changed: 1 addition & 1 deletion

Original file line number	Diff line number	Diff line change
@@ -99,8 +99,8 @@ NodeMainInstance::~NodeMainInstance() {
99	99	if (!owns_isolate_) {
100	100	return;
101	101	}
102		- isolate_->Dispose();
103	102	platform_->UnregisterIsolate(isolate_);
	103	+ isolate_->Dispose();
104	104	}
105	105
106	106	int NodeMainInstance::Run() {

Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc

+6-1Lines changed: 6 additions & 1 deletion

Original file line number	Diff line number	Diff line change
@@ -192,8 +192,13 @@ class WorkerThreadData {
192	192	*static_cast<bool*>(data) = true;
193	193	}, &platform_finished);
194	194
195		- isolate->Dispose();
	195	+ // The order of these calls is important; if the Isolate is first disposed
	196	+ // and then unregistered, there is a race condition window in which no
	197	+ // new Isolate at the same address can successfully be registered with
	198	+ // the platform.
	199	+ // (Refs: https://github.com/nodejs/node/issues/30846)
196	200	w_->platform_->UnregisterIsolate(isolate);
	201	+ isolate->Dispose();
197	202
198	203	// Wait until the platform has cleaned up all relevant resources.
199	204	while (!platform_finished)

Collapse file

‎test/cctest/node_test_fixture.h‎

Copy file name to clipboardExpand all lines: test/cctest/node_test_fixture.h

+1-1Lines changed: 1 addition & 1 deletion

Original file line number	Diff line number	Diff line change
@@ -106,9 +106,9 @@ class NodeTestFixture : public ::testing::Test {
106	106
107	107	void TearDown() override {
108	108	isolate_->Exit();
109		- isolate_->Dispose();
110	109	platform->DrainTasks(isolate_);
111	110	platform->UnregisterIsolate(isolate_);
	111	+ isolate_->Dispose();
112	112	isolate_ = nullptr;
113	113	}
114	114	};

0 commit comments

Comments

0 (0)