Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
lib/net: Convert to using internal/errors
Covert lib/net.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-12926

- Replace thrown errors in lib/net.js
  with errors from lib/internal/errors.
  The ERR_INVALID_OPT_VALUE error have been used
  in the Server.prototype.listen() method
  after a discussion in Ref: #14782
- Update tests according to the above modifications
  • Loading branch information
matzavinos authored and jasnell committed Sep 25, 2017
commit 0cc1ea669b505be6082858577528d992b451b60f
42 changes: 28 additions & 14 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ const normalizedArgsSymbol = internalNet.normalizedArgsSymbol;
function noop() {}

function createHandle(fd) {
var type = TTYWrap.guessHandleType(fd);
const type = TTYWrap.guessHandleType(fd);
if (type === 'PIPE') return new Pipe();
if (type === 'TCP') return new TCP();
throw new TypeError('Unsupported fd type: ' + type);
throw new errors.Error('ERR_INVALID_HANDLE_TYPE');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the message to include the handle type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmatzavin look at

E('ERR_INVALID_FD', '"fd" must be a positive integer: %s');

You can add just after it:

E('ERR_INVALID_FD_TYPE', 'Unsupported fd type: %s');

}


Expand Down Expand Up @@ -695,8 +695,10 @@ protoGetter('localPort', function localPort() {

Socket.prototype.write = function(chunk, encoding, cb) {
if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
throw new TypeError(
'Invalid data, chunk must be a string or buffer, not ' + typeof chunk);
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'chunk',
['string', 'Buffer'],
chunk);
}
return stream.Duplex.prototype.write.apply(this, arguments);
};
Expand Down Expand Up @@ -1035,21 +1037,25 @@ function lookupAndConnect(self, options) {
var localPort = options.localPort;

if (localAddress && !cares.isIP(localAddress)) {
throw new TypeError('"localAddress" option must be a valid IP: ' +
localAddress);
throw new errors.Error('ERR_INVALID_IP_ADDRESS', localAddress);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this changed to a Error? It was a TypeError before?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think not using TypeError is appropriate here. I've never felt that TypeError was a good fit.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad. I corrected this, now the error types in lib/net.js have the correct value.

Copy link
Copy Markdown
Author

@pmatzavin pmatzavin Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell i have already committed (some minutes ago) a change that preserve the error types as they were in lib/net.js. So this is now a TypeError as before. Should I re-change it?

}

if (localPort && typeof localPort !== 'number') {
throw new TypeError('"localPort" option should be a number: ' + localPort);
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options.localPort',
['number'],
localPort);
}

if (typeof port !== 'undefined') {
if (typeof port !== 'number' && typeof port !== 'string') {
throw new TypeError('"port" option should be a number or string: ' +
port);
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'otions.port',
['number', 'string'],
port);
}
if (!isLegalPort(port)) {
throw new RangeError('"port" option should be >= 0 and < 65536: ' + port);
throw new errors.RangeError('ERR_SOCKET_BAD_PORT');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the port be passed as a parameter to the throw here, and also on line 1475?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be nice 👍

Copy link
Copy Markdown
Author

@pmatzavin pmatzavin Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ERR_SOCKET_BAD_PORT definition in lib/internal/errors.js
does not handle a port argument. Should I refactor it to something like this?

E('ERR_SOCKET_BAD_PORT', (port) => {
  const portMsg = port ? `: ${port}` : '';
  return `Port${portMsg} should be > 0 and < 65536`;
});

Copy link
Copy Markdown
Author

@pmatzavin pmatzavin Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commited the change to pass the port value in the 'ERR_SOCKET_BAD_PORT' call in lib/net.js. This will not be included in the err msg until we modify the E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536'); in lib/internal/errors.js(see my above comment/question)

}
}
port |= 0;
Expand All @@ -1065,7 +1071,10 @@ function lookupAndConnect(self, options) {
}

if (options.lookup && typeof options.lookup !== 'function')
throw new TypeError('"lookup" option should be a function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be ERR_INVALID_OPTION_VALUE

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are thrown errors for all the following 4 values in lib/net lookupAndConnect function :

function lookupAndConnect(self, options) {
  // ...
  options.port
  options.localAddress
   options.localPort
  options.lookup
  // ...

So I guess that for consistency, all the errors thrown for invalid values of the above vars should have a code of ERR_INVALID_OPTION_VALUE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked how we use mainly use this right now in the code base and it seems like ERR_INVALID_ARG_TYPE is the right type here. We might improve those types in general later on further but I think this should not block the PR from being merged.

'options.lookup',
['function'],
options.lookup);

var dnsopts = {
family: options.family,
Expand Down Expand Up @@ -1207,7 +1216,10 @@ function Server(options, connectionListener) {
this.on('connection', connectionListener);
}
} else {
throw new TypeError('options must be an object');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options',
['object'],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/[object']/`Object'

When there is a single value, it doesn't need to be in an array

options);
}

this._connections = 0;
Expand Down Expand Up @@ -1465,7 +1477,7 @@ Server.prototype.listen = function(...args) {
var backlog;
if (typeof options.port === 'number' || typeof options.port === 'string') {
if (!isLegalPort(options.port)) {
throw new RangeError('"port" argument must be >= 0 and < 65536');
throw new errors.RangeError('ERR_SOCKET_BAD_PORT');
}
backlog = options.backlog || backlogFromArgs;
// start TCP server listening on host:port
Expand All @@ -1490,7 +1502,9 @@ Server.prototype.listen = function(...args) {
return this;
}

throw new Error('Invalid listen argument: ' + util.inspect(options));
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'listen options',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this should be just options so it will be rendered as:

The value "{ gaga: 4 }" is invalid for option "options"

which seems a tiny bit better then
https://github.com/nodejs/node/blob/77b02d2d8345c407dc0e0121420d823b2ce6fb18/test/parallel/test-net-server-listen-options.js#L63

util.inspect(options));
};

function lookupAndListen(self, port, address, backlog, exclusive) {
Expand Down
61 changes: 35 additions & 26 deletions test/parallel/test-net-connect-options-port.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,35 +27,37 @@ const net = require('net');

// Test wrong type of ports
{
function portTypeError(opt) {
const prefix = '"port" option should be a number or string: ';
const cleaned = opt.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
return new RegExp(`^TypeError: ${prefix}${cleaned}$`);
function portTypeError() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace this with:

const portTypeError = common.expectsError({
  code: 'ERR_INVALID_ARG_TYPE',
  type: TypeError
}, 5);
and the same in L46

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack I have read the CONTRIBUTING.md but I am not sure which of the following should I use to make the requested changes:

  1. amend the commit and push --force-with-lease
  2. git commit --fixup
  3. simply add a new commit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO simply add a commit, since it makes re-reviews easier.
A common practice is to prefix the new commit's message with [fixup] ... so it's clear that it should be squashed when the PR is landed.

return common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});
}

syncFailToConnect(true, portTypeError('true'));
syncFailToConnect(false, portTypeError('false'));
syncFailToConnect([], portTypeError(''), true);
syncFailToConnect({}, portTypeError('[object Object]'), true);
syncFailToConnect(null, portTypeError('null'));
syncFailToConnect(true, portTypeError);
syncFailToConnect(false, portTypeError);
syncFailToConnect([], portTypeError, true);
syncFailToConnect({}, portTypeError, true);
syncFailToConnect(null, portTypeError);
}

// Test out of range ports
{
function portRangeError(opt) {
const prefix = '"port" option should be >= 0 and < 65536: ';
const cleaned = opt.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
return new RegExp(`^RangeError: ${prefix}${cleaned}$`);
function portRangeError() {
return common.expectsError({
code: 'ERR_SOCKET_BAD_PORT',
type: RangeError
});
}

syncFailToConnect('', portRangeError(''));
syncFailToConnect(' ', portRangeError(' '));
syncFailToConnect('0x', portRangeError('0x'), true);
syncFailToConnect('-0x1', portRangeError('-0x1'), true);
syncFailToConnect(NaN, portRangeError('NaN'));
syncFailToConnect(Infinity, portRangeError('Infinity'));
syncFailToConnect(-1, portRangeError('-1'));
syncFailToConnect(65536, portRangeError('65536'));
syncFailToConnect('', portRangeError);
syncFailToConnect(' ', portRangeError);
syncFailToConnect('0x', portRangeError, true);
syncFailToConnect('-0x1', portRangeError, true);
syncFailToConnect(NaN, portRangeError);
syncFailToConnect(Infinity, portRangeError);
syncFailToConnect(-1, portRangeError);
syncFailToConnect(65536, portRangeError);
}

// Test invalid hints
Expand Down Expand Up @@ -129,33 +131,40 @@ function doConnect(args, getCb) {
];
}

function syncFailToConnect(port, regexp, optOnly) {
function syncFailToConnect(port, assertErr, optOnly) {
if (!optOnly) {
// connect(port, cb) and connect(port)
const portArgBlocks = doConnect([port], () => common.mustNotCall());
for (const block of portArgBlocks) {
assert.throws(block, regexp, `${block.name}(${port})`);
assert.throws(block,
assertErr(),
`${block.name}(${port})`);
}

// connect(port, host, cb) and connect(port, host)
const portHostArgBlocks = doConnect([port, 'localhost'],
() => common.mustNotCall());
for (const block of portHostArgBlocks) {
assert.throws(block, regexp, `${block.name}(${port}, 'localhost')`);
assert.throws(block,
assertErr(),
`${block.name}(${port}, 'localhost')`);
}
}
// connect({port}, cb) and connect({port})
const portOptBlocks = doConnect([{ port }],
() => common.mustNotCall());
for (const block of portOptBlocks) {
assert.throws(block, regexp, `${block.name}({port: ${port}})`);
assert.throws(block,
assertErr(),
`${block.name}({port: ${port}})`);
}

// connect({port, host}, cb) and connect({port, host})
const portHostOptBlocks = doConnect([{ port: port, host: 'localhost' }],
() => common.mustNotCall());
for (const block of portHostOptBlocks) {
assert.throws(block, regexp,
assert.throws(block,
assertErr(),
`${block.name}({port: ${port}, host: 'localhost'})`);
}
}
Expand Down
18 changes: 8 additions & 10 deletions test/parallel/test-net-localerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,23 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');
const net = require('net');

// Using port 0 as localPort / localAddress is already invalid.
connect({
host: 'localhost',
port: 0,
localPort: 'foobar',
}, /^TypeError: "localPort" option should be a number: foobar$/);
localAddress: 'foobar',
}, 'ERR_INVALID_IP_ADDRESS', Error);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a TypeError before?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad. I corrected this, now the error types in lib/net.js have the correct value.


connect({
host: 'localhost',
port: 0,
localAddress: 'foobar',
}, /^TypeError: "localAddress" option must be a valid IP: foobar$/);
localPort: 'foobar',
}, 'ERR_INVALID_ARG_TYPE', TypeError);

function connect(opts, msg) {
assert.throws(() => {
net.connect(opts);
}, msg);
function connect(opts, code, type) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be:

const connect = (opts, code, type) => common.expectsError(
  () => net.connect(opts),
  { code, type }
);

assert.throws(() => net.connect(opts),
common.expectsError({ code: code, type: type }));
}
9 changes: 5 additions & 4 deletions test/parallel/test-net-options-lookup.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');
const net = require('net');

const expectedError = /^TypeError: "lookup" option should be a function$/;

['foobar', 1, {}, []].forEach((input) => connectThrows(input));

// Using port 0 as lookup is emitted before connecting.
Expand All @@ -17,7 +15,10 @@ function connectThrows(input) {

assert.throws(() => {
net.connect(opts);
}, expectedError);
}, common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
}));
}

connectDoesNotThrow(() => {});
Expand Down
51 changes: 22 additions & 29 deletions test/parallel/test-net-server-listen-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,9 @@
const common = require('../common');
const assert = require('assert');
const net = require('net');
const util = require('util');

function close() { this.close(); }

function listenError(literals, ...values) {
let result = literals[0];
for (const [i, value] of values.entries()) {
const str = util.inspect(value);
// Need to escape special characters.
result += str.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
result += literals[i + 1];
}
return new RegExp(`Error: Invalid listen argument: ${result}`);
}

{
// Test listen()
net.createServer().listen().on('listening', common.mustCall(close));
Expand All @@ -36,45 +24,50 @@ const listenOnPort = [
];

{
const portError = /^RangeError: "port" argument must be >= 0 and < 65536$/;
const assertPort = () => {
return common.expectsError({
code: 'ERR_SOCKET_BAD_PORT',
type: Error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a RangeError before?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, this one should be a RangeError

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad. I corrected this, now the error types in lib/net.js have the correct value.

});
};

for (const listen of listenOnPort) {
// Arbitrary unused ports
listen('0', common.mustCall(close));
listen(0, common.mustCall(close));
listen(undefined, common.mustCall(close));
listen(null, common.mustCall(close));
// Test invalid ports
assert.throws(() => listen(-1, common.mustNotCall()), portError);
assert.throws(() => listen(NaN, common.mustNotCall()), portError);
assert.throws(() => listen(123.456, common.mustNotCall()), portError);
assert.throws(() => listen(65536, common.mustNotCall()), portError);
assert.throws(() => listen(1 / 0, common.mustNotCall()), portError);
assert.throws(() => listen(-1 / 0, common.mustNotCall()), portError);
assert.throws(() => listen(-1, common.mustNotCall()), assertPort());
assert.throws(() => listen(NaN, common.mustNotCall()), assertPort());
assert.throws(() => listen(123.456, common.mustNotCall()), assertPort());
assert.throws(() => listen(65536, common.mustNotCall()), assertPort());
assert.throws(() => listen(1 / 0, common.mustNotCall()), assertPort());
assert.throws(() => listen(-1 / 0, common.mustNotCall()), assertPort());
}
// In listen(options, cb), port takes precedence over path
assert.throws(() => {
net.createServer().listen({ port: -1, path: common.PIPE },
common.mustNotCall());
}, portError);
}, assertPort());
}

{
function shouldFailToListen(options, optionsInMessage) {
// Plain arguments get normalized into an object before
// formatted in message, options objects don't.
if (arguments.length === 1) {
optionsInMessage = options;
}
function shouldFailToListen(options) {
const block = () => {
net.createServer().listen(options, common.mustNotCall());
};
assert.throws(block, listenError`${optionsInMessage}`,
`expect listen(${util.inspect(options)}) to throw`);
assert.throws(block,
common.expectsError({
code: 'ERR_INVALID_OPT_VALUE',
type: TypeError,
message: /^The value "{.*}" is invalid for option "listen options"$/
}));
}

shouldFailToListen(false, { port: false });
shouldFailToListen({ port: false });
shouldFailToListen(true, { port: true });
shouldFailToListen(true);
shouldFailToListen({ port: true });
// Invalid fd as listen(handle)
shouldFailToListen({ fd: -1 });
Expand Down
13 changes: 10 additions & 3 deletions test/parallel/test-net-server-options.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
'use strict';
require('../common');
const assert = require('assert');
const common = require('../common');
const net = require('net');

assert.throws(function() { net.createServer('path'); },
/^TypeError: options must be an object$/);
common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
}));

assert.throws(function() { net.createServer(0); },
/^TypeError: options must be an object$/);
common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
}));
10 changes: 6 additions & 4 deletions test/parallel/test-net-socket-write-error.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
'use strict';

require('../common');
const common = require('../common');
const assert = require('assert');
const net = require('net');

const server = net.createServer().listen(0, connectToServer);

function connectToServer() {
const client = net.createConnection(this.address().port, () => {
assert.throws(() => {
client.write(1337);
}, /Invalid data, chunk must be a string or buffer, not number/);
assert.throws(() => client.write(1337),
common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
}));

client.end();
})
Expand Down
Loading