net: check EADDRINUSE after binding localPort #15097

joyeecheung · 2017-08-30T17:28:29Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Fixes: #15084

See the comments of checkBindError for details.

Affected core subsystem(s)

net, test

joyeecheung · 2017-08-30T17:29:27Z

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

jasnell · 2017-08-30T19:25:36Z

What should the semver-iness of this be?

lpinca · 2017-08-30T20:13:09Z

test/parallel/test-net-client-bind-twice.js

+const net = require('net');
+
+const server1 = net.createServer(common.mustNotCall());
+server1.listen(common.PORT, '127.0.0.1', common.mustCall(() => {


Can we avoid using common.PORT? If not I think it's better to move this test to sequential.

refack · 2017-08-30T19:57:53Z

test/parallel/test-net-client-bind-twice.js

@@ -0,0 +1,26 @@
+'use strict';
+
+// This tests that net.connect() from a used local port throws EADDRINUSE.


👍
It makes me happy to see a new test with a description.

refack · 2017-08-30T20:02:15Z

test/parallel/test-net-client-bind-twice.js

+const net = require('net');
+
+const server1 = net.createServer(common.mustNotCall());
+server1.listen(common.PORT, '127.0.0.1', common.mustCall(() => {


You should use common.localhostIPv4

refack · 2017-08-30T20:03:18Z

test/parallel/test-net-client-bind-twice.js

+  const server2 = net.createServer(common.mustNotCall());
+  server2.listen(common.PORT + 1, '127.0.0.1', common.mustCall(() => {
+    const client = net.connect({
+      host: server1.address().address,


common.localhostIPv4 here as well because of #14900

refack · 2017-08-30T20:03:35Z

test/parallel/test-net-client-bind-twice.js

+const server1 = net.createServer(common.mustNotCall());
+server1.listen(common.PORT, '127.0.0.1', common.mustCall(() => {
+  const server2 = net.createServer(common.mustNotCall());
+  server2.listen(common.PORT + 1, '127.0.0.1', common.mustCall(() => {


If you're doing common.PORT + 1 this test should be in /test/sequential

refack · 2017-08-30T21:06:02Z

What should the semver-iness of this be?

Tricky one... but IMHO patch. The previous behaviour was "working but not as expected" in that in the case of error it ignored the provided option.

joyeecheung · 2017-08-31T02:18:57Z

@lpinca @refack Thanks for the review. Updated, PTAL.

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

joyeecheung · 2017-08-31T02:22:01Z

@jasnell I would say it's a patch since it never worked anyway...to me depending on the "bind to that port if available, otherwise bind to a random port" behavior seems like a bug.

refack · 2017-08-31T02:37:40Z

@joyeecheung I've added a commit that uses common.localhostIPv4 as the addressed for connecting as well, since server1.address().address can't be "trusted". If it returns '0.0.0.0' or '::' (even though it shouldn't) this will fail on Windows, and since this is not the subject of the test, why risk it?
Obviously feel free to push my suggestion out.

joyeecheung · 2017-08-31T02:40:05Z

@refack Ah, makes sense to me, sorry that I overlooked #15097 (comment) .

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

PR-URL: nodejs#15097 Fixes: nodejs#15084 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

BridgeAR · 2017-09-03T20:20:24Z

Landed in c419adf

PR-URL: nodejs/node#15097 Fixes: nodejs/node#15084 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-09-10T02:22:06Z

This is not landing cleanly on v8.x. @joyeecheung would you be willing to backport to v8.x-staging

PR-URL: nodejs#15097 Fixes: nodejs#15084 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

joyeecheung · 2017-09-10T03:38:46Z

@MylesBorins Backport opened at #15309

PR-URL: #15097 Fixes: #15084 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-09-20T20:55:50Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

joyeecheung · 2017-10-17T16:52:42Z

Depends on #11796 which is don't-land-on-v6.x

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Aug 30, 2017

benjamingr approved these changes Aug 30, 2017

View reviewed changes

jasnell approved these changes Aug 30, 2017

View reviewed changes

BridgeAR approved these changes Aug 30, 2017

View reviewed changes

lpinca reviewed Aug 30, 2017

View reviewed changes

refack suggested changes Aug 30, 2017

View reviewed changes

net: check EADDRINUSE after binding localPort

c9c55ef

joyeecheung force-pushed the fix-localport branch from c3c94cd to c9c55ef Compare August 31, 2017 02:17

[suggestion] use common.localhostIPv4 also for connect

4f4924c

refack approved these changes Aug 31, 2017

View reviewed changes

lpinca approved these changes Aug 31, 2017

View reviewed changes

cjihrig approved these changes Aug 31, 2017

View reviewed changes

BridgeAR closed this Sep 3, 2017

MylesBorins added the backport-requested-v8.x label Sep 10, 2017

joyeecheung mentioned this pull request Sep 10, 2017

[v8.x backport] net: check EADDRINUSE after binding localPort #15309

Closed

4 tasks

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the backport-requested-v6.x label Sep 20, 2017

MylesBorins added backported-to-v8.x and removed backport-requested-v8.x labels Oct 7, 2017

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: check EADDRINUSE after binding localPort #15097

net: check EADDRINUSE after binding localPort #15097

joyeecheung commented Aug 30, 2017 •

edited

Loading

joyeecheung commented Aug 30, 2017

jasnell commented Aug 30, 2017

lpinca Aug 30, 2017

refack Aug 30, 2017

refack Aug 30, 2017

refack Aug 30, 2017

refack Aug 30, 2017

refack commented Aug 30, 2017

joyeecheung commented Aug 31, 2017

joyeecheung commented Aug 31, 2017

refack commented Aug 31, 2017 •

edited

Loading

joyeecheung commented Aug 31, 2017

BridgeAR commented Sep 3, 2017

MylesBorins commented Sep 10, 2017

joyeecheung commented Sep 10, 2017

MylesBorins commented Sep 20, 2017

joyeecheung commented Oct 17, 2017

		@@ -0,0 +1,26 @@
		'use strict';

		// This tests that net.connect() from a used local port throws EADDRINUSE.

net: check EADDRINUSE after binding localPort #15097

net: check EADDRINUSE after binding localPort #15097

Conversation

joyeecheung commented Aug 30, 2017 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung commented Aug 30, 2017

jasnell commented Aug 30, 2017

lpinca Aug 30, 2017

Choose a reason for hiding this comment

refack Aug 30, 2017

Choose a reason for hiding this comment

refack Aug 30, 2017

Choose a reason for hiding this comment

refack Aug 30, 2017

Choose a reason for hiding this comment

refack Aug 30, 2017

Choose a reason for hiding this comment

refack commented Aug 30, 2017

joyeecheung commented Aug 31, 2017

joyeecheung commented Aug 31, 2017

refack commented Aug 31, 2017 • edited Loading

joyeecheung commented Aug 31, 2017

BridgeAR commented Sep 3, 2017

MylesBorins commented Sep 10, 2017

joyeecheung commented Sep 10, 2017

MylesBorins commented Sep 20, 2017

joyeecheung commented Oct 17, 2017

joyeecheung commented Aug 30, 2017 •

edited

Loading

refack commented Aug 31, 2017 •

edited

Loading