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

undefined port should trigger a RangeError when launching a server #14205

Closed
paulbrie opened this issue Jul 12, 2017 · 7 comments · Fixed by #14221
Closed

undefined port should trigger a RangeError when launching a server #14205

paulbrie opened this issue Jul 12, 2017 · 7 comments · Fixed by #14221
Labels
confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem.

Comments

@paulbrie
Copy link

  • Version: 7.9.0
  • Platform: Darwin MacBook-Air-de-paul-7.local 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
  • Subsystem: ?

Hi guys,

On my version/platform it is possible to launch a server with an undefined port. This is misleading given that there's no error message and you believe the server is working but in fact it does not listen at all. On other versions it throws a RangeError.

A complete history can be found here: expressjs/express#3364 but Doug (https://github.com/dougwilson) gave me this one liner which makes it easy to test:

Thanks,
Paul

node -pe 'require("http").createServer(function(){}).listen(undefined, function(){})'
Server {
  domain: null,
  _events:
   { request: [Function],
     connection: [Function: connectionListener],
     listening: { [Function: bound onceWrapper] listener: [Function] } },
  _eventsCount: 3,
  _maxListeners: undefined,
  _connections: 0,
  _handle:
   TCP {
     bytesRead: 0,
     _externalStream: {},
     fd: 11,
     reading: false,
     owner: [Circular],
     onread: null,
     onconnection: [Function: onconnection],
     writeQueueSize: 0 },
  _usingSlaves: false,
  _slaves: [],
  _unref: false,
  allowHalfOpen: true,
  pauseOnConnect: false,
  httpAllowHalfOpen: false,
  timeout: 120000,
  _pendingResponseData: 0,
  maxHeadersCount: null,
  _connectionKey: '6::::0' }
@evanlucas
Copy link
Contributor

if you change that one liner to node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}); s.address()' you'll see that undefined will be converted to 0, which will allow the OS to assign a random port.

@paulbrie
Copy link
Author

paulbrie commented Jul 12, 2017

Hi Evan,

my initial code was:

    // start the server
    console.log('process.env.PORT', process.env.PORT)

    app.listen(process.env.PORT, function () {
        console.log(`Server is running on port ${port}`)
    })

In that context process.env.PORT was undefined and node was not complaining.

@evanlucas
Copy link
Contributor

yes, it doesn't complain because when a port is undefined, we treat it like it's 0 and the OS will choose the port. So it listens, it just listens on a random port. To get the port, you can access server.address().port

@wesleytodd
Copy link
Member

wesleytodd commented Jul 12, 2017

This behavior has recently changed, maybe a few lines in that section of the docs would be good, because I just tried it with node 8.x and 6.x. In node 6 null and 0 work, undefined fails. In node 8, null fails and 0 and undefined work.

EDIT: we have been using null for this since like node 0.8 or something, so this recent update broke us, hence why I noticed this conversation starting over on the express repo :)

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Jul 12, 2017
@sam-github
Copy link
Contributor

This is a regression, IMO.

  • 4.x: null === undefined === 0
  • 6.x: null === 0; undefined errors
  • 7.x/8.x: null errors; undefined === 0

6.x and 8.x doing the opposite of each other doesn't look intentional, both should behave like 4.x.

% nvm i 4
Downloading https://nodejs.org/dist/v4.8.4/node-v4.8.4-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v4.8.4 (npm v2.15.11)
% node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}).unref(); s.address()'
{ address: '::', family: 'IPv6', port: 36947 }
% node -pe 'var s = require("http").createServer(function(){}).listen(null, function(){}).unref(); s.address()'
{ address: '::', family: 'IPv6', port: 46595 }
% nvm i 6
Downloading https://nodejs.org/dist/v6.11.1/node-v6.11.1-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v6.11.1 (npm v3.10.10)
% node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}).unref(); s.address()'
internal/net.js:17
    throw new RangeError('"port" argument must be >= 0 and < 65536');
    ^

RangeError: "port" argument must be >= 0 and < 65536
    at assertPort (internal/net.js:17:11)
    at Server.listen (net.js:1389:5)
    at [eval]:1:52
    at ContextifyScript.Script.runInThisContext (vm.js:25:33)
    at Object.runInThisContext (vm.js:97:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:570:32)
    at evalScript (bootstrap_node.js:353:27)
    at run (bootstrap_node.js:122:11)
    at run (bootstrap_node.js:389:7)
% node -pe 'var s = require("http").createServer(function(){}).listen(null, function(){}).unref(); s.address()'
{ address: '::', family: 'IPv6', port: 40967 }
% nvm i 7
Downloading https://nodejs.org/dist/v7.10.1/node-v7.10.1-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v7.10.1 (npm v4.2.0)
% node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}).unref(); s.address()'
{ address: '::', family: 'IPv6', port: 41121 }
% node -pe 'var s = require("http").createServer(function(){}).listen(null, function(){}).unref(); s.address()'
net.js:1422
  throw new Error('Invalid listen argument: ' + options);
  ^

Error: Invalid listen argument: [object Object]
    at Server.listen (net.js:1422:9)
    at [eval]:1:52
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at Object.runInThisContext (vm.js:95:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:571:32)
    at evalScript (bootstrap_node.js:391:27)
    at run (bootstrap_node.js:124:11)
    at run (bootstrap_node.js:427:7)
    at startup (bootstrap_node.js:123:9)
% nvm i 8
Downloading https://nodejs.org/dist/v8.1.4/node-v8.1.4-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v8.1.4 (npm v5.0.3)
% node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}).unref(); s.address()'
{ address: '::', family: 'IPv6', port: 33561 }
% node -pe 'var s = require("http").createServer(function(){}).listen(null, function(){}).unref(); s.address()'
net.js:1479
  throw new Error('Invalid listen argument: ' + util.inspect(options));
  ^

Error: Invalid listen argument: { port: null }
    at Server.listen (net.js:1479:9)
    at [eval]:1:52
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:569:30)
    at evalScript (bootstrap_node.js:432:27)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:575:3

@sam-github sam-github added confirmed-bug Issues with confirmed bugs. v6.x labels Jul 12, 2017
@evanlucas
Copy link
Contributor

@sam-github I agree. I didn't realize this behavior had changed

@sam-github
Copy link
Contributor

@cjihrig I'm working on fix for 6.x, FYI /cc @nodejs/lts

sam-github added a commit to sam-github/node that referenced this issue Jul 17, 2017
Existing test only covered ports pass as options, add tests for
argument behaviour to avoid regressions.

Ref: nodejs#14205
sam-github added a commit that referenced this issue Jul 21, 2017
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 21, 2017
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 1, 2017
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
sam-github added a commit to sam-github/node that referenced this issue Sep 19, 2017
For consistency with 4.x and 8.x.

This commit also contains a forward port of
nodejs#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: nodejs#14234
Refs: nodejs#14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 20, 2017
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

Backport-PR-URL: #14234
PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this issue Sep 22, 2017
This commit fixes a regression around the handling of null
as the port passed to Server#listen(). With this commit,
null, undefined, and 0 have the same behavior, which was the
case in Node 4.

Fixes: nodejs#14205
PR-URL: nodejs#14221
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 23, 2017
This commit fixes a regression around the handling of null
as the port passed to Server#listen(). With this commit,
null, undefined, and 0 have the same behavior, which was the
case in Node 4.

Fixes: nodejs/node#14205
PR-URL: nodejs/node#14221
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Sep 25, 2017
This commit fixes a regression around the handling of null
as the port passed to Server#listen(). With this commit,
null, undefined, and 0 have the same behavior, which was the
case in Node 4.

Fixes: #14205
PR-URL: #14221
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.

5 participants