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

test: investigate async-hooks/test-tlswrap #14404

Closed
refack opened this issue Jul 20, 2017 · 11 comments
Closed

test: investigate async-hooks/test-tlswrap #14404

refack opened this issue Jul 20, 2017 · 11 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. freebsd Issues and PRs related to the FreeBSD platform. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. windows Issues and PRs related to the Windows platform.

Comments

@refack
Copy link
Contributor

refack commented Jul 20, 2017 •
edited
Loading

  • Version: master
  • Platform: macOS
  • Subsystem: async-hooks,tls

Has been spotted on macOS:

 tools/test.py -j92 --repeat 920 test/async-hooks/test-tlswrap.js 
=== release test-tlswrap ===                    
Path: async-hooks/test-tlswrap
assert.js:43
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "client: when client destroyed":
     Called "before" 2 time(s), but expected 3 invocation(s).
    at checkHook (/Users/trott/io.js/test/async-hooks/hook-checks.js:51:14)
    at Array.forEach (<anonymous>)
    at checkInvocations (/Users/trott/io.js/test/async-hooks/hook-checks.js:28:44)
    at tick1 (/Users/trott/io.js/test/async-hooks/test-tlswrap.js:93:5)
    at Immediate.ontick (/Users/trott/io.js/test/async-hooks/tick.js:7:37)
    at runCallback (timers.js:781:20)
    at tryOnImmediate (timers.js:743:5)
    at processImmediate [as _immediateCallback] (timers.js:714:5)
Command: out/Release/node /Users/trott/io.js/test/async-hooks/test-tlswrap.js
@refack refack added async_hooks Issues and PRs related to the async hooks subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. macos Issues and PRs related to the macOS platform / OSX. labels Jul 20, 2017
@Trott
Copy link
Member

Trott commented Jul 20, 2017

Probably race condition. The comment around before line 93 (where the test fails) kind of suggests that too:

    // TODO: why is client not destroyed here even after 5 ticks?
    // or could it be that it isn't actually destroyed until
    // the server is closed?

@mcollina
Copy link
Member

I've tested it on 8.2.0 as well, same error.

@refack refack mentioned this issue Jul 23, 2017
Closed
4 tasks
@targos
Copy link
Member

targos commented Sep 11, 2017

Windows is also affected: https://ci.nodejs.org/job/node-test-binary-windows/11046/RUN_SUBSET=2,VS_VERSION=vs2015-x86,label=win2008r2/console

@targos targos added the windows Issues and PRs related to the Windows platform. label Sep 11, 2017
@BridgeAR
Copy link
Member

FreeBSD is also affected https://ci.nodejs.org/job/node-test-commit-freebsd/11775/nodes=freebsd10-64/console

I guess it is system independent.

@BridgeAR BridgeAR added the freebsd Issues and PRs related to the FreeBSD platform. label Sep 24, 2017
@BridgeAR
Copy link
Member

@nodejs/async_hooks it would be great if this could be looked at.

Having a green CI for the next Code & Learn would be really neat.

@Trott
Copy link
Member

Trott commented Oct 2, 2017

I managed to replicate this locally on macOS. Did it by running 96 simultaneous copies of the test. Definitely strengthens the case that this is a race condition...

$ tools/test.py -j 96 --repeat 192 async-hooks/test-tlswrap
=== release test-tlswrap ===                    
Path: async-hooks/test-tlswrap
assert.js:45
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "client: when client destroyed":
     Called "before" 2 time(s), but expected 3 invocation(s).
    at checkHook (/Users/trott/io.js/test/async-hooks/hook-checks.js:51:14)
    at Array.forEach (<anonymous>)
    at checkInvocations (/Users/trott/io.js/test/async-hooks/hook-checks.js:28:44)
    at tick1 (/Users/trott/io.js/test/async-hooks/test-tlswrap.js:94:5)
    at Immediate.ontick [as _onImmediate] (/Users/trott/io.js/test/async-hooks/tick.js:7:37)
    at runCallback (timers.js:798:20)
    at tryOnImmediate (timers.js:760:5)
    at processImmediate [as _immediateCallback] (timers.js:731:5)
Command: out/Release/node /Users/trott/io.js/test/async-hooks/test-tlswrap.js
[00:19|% 100|+ 191|-   1]: Done  
$

@Trott
Copy link
Member

Trott commented Oct 2, 2017

Hmmm....I see this test uses common.PORT but asynch-hooks tests are run in parallel, so that's a bug (because another test using port 0 could result in a port collision).

@Trott Trott mentioned this issue Oct 2, 2017
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Oct 2, 2017

PR to fix the common.PORT thing in #15742 but unsurprisingly that change doesn't fix this issue.

Trott added a commit to Trott/io.js that referenced this issue Oct 2, 2017
@Trott
test: remove common.PORT from test-tlswrap
0717bc5 
`test/async-hooks/test/test-tlswrap.js` uses `common.PORT` but
async-hooks tests are run in parallel. Another test using port 0 could
result in a port collision. Remove `common.PORT` from the test.

Refs: nodejs#14404 (comment)
Trott added a commit to Trott/io.js that referenced this issue Oct 2, 2017
@Trott
test: fix flaky async-hooks/test-tlswrap
a91beda 
There is a race condition in async-hooks/test-tlswrap. This addresses it
by waiting 5 more ticks if the client has not been destroyed yet.

Fixes: nodejs#14404
@Trott Trott mentioned this issue Oct 2, 2017
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Oct 2, 2017

Proposed fix for this issue: #15744

@refack
Copy link
Contributor Author

refack commented Oct 2, 2017

@Trott did you manage to repro on CI?

@cjihrig cjihrig mentioned this issue Oct 3, 2017
Merged
2 tasks
@Trott
Copy link
Member

Trott commented Oct 3, 2017

@Trott did you manage to repro on CI?

@refack Yes. https://ci.nodejs.org/job/node-stress-single-test/1436/nodes=osx1010/console

Trott added a commit to Trott/io.js that referenced this issue Oct 5, 2017
@Trott
test: remove common.PORT from test-tlswrap
ddbf07a 
`test/async-hooks/test/test-tlswrap.js` uses `common.PORT` but
async-hooks tests are run in parallel. Another test using port 0 could
result in a port collision. Remove `common.PORT` from the test.

PR-URL: nodejs#15742
Ref: nodejs#14404 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott closed this as completed in ad652af Oct 5, 2017
MylesBorins pushed a commit that referenced this issue Oct 7, 2017
@Trott @MylesBorins
test: remove common.PORT from test-tlswrap
0129980 
`test/async-hooks/test/test-tlswrap.js` uses `common.PORT` but
async-hooks tests are run in parallel. Another test using port 0 could
result in a port collision. Remove `common.PORT` from the test.

PR-URL: #15742
Ref: #14404 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 7, 2017
@Trott @MylesBorins
test: fix flaky async-hooks/test-tlswrap
c940840 
There is a race condition in async-hooks/test-tlswrap. This addresses it
by waiting 5 more ticks if the client has not been destroyed yet.

PR-URL: #15744
Fixes: #14404
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 11, 2017
@Trott @MylesBorins
test: remove common.PORT from test-tlswrap
22ea3a8 
`test/async-hooks/test/test-tlswrap.js` uses `common.PORT` but
async-hooks tests are run in parallel. Another test using port 0 could
result in a port collision. Remove `common.PORT` from the test.

PR-URL: #15742
Ref: #14404 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 11, 2017
@Trott @MylesBorins
test: fix flaky async-hooks/test-tlswrap
1e79a06 
There is a race condition in async-hooks/test-tlswrap. This addresses it
by waiting 5 more ticks if the client has not been destroyed yet.

PR-URL: #15744
Fixes: #14404
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 12, 2017
@Trott @addaleax
test: remove common.PORT from test-tlswrap
fcdc14d 
`test/async-hooks/test/test-tlswrap.js` uses `common.PORT` but
async-hooks tests are run in parallel. Another test using port 0 could
result in a port collision. Remove `common.PORT` from the test.

PR-URL: nodejs/node#15742
Ref: nodejs/node#14404 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 12, 2017
@Trott @addaleax
test: fix flaky async-hooks/test-tlswrap
9e70e07 
There is a race condition in async-hooks/test-tlswrap. This addresses it
by waiting 5 more ticks if the client has not been destroyed yet.

PR-URL: nodejs/node#15744
Fixes: nodejs/node#14404
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. freebsd Issues and PRs related to the FreeBSD platform. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: fix flaky async-hooks/test-tlswrap Trott/io.js
5 participants
@mcollina @refack @Trott @targos @BridgeAR