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.

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

Investigate flaky sequential/test-net-connect-local-error on FreeBSD #13055

Closed
Trott opened this issue May 16, 2017 · 10 comments
Closed

Investigate flaky sequential/test-net-connect-local-error on FreeBSD #13055

Trott opened this issue May 16, 2017 · 10 comments
Labels
freebsd Issues and PRs related to the FreeBSD platform. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.

Comments

@Trott
Copy link
Member

Trott commented May 16, 2017

  • Version: v8.0.0-pre
  • Platform: freebsd
  • Subsystem: test

https://ci.nodejs.org/job/node-test-commit-freebsd/9081/nodes=freebsd10-64/consoleFull

not ok 1453 sequential/test-net-connect-local-error
  ---
  duration_ms: 0.294
  severity: fail
  stack: |-
    assert.js:92
      throw new AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'EADDRINUSE' === 'ECONNREFUSED'
        at Socket.onError (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/sequential/test-net-connect-local-error.js:14:10)
        at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:504:15)
        at emitOne (events.js:115:13)
        at Socket.emit (events.js:210:7)
        at emitErrorNT (net.js:1337:8)
        at _combinedTickCallback (internal/process/next_tick.js:102:11)
        at process._tickCallback (internal/process/next_tick.js:161:9)

The test was recently updated to check for ECONNREFUSED. However, we are seeing EADDRINUSE sometimes on FreeBSD in CI as seen above.

Possible bug in the FreeBSD operating system or quirk (relative to our other hosts) in the way it handles open/half-open/whatever ports? (I normally wouldn't jump to Blame-The-Operating-System as the likely cause right off the bat, but this is looking very familiar to me and I seem to recall it, but I can't find anything so maybe I'm imagining it?)

/cc @sebastianplesciuc @refack @nodejs/testing

@Trott Trott added freebsd Issues and PRs related to the FreeBSD platform. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels May 16, 2017
@Trott
Copy link
Member Author

Trott commented May 16, 2017

@nodejs/platform-freebsd

@Trott
Copy link
Member Author

Trott commented May 16, 2017

Oh, meant to link to the recent changes: 0c2edd2

@sebastianplesciuc
Copy link

I can make another PR to remove that assert if need be or fix it any other way. Please let me know.

@Trott
Copy link
Member Author

Trott commented May 16, 2017

Possibly maybe kinda sorta related? nodejs/node-v0.x-archive#3796

@refack
Copy link
Contributor

refack commented May 16, 2017

I run two "single-test" CI before #12964 (comment) so high probability is that this is test cross-talk. Well sequential !== sterile.
FWIW For Windows (and acording to rfc793) a closed socket will enter TIME_WAIT state for 2*MSL and will not SYN,ACK and will not be allowed to be reused by the OS.
So a previous test using common.PORT will cause EADDRINUSE...
It might happen more and more... So at first thought seems like common.PORT is problematic even in /sequential/.
two possible solutions:

  • we salt common.PORT a little bit per process.
  • we add a delay between tests in /sequential/ and verify the environment is more sterile (common.refreshTmpDir , verify common.PORT is not in TIME_WAIT, etc.)

@Trott
Copy link
Member Author

Trott commented May 16, 2017

@refack For this particular test, I wonder if the collision might be made less likely if we swap PORT and PORT + 1? EADDRINUSE must mean it's the localPort, right? In which case PORT + 1 is somewhat less likely to linger over from another test in the way you describe because it is used much less often than just PORT.

Not necessarily the greatest solution, but if it works, an easy one to implement quickly while we try to figure out something better (or not)?

@refack
Copy link
Contributor

refack commented May 16, 2017

@Trott I'd go one step further and use PORT + 1 and PORT + 2 (which is just super simple slating), and document the reasoning.

@sebastianplesciuc you wanna do the honors?

@Trott
Copy link
Member Author

Trott commented May 16, 2017

I'd go one step further and use PORT + 1 and PORT + 2 (which is just super simple slating), and document the reasoning.

I worry that it might become an justification for doing PORT + 50 in some test and PORT + 45 in another. Maybe we can stick with PORT and PORT + 1 here and go to your proposal if we see continued problems?

@refack
Copy link
Contributor

refack commented May 16, 2017

I worry that it might become an justification for doing PORT + 50 in some test and PORT + 45 in another. Maybe we can stick with PORT and PORT + 1 here and go to your proposal if we see continued problems?

NP

@sebastianplesciuc
Copy link

@refack on it! Thanks

@refack refack closed this as completed in 3429c90 May 22, 2017
jasnell pushed a commit that referenced this issue May 23, 2017
Fixed sequential test-net-connect-local-error by swapping port
and localPort in net.connect options.

PR-URL: #13064
Fixes: #13055
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
jasnell pushed a commit that referenced this issue May 23, 2017
Fixed sequential test-net-connect-local-error by swapping port
and localPort in net.connect options.

PR-URL: #13064
Fixes: #13055
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
refack added a commit to refack/node that referenced this issue Jun 9, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: nodejs#13100
Refs: nodejs#13055
Refs: nodejs#12999
Refs: nodejs#13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this issue Jun 10, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 17, 2017
Fixed sequential test-net-connect-local-error by swapping port
and localPort in net.connect options.

PR-URL: #13064
Fixes: #13055
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
refack added a commit to refack/node that referenced this issue Jul 17, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: nodejs#13100
Refs: nodejs#13055
Refs: nodejs#12999
Refs: nodejs#13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: #14327
PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: #14327
PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: #14327
PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
freebsd Issues and PRs related to the FreeBSD platform. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

3 participants