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

tcp socket localPort option does not work #15084

Closed
beingmohit opened this issue Aug 30, 2017 · 3 comments
Closed

tcp socket localPort option does not work #15084

beingmohit opened this issue Aug 30, 2017 · 3 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem.

Comments

@beingmohit
Copy link

beingmohit commented Aug 30, 2017

Version: v6.11.2
Platform: Linux workstation 4.4.0-92-generic #115-Ubuntu SMP Thu Aug 10 09:04:33 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
Subsystem: net

const net = require('net');

function createServer(port) {
    var server = net.createServer();

    server.on('connection', (socket) => {
        console.log('Server '+port+': socket connected',  socket.remotePort);
    });

    server.listen(port);
}

createServer(6001);
createServer(6002);

var client =  new net.Socket();

client.connect({
    port: 6001,
    address: '::ffff:127.0.0.1',
    localPort: 6002
});

Output: Server 6001: socket connected 35372
Expected: Server 6001: socket connected 6002 (or EADDRINUSE error)

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Aug 30, 2017
@joyeecheung
Copy link
Member

joyeecheung commented Aug 30, 2017

I can reproduce this with master. I think this is similar to what

node/lib/net.js

Lines 1383 to 1390 in 2154a3c

// EADDRINUSE may not be reported until we call listen(). To complicate
// matters, a failed bind() followed by listen() will implicitly bind to
// a random port. Ergo, check that the socket is bound to the expected
// port before calling listen().
//
// FIXME(bnoordhuis) Doesn't work for pipe handles, they don't have a
// getsockname() method. Non-issue for now, the cluster module doesn't
// really support pipes anyway.
tries to address. I will open a PR fixing this shortly (the fix is causing other tests to fail because they are depending on this bug...).

@joyeecheung joyeecheung self-assigned this Aug 30, 2017
@joyeecheung joyeecheung added the confirmed-bug Issues with confirmed bugs. label Aug 30, 2017
@beingmohit
Copy link
Author

After the fix, will it throw EADDRINUSE or use the port 6002 (in above example code) ?

@joyeecheung
Copy link
Member

joyeecheung commented Aug 30, 2017

@beingmohit

It will throw EADDRINUSE when the localAddress option of net.connect(options, cb) is specified as the same as the host passed to listen(port, host, cb). It will use the port 6002 if neither host nor localAddress is specified because libuv sets SO_REUSEADDR on TCP sockets, so we will be connecting from localhost(default of socket.connect) to the unspecified address, which is allowed. EDIT: oops, the default localAddress of socket.connect is also the unspecified address...so it will throw if neither is specified. If only one of them is unspecified, SO_REUSEADDR would allow the client to bind to that port.

So in the example above, it will use 6002 if I set localAddress to 127.0.0.1 (I don't use the IPv6-prefixed version because my current ISP does not support IPv6, although I think it should work either way. Note that address is not a valid option to socket.connect(), I guess what you are trying to set is localAddress?). Also the snippet does not close the servers so running it again after killing it will probably result in EADDRINUSE if those sockets are still in TIME_WAIT state.

Also I am getting consistent errors with these tests with master after running the tests with my patch...I am pretty sure this is caused by #14781 , still investigating..

See errors
=== release test-http-client-req-error-dont-double-fire ===
Path: parallel/test-http-client-req-error-dont-double-fire
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/joyee/projects/node/test/common/index.js:477:10)
    at Object.<anonymous> (/Users/joyee/projects/node/test/parallel/test-http-client-req-error-dont-double-fire.js:10:24)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:202:16)
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/joyee/projects/node/test/common/index.js:477:10)
    at Object.<anonymous> (/Users/joyee/projects/node/test/parallel/test-http-client-req-error-dont-double-fire.js:14:40)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:202:16)
Command: out/Release/node /Users/joyee/projects/node/test/parallel/test-http-client-req-error-dont-double-fire.js
=== release test-net-better-error-messages-port-hostname ===
Path: parallel/test-net-better-error-messages-port-hostname
assert.js:41
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'EADDRNOTAVAIL' === 'ENOTFOUND'
    at Socket.<anonymous> (/Users/joyee/projects/node/test/parallel/test-net-better-error-messages-port-hostname.js:12:10)
    at Socket.<anonymous> (/Users/joyee/projects/node/test/common/index.js:509:15)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
Command: out/Release/node /Users/joyee/projects/node/test/parallel/test-net-better-error-messages-port-hostname.js
=== release test-net-connect-immediate-finish ===
Path: parallel/test-net-connect-immediate-finish
assert.js:41
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'ETIMEDOUT' === 'ENOTFOUND'
    at Socket.client.once.common.mustCall (/Users/joyee/projects/node/test/parallel/test-net-connect-immediate-finish.js:35:10)
    at Socket.<anonymous> (/Users/joyee/projects/node/test/common/index.js:509:15)
    at Object.onceWrapper (events.js:316:30)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
Command: out/Release/node /Users/joyee/projects/node/test/parallel/test-net-connect-immediate-finish.js
[02:00|% 100|+ 1805|-   3]: Done
make: *** [test] Error 1

EDIT: uh, I am stuck with a DNS-hijacking ISP again and that's what has been failing my tests :/

addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 5, 2017
PR-URL: nodejs/node#15097
Fixes: nodejs/node#15084
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 10, 2017
PR-URL: nodejs#15097
Fixes: nodejs#15084
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
PR-URL: #15097
Fixes: #15084
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 11, 2017
PR-URL: #15097
Fixes: #15084
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 11, 2017
PR-URL: #15097
Fixes: #15084
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
PR-URL: #15097
Fixes: #15084
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants