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

test: fix sequential test-net-connect-local-error #13064

Conversation

@sebastianplesciuc
Copy link
Contributor

commented May 16, 2017

Fixed sequential test-net-connect-local-error by swapping port and localPort in net.connect options.

Fixes: #13055

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@mscdex mscdex added the net label May 16, 2017

@refack
refack approved these changes May 16, 2017
test/sequential/test-net-connect-local-error.js Outdated
@@ -14,7 +14,7 @@ client.on('error', common.mustCall(function onError(err) {
assert.strictEqual(err.code, 'ECONNREFUSED');
assert.strictEqual(
err.localPort,
common.PORT,
common.PORT + 1,
`${err.localPort} !== ${common.PORT} in ${err}`

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

Nit: fix assertion fail message

@Trott
Copy link
Member

left a comment

LGTM with nit (about the assert message--either fix it or simply remove it) addressed and green CI.

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:fix-sequential-test-net-connect branch to a258e1c May 17, 2017

@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

@Trott @refack Fixed. I removed the message as Trott suggested. I think it looks neater this way. If it fails, the assertion is simple enough to read it even without the message.

@refack
refack approved these changes May 17, 2017
@refack

This comment has been minimized.

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

Still flaky (1 out of 100 runs of the whole sequential suite):

not ok 25 sequential/test-net-connect-local-error
  ---
  duration_ms: 0.364
  severity: fail
  stack: |-
    assert.js:92
      throw new AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'EADDRINUSE' === 'ECONNREFUSED'
        at Socket.onError (/usr/home/iojs/build/workspace/node-stress-single-test/nodes/freebsd10-64/test/sequential/test-net-connect-local-error.js:14:10)
        at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-stress-single-test/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)
@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

Could it be because of this? test-dgram-bind-shared-ports runs before this one, right?

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

Could it be because of this? test-dgram-bind-shared-ports runs before this one, right?

Good catch, that's probably it...

As I see it we have several options:

  1. Keep this PR as is (1% chance of a falke out)
  2. Rename the test so it runs before test-dgram-bind-shared-ports (Hacky but simple)
  3. Use common.PORT + 2 and common.PORT + 3 with a comment about clashing with test-dgram-bind-shared-ports (not my favorite)
  4. I'll finish my PR that salts common.PORT even in sequential mode... (will take time, and might get blocked)
  5. Add a timeout around the test code, with comment why (also not my favorite)
@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

About no. 2, wouldn't test-dgram-bind-shared-ports start to flake if we choose to do this?

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

About no. 2, wouldn't test-dgram-bind-shared-ports start to flake if we choose to do this?

I don't think so because here we don't actually bind the port.

@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

What if we use some IANA reserved ports? How much of a guarantee could we have that those ports won't be used by anything else? Could we define some port constants that indicate "hey, we should never directly bind to this port in any test ever"?

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

What if we use some IANA reserved ports? How much of a guarantee could we have that those ports won't be used by anything else? Could we define some port constants that indicate "hey, we should never directly bind to this port in any test ever"?

That was my thought when we started with #12964, also a valid solution.
/cc @nodejs/build can you guarantee that there is at least one port (e.g. 26) that is never bound on the all CI machines?

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 18, 2017

So stress test on const NEVER_PORT = 26; passes 150 iterations.
@Trott PTAL

/cc @nodejs/testing
The idea is to add const NEVER_PORT = 26; to test/common as a port that is valid but should never be bound. Any feedback?

@santigimeno

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I'm confused. If the unexpected error we're receiving is EADDRINUSE, it looks like the problem is with localPort and not port right?

@refack

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I'm confused. If the unexpected error we're receiving is EADDRINUSE, it looks like the problem is with localPort and not port right?

The expected error is ECONNREFUSED.
If some other test binds to common.PORT + 1 (or common.PORT when we switch ports) we get EADDRINUSE because of TIME_WAIT on the socket, and that's not what we were looking for.

That is why we need a destination port that will never be bound and will always cause ECONNREFUSED.
I hope that makes the problem more clear.

@Trott

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Is there any chance at all that the solution is to call socket1.close() and socket2.close() in test-dgram-bind-shared-ports?

@refack

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Is there any chance at all that the solution is to call socket1.close() and socket2.close() in test-dgram-bind-shared-ports?

I'll try

@refack refack referenced this pull request May 18, 2017
2 of 3 tasks complete
@refack

This comment has been minimized.

Copy link
Member

commented May 19, 2017

@Trott @cjihrig PTAL at current solution (IMHO this change needs reapproval)

@refack refack self-assigned this May 19, 2017

@refack
Copy link
Member

left a comment

technical blocking until others reapprove code smell removed

Dismissing my review

@refack

This comment has been minimized.

Copy link
Member

commented May 19, 2017

Cross-ref: #13033

@Trott

This comment has been minimized.

Copy link
Member

commented May 20, 2017

Not sure how I feel about this. I suspect NEVER_PORT needs either a different name or else a comment explaining what it's about. Is the idea that it should be a port we can reasonably rely on never listening? Seems like a code smell a bit, but not sure I have a better solution offhand.

/cc @nodejs/testing for other opinions...

@santigimeno

This comment has been minimized.

Copy link
Member

commented May 20, 2017

Maybe I'm missing something but, considering that the purpose of this test is checking that the properties of the error object are properly set, why not consider EADDRINUSE a legit error and move on?

For the record, I consider EADDRINUSE a legit error on FreeBSD as when running running this test there could be another socket in the TIME_WAIT state whose connection properties (localAddress:localPort --> remoteAddress:remotePort) were the same as the ones in this test.

@refack refack force-pushed the sebastianplesciuc:fix-sequential-test-net-connect branch from b681f3f to a258e1c May 20, 2017

@refack
Copy link
Member

left a comment

Accept EADDRINUSE

port: common.PORT + 1,
localPort: common.PORT,
port: common.PORT,
localPort: common.PORT + 1,
localAddress: common.localhostIPv4
});

client.on('error', common.mustCall(function onError(err) {
assert.strictEqual(err.syscall, 'connect');
assert.strictEqual(err.code, 'ECONNREFUSED');

This comment has been minimized.

Copy link
@refack

refack May 20, 2017

Member

@sebastianplesciuc IMHO @santigimeno it right, we should just add an if for EADDRINUSE (or err.code in ['ECONNREFUSED', 'EADDRINUSE'])
/cc @Trott @cjihrig

`${err.localAddress} !== ${common.localhostIPv4} in ${err}`
);
assert.strictEqual(err.localPort, common.PORT + 1);
assert.strictEqual(err.localAddress, common.localhostIPv4);
assert.strictEqual(
err.message,
`connect ECONNREFUSED ${err.address}:${err.port} ` +

This comment has been minimized.

Copy link
@refack

refack May 20, 2017

Member

If we decide to accept EADDRINUSE this'll need to be adjusted too

@refack
refack approved these changes May 20, 2017
@refack

This comment has been minimized.

Copy link
Member

commented May 20, 2017

Not sure how I feel about this. I suspect NEVER_PORT needs either a different name or else a comment explaining what it's about. Is the idea that it should be a port we can reasonably rely on never listening? Seems like a code smell a bit, but not sure I have a better solution offhand.

/cc nodejs/testing for other opinions...

Ohh, definatly a code smell 👃...
It started with a test balloon I added, but since it worked I wanted feedback. I think I'll remove it.

Maybe I'm missing something but, considering that the purpose of this test is checking that the properties of the error object are properly set, why not consider EADDRINUSE a legit error and move on?

For the record, I consider EADDRINUSE a legit error on FreeBSD as when running running this test there could be another socket in the TIME_WAIT state whose connection properties (localAddress:localPort --> remoteAddress:remotePort) were the same as the ones in this test.

That's sounds reasonable.

Also in the near future (#13100) we'll fix test-dgram-bind-shared-ports to stop using common.PORT

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:fix-sequential-test-net-connect branch from a258e1c May 20, 2017

@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

@refack fixed. Let me know if it's ok.

@refack
refack approved these changes May 20, 2017
Copy link
Member

left a comment

LGTM, let's see a CI and another stress

@refack

This comment has been minimized.

Copy link
Member

commented May 20, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 20, 2017

Ping @Trott @cjihrig. Reverted code smell. Instead we allow two error codes. PTAL.

assert.strictEqual(
err.message,
`connect ECONNREFUSED ${err.address}:${err.port} ` +
`connect ${err.code} ${err.address}:${err.port} ` +

This comment has been minimized.

Copy link
@Trott

Trott May 20, 2017

Member

Are we 100% sure that ECONNREFUSED and EADDRINUSE return the same exact error message other than the err.code and that therefore this line is correct?

This comment has been minimized.

Copy link
@refack

refack May 20, 2017

Member

AFACT they come from the same mechanism.
I tried to stress this, but didn't want to hog the freeBSD machine so only run 50 cycles.
Last time we saw 1 in a 100 give the EADDRINUSE.
So worst case we have a flake and some good comments and bread crumbs to dig further.

test/sequential/test-net-connect-local-error.js Outdated
@@ -3,28 +3,21 @@ const common = require('../common');
const assert = require('assert');
const net = require('net');

const expectedErrorCodes = ['ECONNREFUSED', 'EADDRINUSE'];

This comment has been minimized.

Copy link
@Trott

Trott May 20, 2017

Member

Can we add a comment here explaining why EADDRINUSE is included? Otherwise, someone will come across the test in 8 months and remove EADDRINUSE and we'll be right back where we are. :-D

This comment has been minimized.

Copy link
@sebastianplesciuc

sebastianplesciuc May 22, 2017

Author Contributor

I need your advice on a proper comment for this. Is this ok?

// EADDRINUSE is expected to occur on FreeBSD
// Please see https://github.com/nodejs/node/issues/13055 for more details

Not really sure if you guys approve of URLs in code comments.

This comment has been minimized.

Copy link
@gibfahn

gibfahn May 22, 2017

Member

Yeah that's fine @sebastianplesciuc . Full URL is great.

This comment has been minimized.

Copy link
@Trott

Trott May 23, 2017

Member

Format suggested is fine. EADDRINUSE doesn't occur every time, just sometimes, right? EADDRINUSE is expected makes it sound like it occurs every time. Maybe EADDRINUSE can occur on FreeBSD.?

This comment has been minimized.

Copy link
@Trott

Trott May 23, 2017

Member

Whoops, I'm too late! :-D Anyway, the comment is fine. What I suggested is just a nit. It can be ignored.

@santigimeno
Copy link
Member

left a comment

LGTM with #13064 (comment) addressed

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:fix-sequential-test-net-connect branch to cd2f002 May 22, 2017

Sebastian Plesciuc
test: fix sequential test-net-connect-local-error
Fixed sequential test-net-connect-local-error by swapping port
and localPort in net.connect options.

Fixes: #13055
@@ -3,28 +3,23 @@ const common = require('../common');
const assert = require('assert');
const net = require('net');

// EADDRINUSE is expected to occur on FreeBSD
// Please see https://github.com/nodejs/node/issues/13055 for more details

This comment has been minimized.

Copy link
@refack

refack May 22, 2017

Member

Change to:
Ref: https://github.com/nodejs/node/issues/13055

This comment has been minimized.

Copy link
@refack

refack May 22, 2017

Member

NM. I'll do it.

refack added a commit to refack/node that referenced this pull request May 22, 2017
test: fix sequential test-net-connect-local-error
Fixed sequential test-net-connect-local-error by swapping port
and localPort in net.connect options.

PR-URL: nodejs#13064
Fixes: nodejs#13055
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@refack

This comment has been minimized.

Copy link
Member

commented May 22, 2017

Landed in 3429c90

@refack refack closed this May 22, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 22, 2017

jasnell added a commit that referenced this pull request May 23, 2017
test: fix sequential test-net-connect-local-error
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 <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
jasnell added a commit that referenced this pull request May 23, 2017
test: fix sequential test-net-connect-local-error
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 <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@refack refack referenced this pull request May 25, 2017
4 of 4 tasks complete
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Jul 17, 2017
test: fix sequential test-net-connect-local-error
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 <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017

@refack refack removed their assignment Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.