src: run native immediates during Environment cleanup #30666

addaleax · 2019-11-26T16:49:36Z

This is a fix for #30643 that is, unfortunately, a bit more complex than I’d like. The last commit message provides an alternative, so if anyone prefers then I’d probably implement that first as quick fix for the flaky test and then rebase this PR against it.

src: keep object alive in stream_pipe code

This was overlooked in a489583.

Refs: #30374

src: add more `can_call_into_js()` guards

This is in preparation for running native SetImmediate()
callbacks during shutdown.

src: no SetImmediate from destructor in stream_pipe code

Guard against running SetImmediate() from the destructor.
The object will not be alive or usable in the callback,
so it does not make sense to attempt to schedule the
SetImmediate().

src: run native immediates during Environment cleanup

This can be necessary, because some parts of the Node.js code base
perform cleanup operations in the Immediate callbacks, e.g. HTTP/2.

This resolves flakiness in an HTTP/2 test that failed when a
SetImmediate() callback was not run or destroyed before the
Environment destructor started, because that callback held a
strong reference to the Http2Session object and the expectation
was that no such objects exist once the Environment constructor
starts.

Another, slightly more direct, alternative would have
been to clear the immediate queue rather than to run it. However,
this approach seems to make more sense as code generally assumes
that the SetImmediate() callback will always run; For example,
N-API uses an immediate callback to call buffer finalization
callbacks.

Unref’ed immediates are skipped, as the expectation is generally
that they may not run anyway.

Fixes: #30643

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This was overlooked in a489583. Refs: nodejs#30374

This is in preparation for running native `SetImmediate()` callbacks during shutdown.

Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`.

This can be necessary, because some parts of the Node.js code base perform cleanup operations in the Immediate callbacks, e.g. HTTP/2. This resolves flakiness in an HTTP/2 test that failed when a `SetImmediate()` callback was not run or destroyed before the `Environment` destructor started, because that callback held a strong reference to the `Http2Session` object and the expectation was that no such objects exist once the `Environment` constructor starts. Another, slightly more direct, alternative would have been to clear the immediate queue rather than to run it. However, this approach seems to make more sense as code generally assumes that the `SetImmediate()` callback will always run; For example, N-API uses an immediate callback to call buffer finalization callbacks. Unref’ed immediates are skipped, as the expectation is generally that they may not run anyway. Fixes: nodejs#30643

nodejs-github-bot · 2019-11-26T16:50:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27048/

Trott

LGTM. I imagine this might also fix other infrequent and hard-to-reproduce crashes that have started showing up recently.

nodejs-github-bot · 2019-11-27T21:15:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27095/

addaleax · 2019-11-28T16:49:41Z

Landed in 8792c3f...4d73fd3

This was overlooked in a489583. Refs: #30374 PR-URL: #30666 Fixes: #30643 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This is in preparation for running native `SetImmediate()` callbacks during shutdown. PR-URL: #30666 Fixes: #30643 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`. PR-URL: #30666 Fixes: #30643 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This can be necessary, because some parts of the Node.js code base perform cleanup operations in the Immediate callbacks, e.g. HTTP/2. This resolves flakiness in an HTTP/2 test that failed when a `SetImmediate()` callback was not run or destroyed before the `Environment` destructor started, because that callback held a strong reference to the `Http2Session` object and the expectation was that no such objects exist once the `Environment` constructor starts. Another, slightly more direct, alternative would have been to clear the immediate queue rather than to run it. However, this approach seems to make more sense as code generally assumes that the `SetImmediate()` callback will always run; For example, N-API uses an immediate callback to call buffer finalization callbacks. Unref’ed immediates are skipped, as the expectation is generally that they may not run anyway. Fixes: #30643 PR-URL: #30666 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This was overlooked in a489583. Refs: #30374 PR-URL: #30666 Fixes: #30643 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This is in preparation for running native `SetImmediate()` callbacks during shutdown. PR-URL: #30666 Fixes: #30643 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`. PR-URL: #30666 Fixes: #30643 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This can be necessary, because some parts of the Node.js code base perform cleanup operations in the Immediate callbacks, e.g. HTTP/2. This resolves flakiness in an HTTP/2 test that failed when a `SetImmediate()` callback was not run or destroyed before the `Environment` destructor started, because that callback held a strong reference to the `Http2Session` object and the expectation was that no such objects exist once the `Environment` constructor starts. Another, slightly more direct, alternative would have been to clear the immediate queue rather than to run it. However, this approach seems to make more sense as code generally assumes that the `SetImmediate()` callback will always run; For example, N-API uses an immediate callback to call buffer finalization callbacks. Unref’ed immediates are skipped, as the expectation is generally that they may not run anyway. Fixes: #30643 PR-URL: #30666 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This was overlooked in a489583. Refs: nodejs#30374 PR-URL: nodejs#30666 Fixes: nodejs#30643 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This is in preparation for running native `SetImmediate()` callbacks during shutdown. PR-URL: nodejs#30666 Fixes: nodejs#30643 Refs: nodejs#30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`. PR-URL: nodejs#30666 Fixes: nodejs#30643 Refs: nodejs#30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This can be necessary, because some parts of the Node.js code base perform cleanup operations in the Immediate callbacks, e.g. HTTP/2. This resolves flakiness in an HTTP/2 test that failed when a `SetImmediate()` callback was not run or destroyed before the `Environment` destructor started, because that callback held a strong reference to the `Http2Session` object and the expectation was that no such objects exist once the `Environment` constructor starts. Another, slightly more direct, alternative would have been to clear the immediate queue rather than to run it. However, this approach seems to make more sense as code generally assumes that the `SetImmediate()` callback will always run; For example, N-API uses an immediate callback to call buffer finalization callbacks. Unref’ed immediates are skipped, as the expectation is generally that they may not run anyway. Fixes: nodejs#30643 PR-URL: nodejs#30666 Refs: nodejs#30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This was overlooked in a489583. Refs: #30374 Backport-PR-URL: #32301 PR-URL: #30666 Fixes: #30643 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This is in preparation for running native `SetImmediate()` callbacks during shutdown. Backport-PR-URL: #32301 PR-URL: #30666 Fixes: #30643 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`. Backport-PR-URL: #32301 PR-URL: #30666 Fixes: #30643 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

This can be necessary, because some parts of the Node.js code base perform cleanup operations in the Immediate callbacks, e.g. HTTP/2. This resolves flakiness in an HTTP/2 test that failed when a `SetImmediate()` callback was not run or destroyed before the `Environment` destructor started, because that callback held a strong reference to the `Http2Session` object and the expectation was that no such objects exist once the `Environment` constructor starts. Another, slightly more direct, alternative would have been to clear the immediate queue rather than to run it. However, this approach seems to make more sense as code generally assumes that the `SetImmediate()` callback will always run; For example, N-API uses an immediate callback to call buffer finalization callbacks. Unref’ed immediates are skipped, as the expectation is generally that they may not run anyway. Fixes: #30643 Backport-PR-URL: #32301 PR-URL: #30666 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax added 4 commits November 26, 2019 17:00

src: keep object alive in stream_pipe code

8b632df

This was overlooked in a489583. Refs: nodejs#30374

src: add more can_call_into_js() guards

9d455b9

This is in preparation for running native `SetImmediate()` callbacks during shutdown.

src: no SetImmediate from destructor in stream_pipe code

2286fc0

Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 26, 2019

addaleax mentioned this pull request Nov 26, 2019

investigate flaky test-worker-terminate-http2-respond-with-file #30643

Closed

Trott approved these changes Nov 26, 2019

View reviewed changes

jasnell approved these changes Nov 27, 2019

View reviewed changes

lundibundi mentioned this pull request Nov 27, 2019

node-test-commit-arm compiler ICE debian9-docker-armv7 nodejs/build#2066

Closed

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2019

addaleax closed this Nov 28, 2019

addaleax deleted the run-immediates-on-cleanup branch November 28, 2019 16:49

addaleax added a commit that referenced this pull request Nov 28, 2019

src: keep object alive in stream_pipe code

f73716d

This was overlooked in a489583. Refs: #30374 PR-URL: #30666 Fixes: #30643 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax added a commit that referenced this pull request Nov 30, 2019

src: keep object alive in stream_pipe code

d54432f

This was overlooked in a489583. Refs: #30374 PR-URL: #30666 Fixes: #30643 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

BridgeAR mentioned this pull request Dec 3, 2019

v13.3.0 proposal #30774

Merged

addaleax mentioned this pull request Dec 30, 2019

Buffer api error since 13.3.0 #31134

Closed

addaleax mentioned this pull request Mar 16, 2020

[v12.x] backport BaseObjectPtr, SetImmediate(), Worker+report #32301

Closed

addaleax added backport-open-v12.x and removed backport-blocked-v12.x labels Mar 16, 2020

codebytere mentioned this pull request Apr 3, 2020

v12.16.2 proposal #32313

Merged

addaleax added backported-to-v12.x and removed backport-open-v12.x labels Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: run native immediates during Environment cleanup #30666

src: run native immediates during Environment cleanup #30666

addaleax commented Nov 26, 2019

nodejs-github-bot commented Nov 26, 2019

Trott left a comment

nodejs-github-bot commented Nov 27, 2019

addaleax commented Nov 28, 2019

src: run native immediates during Environment cleanup #30666

src: run native immediates during Environment cleanup #30666

Conversation

addaleax commented Nov 26, 2019

src: keep object alive in stream_pipe code

src: add more can_call_into_js() guards

src: no SetImmediate from destructor in stream_pipe code

src: run native immediates during Environment cleanup

Checklist

nodejs-github-bot commented Nov 26, 2019

Trott left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 27, 2019

addaleax commented Nov 28, 2019

src: add more `can_call_into_js()` guards