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.

Sign up for GitHub

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

lib/net: Convert to using internal/errors #14782

Closed
wants to merge 10 commits into from
Closed

lib/net: Convert to using internal/errors #14782

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0cc1ea6
lib/net: Convert to using internal/errors
Aug 11, 2017
00b8f49
[fixup] lib/net: update tests
Aug 20, 2017
16d8519
[fixup] lib/net: refac ERR_INVALID_ARG_TYPE err
Aug 20, 2017
90eb1a5
[fixup] lib/net: update Server.listen() err msg
Aug 21, 2017
056dfcc
[fixup] lib/net.js update createHandle() err
Aug 24, 2017
7373ee2
[fixup] lib/net: ERR_INVALID_FD_TYPE add test
Aug 24, 2017
787682b
[fixup] lib/net: correct err types
Aug 25, 2017
74d6b0b
[fixup] lib/net: ERR_SOCKET_BAD_PORT handle port
Aug 29, 2017
5a4e982
[fixup] lib/net: simplify err ERR_SOCKET_BAD_PORT
Sep 1, 2017
30a1ca0
[fixup] lib/net: remove failing test
Sep 27, 2017
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
5 changes: 5 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,11 @@ Used when `hostname` can not be parsed from a provided URL.

Used when a file descriptor ('fd') is not valid (e.g. it has a negative value).

<a id="ERR_INVALID_FD_TYPE"></a>
### ERR_INVALID_FD_TYPE

Used when a file descriptor ('fd') type is not valid.

<a id="ERR_INVALID_FILE_URL_HOST"></a>
### ERR_INVALID_FILE_URL_HOST

Expand Down
2 changes: 1 addition & 1 deletion lib/dgram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ Socket.prototype.send = function(buffer,

port = port >>> 0;
if (port === 0 || port > 65535)
throw new errors.RangeError('ERR_SOCKET_BAD_PORT');
throw new errors.RangeError('ERR_SOCKET_BAD_PORT', port);

// Normalize callback so it's either a function or undefined but not anything
// else.
Expand Down
2 changes: 1 addition & 1 deletion lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ function lookupService(host, port, callback) {
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'host', host);

if (!isLegalPort(port))
throw new errors.RangeError('ERR_SOCKET_BAD_PORT');
throw new errors.RangeError('ERR_SOCKET_BAD_PORT', port);

if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ E('ERR_INVALID_CURSOR_POS',
'Cannot set cursor row without setting its column');
E('ERR_INVALID_DOMAIN_NAME', 'Unable to determine the domain name');
E('ERR_INVALID_FD', '"fd" must be a positive integer: %s');
E('ERR_INVALID_FD_TYPE', 'Unsupported fd type: %s');
E('ERR_INVALID_FILE_URL_HOST',
'File URL host must be "localhost" or empty on %s');
E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');
Expand Down Expand Up @@ -266,7 +267,7 @@ E('ERR_SERVER_ALREADY_LISTEN',
'Listen method has been called more than once without closing.');
E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound');
E('ERR_SOCKET_BAD_BUFFER_SIZE', 'Buffer size must be a positive integer');
E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536. Received %s.');
E('ERR_SOCKET_BAD_TYPE',
'Bad socket type specified. Valid types are: udp4, udp6');
E('ERR_SOCKET_BUFFER_SIZE',
Expand Down
42 changes: 28 additions & 14 deletions lib/net.js
View file
Open in desktop
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.TypeError('ERR_INVALID_FD_TYPE', type);
}


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.TypeError('ERR_INVALID_IP_ADDRESS', localAddress);
}

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',
'options.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', port);
Copy link
Member

Choose a reason for hiding this comment

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

The error type must also be able to handle the provided port.

Copy link
Author

Choose a reason for hiding this comment

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

I have made a change to include the port in the err msg if the port arg is passed to the error constructor.

}
}
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
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
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
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',
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', options.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.Error('ERR_INVALID_OPT_VALUE',
Copy link
Member

@joyeecheung joyeecheung Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think we probably need a new error for this or change ERR_INVALID_OPT_VALUE. For one thing this would look like The value "{ port: null }" is invalid for option "options" when you do listen(null), which is a bit confusing because the user does not pass a string "{ port: null }" there. Also technically ERR_INVALID_OPT_VALUE indicates that the values in options are invalid, not the options itself, this method convert arguments into an options object so it is kind of a mix of ERR_INVALID_OPT_VALUE and ERR_INVALID_ARG_TYPE..

This applies to socket.connect() as well.

Copy link
Member

Choose a reason for hiding this comment

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

EDIT: NaN does not get this far, null does.

Copy link
Author

@pmatzavin pmatzavin Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We kind of stretched the ERR_INVALID_OPT_VALUE error here to fit in this specific case. Trying to minimize too specific error-codes that may not be used elsewhere. There is also a discussion about it above: #14782 (comment). If the current implementation is not sufficient we could go with a new error.

Copy link
Member

@joyeecheung joyeecheung Sep 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@pmatzavin I think a new error would not be too specific because neither ERR_INVALID_OPT_VALUE nor ERR_INVALID_ARG_TYPE properly describe errors thrown by this kind of "overloading arguments & options" methods, and the messages that they produce are a bit confusing for these errors. Also there are at least more than one method that do this (server.listen() and socket.connect()).

Copy link
Author

@pmatzavin pmatzavin Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In net.js the ERR_INVALID_OPT_VALUE err constructor is directly called only (once) in Server.prototype.listen. IMHO the first part of the msg The value "{ port: null }" is invalid accurately describes the cause of the error (it does not mention if the user provided that value or not). The second part of the message for option "options" may be confusing and is not 100% accurate, that's why i said that we kind of stretched the ERR_INVALID_OPT_VALUE to fit this specific case. Either way the error is that a value is invalid inside the options object. I agree that a new error like ERR_INVALID_OPTION_OBJECT may be more accurate but i am not sure if it is necessary. Any more feedback-should we change this?

Copy link
Member

@BridgeAR BridgeAR Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@joyeecheung after looking at this further I somewhat agree with you that it is not very intuitive as it is right now. Even though the original error was not really any better. But our current errors mainly reflect the code they are executed in. So most messages are e.g. a clear 1-to-1 of the argument name that is used in the code. The user will have to check that line of code from time to time to understand what the explicit error stands for and this might be such a case.

I am not certain that finding a good solution for this should hold up the PR and I personally think we should try to rework the errors in general as soon as they are all ported over.

'options',
util.inspect(options));
};

function lookupAndListen(self, port, address, backlog, exclusive) {
Expand Down
35 changes: 17 additions & 18 deletions test/parallel/test-dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,24 +220,23 @@ assert.throws(() => {
message: `The value "${invalidHost}" is invalid for option "host"`
}));

const badPortMsg = common.expectsError({
code: 'ERR_SOCKET_BAD_PORT',
type: RangeError,
message: 'Port should be > 0 and < 65536'
}, 4);
assert.throws(() => dns.lookupService('0.0.0.0', null, common.mustNotCall()),
badPortMsg);

assert.throws(
() => dns.lookupService('0.0.0.0', undefined, common.mustNotCall()),
badPortMsg
);

assert.throws(() => dns.lookupService('0.0.0.0', 65538, common.mustNotCall()),
badPortMsg);

assert.throws(() => dns.lookupService('0.0.0.0', 'test', common.mustNotCall()),
badPortMsg);
const portErr = (port) => {
common.expectsError(
() => {
dns.lookupService('0.0.0.0', port, common.mustNotCall());
},
{
code: 'ERR_SOCKET_BAD_PORT',
message:
`Port should be > 0 and < 65536. Received ${port}.`,
type: RangeError
}
);
};
portErr(null);
portErr(undefined);
portErr(65538);
portErr('test');

assert.throws(() => {
dns.lookupService('0.0.0.0', 80, null);
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-internal-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,10 @@ assert.strictEqual(
errors.message('ERR_INVALID_ARG_TYPE', [['a', 'b', 'c'], 'not d']),
'The "a", "b", "c" arguments must not be of type d');

// Test ERR_INVALID_FD_TYPE
assert.strictEqual(errors.message('ERR_INVALID_FD_TYPE', ['a']),
'Unsupported fd type: a');

// Test ERR_INVALID_URL_SCHEME
assert.strictEqual(errors.message('ERR_INVALID_URL_SCHEME', ['file']),
'The URL must be of scheme file');
Expand Down Expand Up @@ -237,6 +241,10 @@ assert.throws(
message: /^At least one arg needs to be specified$/
}));

// Test ERR_SOCKET_BAD_PORT
assert.strictEqual(
errors.message('ERR_SOCKET_BAD_PORT', [0]),
'Port should be > 0 and < 65536. Received 0.');

// Test ERR_TLS_CERT_ALTNAME_INVALID
assert.strictEqual(
Expand Down
65 changes: 35 additions & 30 deletions test/parallel/test-net-connect-options-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,35 +27,33 @@ 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}$`);
}

syncFailToConnect(true, portTypeError('true'));
syncFailToConnect(false, portTypeError('false'));
syncFailToConnect([], portTypeError(''), true);
syncFailToConnect({}, portTypeError('[object Object]'), true);
syncFailToConnect(null, portTypeError('null'));
const portTypeError = common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
}, 96);

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}$`);
}

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'));
const portRangeError = common.expectsError({
code: 'ERR_SOCKET_BAD_PORT',
type: RangeError
}, 168);

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 +127,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
25 changes: 12 additions & 13 deletions test/parallel/test-net-localerror.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,24 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

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

// Using port 0 as localPort / localAddress is already invalid.
const connect = (opts, code, type) => {
common.expectsError(
() => net.connect(opts),
{ code, type }
);
};

connect({
host: 'localhost',
port: 0,
localPort: 'foobar',
}, /^TypeError: "localPort" option should be a number: foobar$/);
localAddress: 'foobar',
}, 'ERR_INVALID_IP_ADDRESS', TypeError);

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

function connect(opts, msg) {
assert.throws(() => {
net.connect(opts);
}, msg);
}
localPort: 'foobar',
}, 'ERR_INVALID_ARG_TYPE', TypeError);
9 changes: 5 additions & 4 deletions test/parallel/test-net-options-lookup.js
View file
Open in desktop
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
Loading