test: fix flaky test-async-wrap-uncaughtexception by Trott · Pull Request #16598 · nodejs/node

Trott · 2017-10-30T02:47:48Z

It's possible for beforeExit to be emitted more than once if work is
scheduled by its listeners. Accommodate this fact in the test.

Fixes: #16210

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test async-wrap

It's possible for `beforeExit` to be emitted more than once if work is scheduled by its listeners. Accommodate this fact in the test. Fixes: nodejs#16210

Trott · 2017-10-30T02:49:52Z

ping @joaocgreis @addaleax @joyeecheung I want to make sure this is the correct fix and not working around a legitimate bug in core that needs fixing. See #16210 for background/details.

Trott · 2017-10-30T05:47:07Z

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

Trott · 2017-10-30T05:47:26Z

@nodejs/testing

joyeecheung

The process.removeAllListeners('uncaughtException'); in L14 can be removed now?

refack

Not convinced this is the right fix.

refack · 2017-10-30T21:06:44Z

I'm not sure this is the right fix.

This seems like a noop change. If there is a second call to 'uncaughtException' then the process will err-exit anyway.
I'm not sure I understand why this is non-deterministic.

addaleax · 2017-10-30T21:09:24Z

I kind of have to side with @refack here – if this is a fix, then it’s not obvious where the second uncaught exception is coming from. From the looks of it, that would not be a bug in the test but rather a bug

joyeecheung · 2017-10-31T05:41:24Z

@refack @addaleax Maybe we should just remove the removeAllListeners('uncaughtException') line? If there are more uncaught exceptions(i.e. a bug), the mustCall wrapping the listener would fail. If there are not, not removing the listeners does not block the process from exiting anyway.

refack · 2017-10-31T13:23:19Z

Some stress tests:
master: https://ci.nodejs.org/job/node-stress-single-test/1475/nodes=ubuntu1604-64/console 🔴
PR as is: https://ci.nodejs.org/job/node-stress-single-test/1476/nodes=ubuntu1604-64/console ✔️
PR rebased: https://ci.nodejs.org/job/node-stress-single-test/1477/nodes=ubuntu1604-64/console 🔴

refack · 2017-10-31T13:30:59Z

@joyeecheung testing (refack/node@master...refack:pr/16412)
~~https://ci.nodejs.org/job/node-stress-single-test/1478/nodes=ubuntu1604-64/ ✔️~~ [wrong params]
f7a34bdd02ec14cff680105900eca217957f0530 - https://ci.nodejs.org/job/node-stress-single-test/nodes=ubuntu1604-64/1481/console ✔️
rerun e567402 - https://ci.nodejs.org/job/node-stress-single-test/1482/nodes=ubuntu1604-64/console 🔴
99eae86aa0ed5f87e00522e1d60df3945dc0b79c - https://ci.nodejs.org/job/node-stress-single-test/1483/nodes=ubuntu1604-64/console ✔️
87efc69ecdd3673c3553d57137902db953743915 - https://ci.nodejs.org/job/node-stress-single-test/1484/nodes=ubuntu1604-64/console

refack · 2017-10-31T17:08:39Z

OK so there is definatly a change/regression. Seeing this locally with officialy build binaries:

8.6.0 ✔️

d:\code\node-github-desktop$ node8.6.0 D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js
uncaughtException Error: ah crap
at RandomBytes.require.randomBytes.common.mustCall (D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:51:9)
at RandomBytes.ondone (D:\code\node-github-desktop\test\common\index.js:525:15)
beforeExit
at process.on (D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:17:11)
at emitOne (events.js:115:13)
at process.emit (events.js:210:7)
RANDOMBYTESREQUEST(5): trigger: 1 execution: 1
before: 5
Immediate(6): trigger: 5 execution: 5
after: 5
before: 6
after: 6
destroy: 5
destroy: 6

8.7.0 🔴

d:\code\node-github-desktop$ node8.7.0 D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js
uncaughtException Error: ah crap
at RandomBytes.require.randomBytes.common.mustCall (D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:51:9)
at RandomBytes.ondone (D:\code\node-github-desktop\test\common\index.js:525:15)
beforeExit
at process.on (D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:17:11)
at emitOne (events.js:115:13)
at process.emit (events.js:210:7)
RANDOMBYTESREQUEST(5): trigger: 1 execution: 1
before: 5
Immediate(6): trigger: 5 execution: 5
after: 5
before: 6
after: 6
destroy: 5
destroy: 6
beforeExit
at process.on (D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:17:11)
at emitOne (events.js:115:13)
at process.emit (events.js:210:7)
RANDOMBYTESREQUEST(5): trigger: 1 execution: 1
before: 5
Immediate(6): trigger: 5 execution: 5
after: 5
before: 6
after: 6
destroy: 5
destroy: 6
D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:20
if (++count > 1) throw e;
^
Error
at process.on (D:\code\node-github-desktop\test\parallel\test-async-wrap-uncaughtexception.js:17:11)
at emitOne (events.js:115:13)
at process.emit (events.js:210:7)

Trott · 2017-11-01T03:39:05Z

@refackgit bisect to the rescue?

Trott · 2017-11-02T00:16:00Z

I think I found the commit that is the likely culprit. See #16210 (comment)

Closing this for now.

test: fix flaky test-async-wrap-uncaughtexception
a200dbb
It's possible for `beforeExit` to be emitted more than once if work is scheduled by its listeners. Accommodate this fact in the test. Fixes: nodejs#16210

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 30, 2017

Trott mentioned this pull request Oct 30, 2017
Flaky test-async-wrap-uncaughtexception #16210
Closed

mscdex added the async_wrap label Oct 30, 2017

jasnell approved these changes Oct 30, 2017
View reviewed changes

danbev approved these changes Oct 30, 2017
View reviewed changes

joyeecheung reviewed Oct 30, 2017
View reviewed changes

joyeecheung approved these changes Oct 30, 2017
View reviewed changes

cjihrig approved these changes Oct 30, 2017
View reviewed changes

misterdjules mentioned this pull request Oct 30, 2017
deps: update V8 to 6.3 #16271
Closed
2 tasks

apapirovski approved these changes Oct 30, 2017
View reviewed changes

refack added the async_hooks Issues and PRs related to the async hooks subsystem. label Oct 30, 2017

refack suggested changes Oct 30, 2017
View reviewed changes

Trott closed this Nov 2, 2017

Trott mentioned this pull request Nov 2, 2017
test: fix flaky test-async-wrap-uncaughtexception #16692
Closed
3 tasks

yhwang mentioned this pull request Nov 2, 2017
build: remove cctest extension #16680
Closed
2 tasks

Trott deleted the fix-flaky-async-wrap branch January 13, 2022 22:47

Uh oh!

Uh oh!

Conversation

Trott commented Oct 30, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Oct 30, 2017

Uh oh!

Trott commented Oct 30, 2017

Uh oh!

Trott commented Oct 30, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 30, 2017

Uh oh!

addaleax commented Oct 30, 2017

Uh oh!

joyeecheung commented Oct 31, 2017

Uh oh!

refack commented Oct 31, 2017

Uh oh!

refack commented Oct 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Oct 31, 2017

8.6.0 ✔️

8.7.0 🔴

Uh oh!

Trott commented Nov 1, 2017

Uh oh!

Trott commented Nov 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

refack commented Oct 31, 2017•
edited
Loading