-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
lib/net: Convert to using internal/errors #14782
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
Changes from 1 commit
0cc1ea6
00b8f49
16d8519
90eb1a5
056dfcc
7373ee2
787682b
74d6b0b
5a4e982
30a1ca0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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'); | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -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); | ||
| }; | ||
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this changed to a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually think not using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
|
|
||
| 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'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would be nice 👍
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I commited the change to pass the port value in the |
||
| } | ||
| } | ||
| port |= 0; | ||
|
|
@@ -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', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should likely be
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are thrown errors for all the following 4 values in So I guess that for consistency, all the errors thrown for invalid values of the above vars should have a code of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 'options.lookup', | ||
| ['function'], | ||
| options.lookup); | ||
|
|
||
| var dnsopts = { | ||
| family: options.family, | ||
|
|
@@ -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'], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/ When there is a single value, it doesn't need to be in an array |
||
| options); | ||
| } | ||
|
|
||
| this._connections = 0; | ||
|
|
@@ -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 | ||
|
|
@@ -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', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this should be just which seems a tiny bit better then |
||
| util.inspect(options)); | ||
| }; | ||
|
|
||
| function lookupAndListen(self, port, address, backlog, exclusive) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @refack I have read the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO simply add a commit, since it makes re-reviews easier. |
||
| 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 | ||
|
|
@@ -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'})`); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 })); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)); | ||
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, this one should be a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }); | ||
|
|
||
| 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 | ||
| })); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmatzavin look at
node/lib/internal/errors.js
Line 200 in cbd3708
You can add just after it: