Undefined hostname defaults to 127.0.0.1

[skrat]

• Version: v7.6.0
• Platform: linux
• Subsystem: http

I'm not sure what's the idea behind defaulting to 127.0.0.1 in http.request when hostname is undefined, but it's not a good one. Please throw an error when hostname is anything but string, telling the user she's an idiot, rather than defaulting to some arbitrary IP address causing potentially dangerous and destructive side-effects (what if Trump's nukes are on the same port on 127.0.0.1?)

It's all cool that it's in the docs, but that doesn't mean it makes any sense or any good.

Common pitfall: undefined slips into the variable used to fill hostname, and all the user gets is this:

Error: connect ECONNREFUSED 127.0.0.1:80
    at Object.exports._errnoException (util.js:1028:11)
    at exports._exceptionWithHostPort (util.js:1051:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)

Not even a hint of what is going on, or where.

[bnoordhuis]

For some historical context: the current behavior is almost six years old to the day (commit a2328dc from April 25, 2011 and released in v0.5.0.)

In other words, node has behaved this way for most of its life and it is a near certainty that changing it now would cause (possibly significant) ecosystem breakage.

[skrat]

Good ol' let's perpetuate this bug because people rely on it argument? What about the good ol' solution to that? Detect undefined (or anything but string) there, and print a deprecation warning? Ie. when someone relies on that bug, then that person is free to stick to the old version.

[bnoordhuis]

Can I suggest you read through some of the issues that were filed after we started printing a deprecation warning for one thing or another? Better top up your coffee mug because some of the threads are epic.

[jasnell]

I believe the disagreement will lie more around whether this qualifies as a bug or not. I do not consider it to be so. It is well documented behavior that has been part of Node.js for six years. There is quite a non-trivial amount of code existing in the ecosystem that depends on this behavior that would be broken for no great reason. Historically, we have tried to err on the side of not breaking the ecosystem unless necessary to do so.

[skrat]

Would anyone be able to come up with any other than historical reasons argument? I'm not going to read any of those issue comments @bnoordhuis suggested, I'm not a maintainer. Historical / compatibility / public outrage argument aside: I agree on all the defaults you have there except for hostname, which I think should not have a default value and should be required. Last, very important point that you didn't yet address in your response is the default stack trace which says nothing to the user.

[jasnell]

There is certainly room for improvement in the type checking for hostname (which gives a variety of responses based on what hostname is. A PR is the best way to address that. Defaulting hostname to localhost/127.0.0.1 when it is undefined is not a change that I would agree with, however.

[sam-github]

net APIs default to localhost when an explicit host isn't provided. Very convenient, very useful, break-the-world if its changed. -:inf: on doing so.

For stack trace, its an unfortunate reality of async engines that the stack trace doesn't include the original call location, bugs us all, no credible way to fix this has been thought of yet. Sorry.

Or, perhaps you dislike that the message uses the IP address, instead of the hostname?

> net.connect(2, 'samtu.local', function() { console.log(err)}); null;
null
> Error: connect ECONNREFUSED 10.0.3.1:2
    at Object.exports._errnoException (util.js:1029:11)
    at exports._exceptionWithHostPort (util.js:1052:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1093:14)

I can see how knowing the pre-resolved name as well as the resolved IP would be useful.

[skrat]

Very convenient, very useful

Is the point I disagree on. It's very confusing IMO. It's as if you'd say that opening a file descriptor with undefined path defaults to /dev/null and call it very convenient and very useful.

[sam-github]

You sound like you got burnt by this, and spent some nasty time debugging. I'm sympathetic, but one person's frustration doesn't mean its confusing to everybody.

Be warned, that undefined triggering a default is common in node.js, your example above is actually pretty close to https://nodejs.org/api/child_process.html#child_process_options_stdio, wherein opening a child fd on undefined causes it to be opened on the parent's fd at the same offset. EDIT: for 0-2, the standard I/O ones

[skrat]

@sam-github thanks for the psychoanalysis, much appreciated.

[sam-github]

Well, if you didn't actually get burn by this, and it didn't cause any nastiness, but you just wanted to rant, I'm less sympathetic. Good day.