net: remove unreachable error

[bewchy]

When creating a socket, if the file descriptor is not a valid PIPE or
TCP type, createHandle throws an ERR_INVALID_FD_TYPE. This means that
this._handle.open is guaranteed to succeed and will never return an
error.

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

[bewchy]

Amended the commit to remove trailing whitespaces on a line 270

[addaleax]

I hate to say this, but: This call is not guaranteed to succeed. There are a number of situations in which it doesn’t, but it might be hard to artificially create them in a cross-platform way?

However: There is a discrepancy here between .open() for pipes and for TCP instances. For pipes, we throw an exception from the C++ layer if an error occurs (so the line here is actually not reachable), but for TCP connections, we return the error code as a number.

I’m not sure which one I’d prefer, but it’s probably a better idea to align the two, and figure out how to cover it rather than to remove error checking.

[bewchy]

I couldn't figure out how to create a file descriptor that both passes the createHandle and fails this._handle.open

[addaleax]

@bewchy Yeah, I’m having a hard time too. But there’s a bunch of error conditions in the libuv code behind this, and some of them are very platform-specific (e.g. only macOS/only Windows)…

For example, I think this fails on Windows if fd is one of the stdio fds, referring to a pipe, and we’re currently at the handle limit, because we call DuplicateHandle() in that case. It’s a scenario that we can’t realistically test for, but silently failing would definitely not be okay.

[bewchy]

So how about we go to the code that this._handle.open calls and replace all error code returns with throwing errors? Then you can let the error propagate (the same way ERR_INVALID_FD_TYPE currently propagates from createHandle on the line above)

[Trott]

I'm putting this "Request Changes" here so that no one accidentally lands this in a crush of Code & Learn PRs until the whole thing being discussed is resolved. If it gets resolved, feel free to dismiss this review. No need to check with me on it!

[BridgeAR]

After discussing this with @addaleax I agree that the code should stay as it is. It is difficult to write a test for this but what we could do is adding a code comment what this actually tests and why it is hard to test.

[bewchy]

I'll update the PR to include some comments then

changes implemented

[Trott]

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

[Trott]

Landed in 9827858.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)