Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
Trott opened this issue Nov 25, 2019 · 3 comments
Closed

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

Trott opened this issue Nov 25, 2019 · 3 comments
Assignees
@addaleax
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. http2 Issues or PRs related to the http2 subsystem. worker Issues and PRs related to Worker support.

Comments

@Trott
Copy link
Member

Trott commented Nov 25, 2019

Haven't seen this one before. Worth further investigation?

https://ci.nodejs.org/job/node-test-commit-linuxone/17161/nodes=rhel72-s390x/console

test-linuxonecc-rhel72-s390x-2

00:05:40 not ok 2360 parallel/test-worker-terminate-http2-respond-with-file
00:05:40   ---
00:05:40   duration_ms: 0.729
00:05:40   severity: crashed
00:05:40   exitcode: -6
00:05:40   stack: |-
00:05:40     out/Release/node[53404]: ../src/env.cc:435:virtual node::Environment::~Environment(): Assertion `(base_object_count()) == (0)' failed.
00:05:40      1: 0x805f36ec node::Abort() [out/Release/node]
00:05:40      2: 0x805f377c  [out/Release/node]
00:05:40      3: 0x80593306 node::Environment::~Environment() [out/Release/node]
00:05:40      4: 0x805936d2 node::Environment::~Environment() [out/Release/node]
00:05:40      5: 0x8068fe3c node::worker::Worker::Run() [out/Release/node]
00:05:40      6: 0x8069008c  [out/Release/node]
00:05:40      7: 0x3ff8888825a  [/lib64/libpthread.so.0]
00:05:40      8: 0x3ff8878dcf2  [/lib64/libc.so.6]
00:05:40   ...

@nodejs/workers @nodejs/http2 @nodejs/testing
@Trott Trott added flaky-test Issues and PRs related to the tests with unstable failures on the CI. http2 Issues or PRs related to the http2 subsystem. worker Issues and PRs related to Worker support. labels Nov 25, 2019
@targos
Copy link
Member

targos commented Nov 25, 2019

It failed locally once today. I wasn't able to reproduce, even with a lot of repetitions and parallelism.

@addaleax
Copy link
Member

This failed locally in 1/3000 runs for me. It’s not going to be easy but I’ll try to debug this – it’s most likely coming from #30374, although it’s quite possible that it just uncovered a pre-existing memory leak.

@addaleax addaleax self-assigned this Nov 26, 2019
addaleax added a commit to addaleax/node that referenced this issue Nov 26, 2019
@addaleax
src: run native immediates during Environment cleanup
64ed123 
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
@addaleax addaleax mentioned this issue Nov 26, 2019
Closed
3 tasks
@addaleax
Copy link
Member

See #30666 for a fix

addaleax added a commit that referenced this issue Nov 28, 2019
@addaleax
src: add more can_call_into_js() guards
5b59c24 
This is in preparation for running native `SetImmediate()`
callbacks during shutdown.

PR-URL: #30666
Fixes: #30643
Refs: #30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Nov 28, 2019
@addaleax
src: no SetImmediate from destructor in stream_pipe code
4c9db6c 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Nov 28, 2019
@addaleax
src: run native immediates during Environment cleanup
4d73fd3 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Nov 30, 2019
@addaleax
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Nov 30, 2019
@addaleax
src: add more can_call_into_js() guards
94357db 
This is in preparation for running native `SetImmediate()`
callbacks during shutdown.

PR-URL: #30666
Fixes: #30643
Refs: #30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Nov 30, 2019
@addaleax
src: no SetImmediate from destructor in stream_pipe code
bea2501 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Nov 30, 2019
@addaleax
src: run native immediates during Environment cleanup
6c249c0 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: keep object alive in stream_pipe code
b18531d 
This was overlooked in a489583.

Refs: nodejs#30374

PR-URL: nodejs#30666
Fixes: nodejs#30643
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: add more can_call_into_js() guards
ece9faf 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: no SetImmediate from destructor in stream_pipe code
c3fff25 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: run native immediates during Environment cleanup
2470035 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: keep object alive in stream_pipe code
7647bfe 
This was overlooked in a489583.

Refs: #30374

Backport-PR-URL: #32301
PR-URL: #30666
Fixes: #30643
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: add more can_call_into_js() guards
51230f7 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: no SetImmediate from destructor in stream_pipe code
5b65348 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 1, 2020
@addaleax @MylesBorins
src: run native immediates during Environment cleanup
2625244 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@addaleax addaleax

Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. http2 Issues or PRs related to the http2 subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: run native immediates during Environment cleanup addaleax/node
3 participants
@Trott @addaleax @targos