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

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

Closed
wants to merge 10 commits into from

Conversation

@pmatzavin
Copy link
Contributor

@pmatzavin pmatzavin commented Aug 12, 2017

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
  • Update tests according to the above modification
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

lib/net, doc

doc/api/errors.md Outdated
@@ -1004,6 +1004,22 @@ Used when a callback is called more then once.
can either be fulfilled or rejected but not both at the same time. The latter
would be possible by calling a callback more then once.

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

This comment has been minimized.

@Trott

Trott Aug 12, 2017
Member

Any reason not to use the existing ERR_INVALID_ARG_TYPE instead of creating this new error?

This comment has been minimized.

@refack

refack Aug 12, 2017
Member

There is an ongoing debate on whether error codes should be very specific (like this case), or more general (like what @Trott is suggesting)
For now the trend is towards a clear separation between bad arguments or options, and run time errors. But minimal separation between different argument errors.
@pmatzavin IMHO, the description you've given here should be in the net.Server.listen API documentation, and ERR_INVALID_ARG_TYPE is good enough.

This comment has been minimized.

@pmatzavin

pmatzavin Aug 15, 2017
Author Contributor

@Trott @refack The reason that I used this new error was that the listen arg had many valid combinations of types/schema. I was not sure about the way that I handled it. I see your point and I agree with you. I will replaced it, with ERR_INVALID_ARG_TYPE.

This comment has been minimized.

@pmatzavin

pmatzavin Aug 19, 2017
Author Contributor

@Trott @refack I am having trouble using the 'ERR_INVALID_ARG_TYPE' for the
Server.prototype.listen() in the lib/net.js.
I am describing the problem below :

The Server.prototype.listen() implementation
has some early return statements for the following cases:

options = options._handle || options.handle || options;

  • options instanceof TCP
  • typeof options.fd === 'number' && options.fd >= 0
  • (args.length === 0 || typeof args[0] === 'function' || (typeof options.port === 'undefined' && 'port' in options)) || typeof options.port === 'number' || typeof options.port === 'string'
  • options.path && isPipeName(options.path)

At the end, if none of these early return statements runs, then the error is thrown.

So I am having trouble using the
E('ERR_INVALID_ARG_TYPE', invalidArgType) error.
As I cannot compute a suitable error message using the
invalidArgType(name, expected, actual) (file: lib/internal/errors.js) callback,
for the case that I described above.

I have not yet found a similar case in the source code.
That's why I used the new error ERR_NET_INVALID_LISTEN_ARG in the first place.
Do you have any suggestions on how should I handle this case or on how should I use the ERR_INVALID_ARG_TYPE error for this case?

Should I refactor the invalidArgType method in the lib/internal/errors.js
to return a more generic error message (like: invalid type)
if the expected argument is not specified?

This comment has been minimized.

@Trott

Trott Aug 19, 2017
Member

If I'm understanding correctly, the error isn't (necessarily?) an invalid argument type, but an invalid argument.

If we can make ERR_INVALID_OPT_VALUE work instead, that would be great. Will that result in a similar difficulty? If so, then I'm OK with a new error. If we've been trying for more general errors, E_INVALID_OPTION_OBJECT or something like that might be preferable so it can be re-used elsewhere. But there's a valid argument for specific codes too, so either way is fine by me.

This comment has been minimized.

@pmatzavin

pmatzavin Aug 20, 2017
Author Contributor

@Trott I used the ERR_INVALID_OPT_VALUE error instead of creating a new error.
I amended the change and pushed -f to the current pull request.

Copy link
Member

@Trott Trott left a comment

Hi @pmatzavin! Welcome and thanks for the PR! I left a question/suggestion for a change.

@refack refack added the semver-major label Aug 12, 2017
@pmatzavin
Copy link
Contributor Author

@pmatzavin pmatzavin commented Aug 15, 2017

Hi @Trott! Thanks for the help and the suggestion. I am traveling right now without my laptop. I will update my PR by this Saturday.

@Trott Trott dismissed their stale review Aug 19, 2017

discussion happened, dismissing review

@Trott
Copy link
Member

@Trott Trott commented Aug 19, 2017

@nodejs/ctc This is semver-major, needs some CTC reviews.

@pmatzavin pmatzavin force-pushed the pmatzavin:lib_net_convert_err branch Aug 20, 2017
@pmatzavin
Copy link
Contributor Author

@pmatzavin pmatzavin commented Aug 20, 2017

updated

@refack
refack approved these changes Aug 20, 2017
Copy link
Member

@refack refack left a comment

LGTM
(just some nits)

test/parallel/test-net-connect-options-port.js Outdated
const prefix = '"port" option should be a number or string: ';
const cleaned = opt.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
return new RegExp(`^TypeError: ${prefix}${cleaned}$`);
function portTypeError() {

This comment has been minimized.

@refack

refack Aug 20, 2017
Member

You can replace this with:

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 20, 2017
Author Contributor

@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

This comment has been minimized.

@refack

refack Aug 20, 2017
Member

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.

test/parallel/test-net-localerror.js Outdated
assert.throws(() => {
net.connect(opts);
}, msg);
function connect(opts, code, type) {

This comment has been minimized.

@refack

refack Aug 20, 2017
Member

This can now be:

const connect = (opts, code, type) => common.expectsError(
  () => net.connect(opts),
  { code, type }
);
test/parallel/test-net-server-options.js Outdated
const assert = require('assert');
const common = require('../common');

This comment has been minimized.

@refack

refack Aug 20, 2017
Member

Actually require('../common'); should be first. It does some wiring, and maybe we'll want it to monkeypatch assert in the future.

@refack
Copy link
Member

@refack refack commented Aug 20, 2017

Hello @pmatzavin and welcome. Thank you very much for the contribution 🥇
If you haven't already, it's recommended you take a look at CONTRIBUTING.md guide (especialy the part about "discuss and update"). Customarily PRs are kept open for at least 48 hour so that anyone interested gets a chance to comment or review.
About Error migrations, since they are semver-major they require 2 CTC members' approval (which can take longer than usual PR approval). A consequence of that is that this PR might become conflicted with master, so you might be asked to rebase it late.
Anyway thank you for the contribution, and good luck 🎉

P.S. If you have any question you can also feel free to contact me directly (@ mention, email, IM, or IRC)

@refack refack added this to In Progress in Error Codes Aug 20, 2017
lib/net.js Outdated
throw new TypeError('options must be an object');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options',
['object'],

This comment has been minimized.

@jasnell

jasnell Aug 20, 2017
Member

s/[object']/`Object'

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

@rvagg
Copy link
Member

@rvagg rvagg commented Aug 20, 2017

SGTM, but I defer to @jasnell on the integration with the full new error messaging approach, when he gives a +1 then consider mine in here too

@refack
refack approved these changes Aug 21, 2017
Copy link
Member

@refack refack left a comment

Small non blocking nit

lib/net.js Outdated
@@ -1485,7 +1497,9 @@ Server.prototype.listen = function() {
return this;
}

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

This comment has been minimized.

@refack

refack Aug 21, 2017
Member

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

@refack
refack approved these changes Aug 21, 2017
Copy link
Member

@refack refack left a comment

💯

@refack
Copy link
Member

@refack refack commented Aug 21, 2017

@jasnell PTAL

lib/net.js Outdated
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');

This comment has been minimized.

@jasnell

jasnell Aug 23, 2017
Member

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

This comment has been minimized.

@refack

refack Aug 23, 2017
Member

@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');
test/parallel/test-net-localerror.js Outdated
const connect = (opts, code, type) => common.expectsError(
() => net.connect(opts),
{ code, type }
);

This comment has been minimized.

@jasnell

jasnell Aug 23, 2017
Member

this doesn't quite match our typical code format .... it should be something like

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

This comment has been minimized.

@refack

refack Aug 23, 2017
Member

That's my mistake.
@pmatzavin for arrow functions that span multiple lines, in node's code it's prefered to use the {} syntax for extra clarity of the scope (even if it is just one expression like here).

Copy link
Member

@jasnell jasnell left a comment

A few nits but generally LGTM

@pmatzavin
Copy link
Contributor Author

@pmatzavin pmatzavin commented Aug 25, 2017

@refack There was no test for the fd error thrown in the lib/net.js Socket() constructor so I added a test in a new file test/parallel/test-net-socket-fd-error.js PTAL. Sorry for the late update.

lib/net.js Outdated
}
if (!isLegalPort(port)) {
throw new RangeError('"port" option should be >= 0 and < 65536: ' + port);
throw new errors.RangeError('ERR_SOCKET_BAD_PORT');

This comment has been minimized.

@maclover7

maclover7 Aug 25, 2017
Member

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

This comment has been minimized.

@BridgeAR

BridgeAR Aug 25, 2017
Member

I think that would be nice 👍

This comment has been minimized.

@pmatzavin

pmatzavin Aug 25, 2017
Author Contributor

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 25, 2017
Author Contributor

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)

Copy link
Member

@BridgeAR BridgeAR left a comment

Thanks a lot for your contribution. It looks pretty good overall. A few Error classes seemed to be mistaken though.

Besides that it would be nice to use common.expectsError(throwingFunction, options) instead of assert.throws(throwingFunction, common.expectsError(options)) but this should not block the PR.

lib/net.js Outdated
@@ -1018,21 +1020,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);

This comment has been minimized.

@BridgeAR

BridgeAR Aug 25, 2017
Member

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

This comment has been minimized.

@jasnell

jasnell Aug 25, 2017
Member

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 25, 2017
Author Contributor

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 25, 2017
Author Contributor

@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?

test/parallel/test-net-server-listen-options.js Outdated
const assertPort = () => {
return common.expectsError({
code: 'ERR_SOCKET_BAD_PORT',
type: Error

This comment has been minimized.

@BridgeAR

BridgeAR Aug 25, 2017
Member

This was a RangeError before?

This comment has been minimized.

@jasnell

jasnell Aug 25, 2017
Member

Good point, this one should be a RangeError

This comment has been minimized.

@pmatzavin

pmatzavin Aug 25, 2017
Author Contributor

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

test/parallel/test-net-localerror.js Outdated
localPort: 'foobar',
}, /^TypeError: "localPort" option should be a number: foobar$/);
localAddress: 'foobar',
}, 'ERR_INVALID_IP_ADDRESS', Error);

This comment has been minimized.

@BridgeAR

BridgeAR Aug 25, 2017
Member

This was a TypeError before?

This comment has been minimized.

@pmatzavin

pmatzavin Aug 25, 2017
Author Contributor

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

}
if (!isLegalPort(port)) {
throw new RangeError('"port" option should be >= 0 and < 65536: ' + port);
throw new errors.RangeError('ERR_SOCKET_BAD_PORT', port);

This comment has been minimized.

@BridgeAR

BridgeAR Aug 29, 2017
Member

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 29, 2017
Author Contributor

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

lib/internal/errors.js Outdated
portMsg = `:${port}`;
}
return `Port${portMsg} should be > 0 and < 65536`;
});

This comment has been minimized.

@BridgeAR

BridgeAR Aug 31, 2017
Member

This is more complicated then it has to be. I would rather have

E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536. Received %s.');

This comment has been minimized.

@pmatzavin

pmatzavin Sep 1, 2017
Author Contributor

fixed (commit: b336aa1)

pmatzavin added 9 commits Aug 11, 2017
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
- refactor the portTypeError and portRangeError assertions,
  by passing the expected call count to common.expectsError in
  test-net-connect-options-port.js
  (Each syncFailToConnect() call will call 4 or 2 times the
  doConnect() method, wich returns an array of size 6,
  so the call counts are 96 and 168 for portTypeError and
  portRangeError respectively)
- refactor the connect() method in test-net-localerror.js,
  by removing the reduntant assert
  and use the common.expectsError() assertion
- move require('../common') to the top in test-net-server-options.js
- refactor the valid types argument of ERR_INVALID_ARG_TYPE errors,
  to be a string instead of an array when the number of valid types
  equals 1, in lib/net.js
replace "listen options" with "options"
- Create a new error 'ERR_INVALID_FD_TYPE'
  in lib/internal/errors.js,
  following the corresponding discussion at Ref: #14782.
  This error should be thrown
  when a file descriptor type is not valid.
- add test for the new err in test-internal-errors.js
- update the error doc in errors.md
- use the new err in createHandle() in lib/net.js
- Add brackets {} in arrow function in test-net-localerror.js
  to match the typical source code's format.
- add missing test for the 'ERR_INVALID_FD_TYPE' err that is thrown
  from the net.Socket constructor
- correct the err type from Error to TypeError
- correct 'ERR_INVALID_IP_ADDRESS' type to TypeError err in net.js
- correct 'ERR_SOCKET_BAD_PORT' type to RangeError in net.js
- correct 'ERR_INVALID_OPT_VALUE' type to Error in net.js
- pass the port value to the the 'ERR_SOCKET_BAD_PORT' err in net.js
- Modify the ERR_SOCKET_BAD_PORT definition in internal/errors.js
  to include the specified port in the err msg, if the port
  arg is passed to the err constructor.
- Add tests for ERR_SOCKET_BAD_PORT msg in test-internal-errors.js
- simplify the ERR_SOCKET_BAD_PORT error definition in errors.js
  by replacing the cb arg with the string:
  'Port should be > 0 and < 65536. Received %s.'
- update the ERR_SOCKET_BAD_PORT test in test-internal-errors.js
- update the ERR_SOCKET_BAD_PORT constructor call in
  lib/dgram.js and lib/dns to include the actual port
  as the second arg
- update the already existing ERR_SOCKET_BAD_PORT message
  assertion in test-dns.js, to include the actual port.
@jasnell jasnell force-pushed the pmatzavin:lib_net_convert_err branch to 5a4e982 Sep 25, 2017
@jasnell
Copy link
Member

@jasnell jasnell commented Sep 25, 2017

I went ahead and rebased the branch.

@@ -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',

This comment has been minimized.

@jasnell

jasnell Sep 25, 2017
Member

This should likely be ERR_INVALID_OPTION_VALUE

This comment has been minimized.

@pmatzavin

pmatzavin Sep 28, 2017
Author Contributor

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

This comment has been minimized.

@BridgeAR

BridgeAR Oct 2, 2017
Member

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.

@pmatzavin
Copy link
Contributor Author

@pmatzavin pmatzavin commented Sep 26, 2017

Sry for being unresponsive. I was unavailable due to some hlth problem. I will look into the issues.

@jasnell
Copy link
Member

@jasnell jasnell commented Sep 26, 2017

@pmatzavin ... no worries. Hope you're feeling better.

@pmatzavin
Copy link
Contributor Author

@pmatzavin pmatzavin commented Sep 27, 2017

I am getting different results when running the tests locally. The CI failing test(parallel/test-net-socket-fd-error) passes in my local env. I haven't found yet, why this can be inconsistent.
I double-checked that I have the correct source-code (after the rebase)

removed the
test/parallel/test-net-socket-fd-error.js

this test was added in the current PR
but is not consistent. As the used
fd mock value does not consistenly
resolve to a fd type not equal to 'PIPE'
and not equal to 'TCP'
@pmatzavin
Copy link
Contributor Author

@pmatzavin pmatzavin commented Sep 27, 2017

I removed the failing test / file:
test/parallel/test-net-socket-fd-error.js

This test was added by me in the current PR
but is not consistent. As the used
fd mock value does not consistently
resolve to a fd type not equal to 'PIPE'
and not equal to 'TCP'. The test was trying to assert the err in:

/* lib/net.js */
function createHandle(fd) {
  const type = TTYWrap.guessHandleType(fd);
  if (type === 'PIPE') return new Pipe();
  if (type === 'TCP') return new TCP();
  throw new errors.TypeError('ERR_INVALID_FD_TYPE', type);
}

by calling

new net.Socket({
 fd: 'invalid'
});

but i could not find a fd value/number that will fit for this test.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 27, 2017

@pmatzavin I think only one review has to be addressed and it should be good to go. I will start a CI as soon as that is done.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Oct 2, 2017

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 15, 2017

Landed in 7f55349

Note that there is still #15589 that we need to fix after this (See #14782 (review) regarding using ERR_INVALID_OPT_VALUE v.s. ERR_INVALID_ARG_TYPE v.s. a new error for the overloaded server.listen()). IMO We should not release this PR until that gets resolved.

joyeecheung added a commit that referenced this pull request Oct 15, 2017
Covert lib/net.js over to using lib/internal/errors.js

- 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
- Update tests according to the above modifications

PR-URL: #14782
Refs: #11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
Covert lib/net.js over to using lib/internal/errors.js

- 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
- Update tests according to the above modifications

PR-URL: nodejs/node#14782
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell moved this from In Progress to Done in Error Codes Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.