dns: throw a TypeError in lookupService with invalid port

[evanlucas]

Previously, port was assumed to be a number and would cause an abort in
cares_wrap. This change throws a TypeError if port is not a number
before we actually hit C++.

Fixes: #4837

[benjamingr]

Might be a good idea to add the failing value here:

throw new TypeError(`"port" argument must be a number, got ${port} instead.`);

That's not a regression from how other TypeErrors are thrown here though so not a biggie.

[benjamingr]

LGTM

[cjihrig]

I wonder if we could reuse isLegalPort() from lib/net.js.

EDIT: That would also requiring coercing the value.

[mscdex]

I think it might be better to coerce and check the result with isFinite(). Allowing a port number as a string here would be consistent with other subsystems that allow such inputs (e.g. http.request()).

[evanlucas]

I agree with accepting a string for the port for consistency, but will that make this semver-major since it is a behavior change?

[mscdex]

I would say so.

[evanlucas]

So, in that case, I think that we should at least throw a TypeError if port is not a number first, and then, in a separate PR, change it to coerce the value to a number and make sure it is in the proper port range as a semver-major change. Thoughts?

[benjamingr]

I definitely think coercion of numbers should be discussed first. I also think that this PR should go forward since it fixes a bug regardless of that issue (non-number ports aren't accepted as of today).

[cjihrig]

LGTM with comments.

[evanlucas]

ok, nits addressed (minus checking that the port is in a valid range). PTAL

[cjihrig]

LGTM

[r-52]

LGTM

[evanlucas]

@mscdex LGTY?

[mscdex]

LGTM

[evanlucas]

CI: https://ci.nodejs.org/job/node-test-pull-request/1374/

[evanlucas]

Landed in 0f8e63c. Thanks!

[jasnell]

Just noting: commits that add new throws need to be marked as semver-major :-) label added!

[evanlucas]

This was causing the process to abort previously. Should that still make it semver-major?

[cjihrig]

I would say replacing an abort with a thrown exception is not semver-major.

[jasnell]

hmm.. good point. :-)... in fact, in that case it's likely appropriate for LTS also.

[benjamingr]

Yeah, this is a bug fix over anything else

[rvagg]

@nodejs/lts cherry-picking this commit will bring over an assert for Invalid argument in an error name. This will fail because that name is new, and semver-major, on master. The commit needs to be changed to test for invalid argument (lower-case).