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.connect: behaviour when no arguments are passed #33930

Closed
rexagod opened this issue Jun 17, 2020 · 2 comments
Closed

net.connect: behaviour when no arguments are passed #33930

rexagod opened this issue Jun 17, 2020 · 2 comments
Labels
discuss Issues opened for discussions and feedbacks. net Issues and PRs related to the net subsystem.

Comments

@rexagod
Copy link
Member

rexagod commented Jun 17, 2020

net.connect() - no args does ECONNREFUSED but should be invalid usage

I think I commented on this, and fixed it in backports, and it was thought
that net.connect() should be same as net.connect({port:undefined}), but I
don't agree, it never makes sense to connect when you don't say what you are
connecting to, it has no use case. also strange that undefined works like no
args were passed, but null is like {port: null} was passed. This all
seems ugly and messy. Check the tests.

For the quote above outlined in #33715 (comment), two solutions come to mind.

  • Throw ERR_MISSING_ARGS when no arguments are passed (as suggested above -- invalid usage).
  • To fix the difference in behavior between port: undefined and port: null, either make port a mandatory field that throws ERR_INVALID_ARGS for anything other than string or numeric values or don't throw ERR_INVALID_ARGS for port: null. This needs more brainstorming.

I'd appreciate a few pointers here to see where this should be going.

cc @sam-github

@lundibundi lundibundi added discuss Issues opened for discussions and feedbacks. net Issues and PRs related to the net subsystem. labels Jun 17, 2020
@lundibundi
Copy link
Member

IMO it should throw ERR_MISSING_ARGS if possible as none of those arguments are defined as optional in our docs and we should follow that and don't try to deduce what was meant by no-arg call. Furthermore, it makes it confusing when user upon calling net.connect() gets error that port value is not correct when the user didn't even provide that value in any way.

ping @nodejs/net

@mkrawczuk
Copy link
Contributor

mkrawczuk commented Jun 22, 2020

I'm currently looking into this. Tried both of the suggestions, but they do not seem to be a clean solution. In my opinion, the problem is that the function normalizeArgs is used both for listen and connect functions. While for listen it makes sense to bind to a random port, there is no use case for connecting to a random port.

Notice that the problem also relates to net.Socket.connect().

My conclusion is that sorting this out means a huge change to the Net module.

lundibundi added a commit to lundibundi/node that referenced this issue Jun 23, 2020
Previously Node.js would handle empty `net.connect()` and
`socket.connect()` call as if the user passed empty options object which
doesn't really make sense. This was due to the fact that it uses the
same `normalizeArgs` function as `.listen()` call where such call is
perfectly fine.

This will make it clear what is the problem with such call and how it
can be resolved. It now throws `ERR_MISSING_ARGS` if no arguments were
passed or neither `path` nor `port` is specified.

Fixes: nodejs#33930
@Trott Trott closed this as completed in b546a2b Jun 24, 2020
Trott pushed a commit that referenced this issue Jun 24, 2020
Previously Node.js would handle empty `net.connect()` and
`socket.connect()` call as if the user passed empty options object which
doesn't really make sense. This was due to the fact that it uses the
same `normalizeArgs` function as `.listen()` call where such call is
perfectly fine.

This will make it clear what is the problem with such call and how it
can be resolved. It now throws `ERR_MISSING_ARGS` if no arguments were
passed or neither `path` nor `port` is specified.

Fixes: #33930

PR-URL: #34022
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants