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

net: remove unreachable error #24222

Closed
wants to merge 4 commits into from

Conversation

bewchy
Copy link
Contributor

@bewchy bewchy commented Nov 7, 2018

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

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Nov 7, 2018
@BridgeAR BridgeAR added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 7, 2018
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.
@bewchy bewchy force-pushed the error-on-socket-constructor branch from d259599 to d961de0 Compare November 7, 2018 11:53
@bewchy
Copy link
Contributor Author

bewchy commented Nov 7, 2018

Amended the commit to remove trailing whitespaces on a line 270

@addaleax
Copy link
Member

addaleax commented Nov 7, 2018

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
Copy link
Contributor Author

bewchy commented Nov 7, 2018

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

@addaleax
Copy link
Member

addaleax commented Nov 7, 2018

@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
Copy link
Contributor Author

bewchy commented Nov 7, 2018

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
Trott previously requested changes Nov 8, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Contributor Author

bewchy commented Nov 13, 2018

I'll update the PR to include some comments then

lib/net.js Outdated Show resolved Hide resolved
Co-Authored-By: bewchy <bewchabbacc@gmail.com>
@Trott Trott dismissed their stale review November 13, 2018 22:53

changes implemented

@Trott
Copy link
Member

Trott commented Nov 13, 2018

@Trott
Copy link
Member

Trott commented Nov 14, 2018

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/.)

@Trott Trott closed this Nov 14, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 14, 2018
PR-URL: nodejs#24222
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24222
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24222
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 15, 2018
PR-URL: #24222
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants