net: make Socket.connect() consistently asynchronous #8180
Conversation
| @@ -868,26 +847,33 @@ Socket.prototype.connect = function(options, cb) { | |||
| } | |||
|
|
|||
| timers._unrefActive(this); | |||
|
|
|||
|
I like the refactor, but don't get what you trying to get with this approach? The |
|
@indutny sorry that the description of this PR is a bit unclear. This started as work for #8140 (answering your ping 😄). Along the way I realized that the argument checking was a little problematic, as it was possible to have an asynchronous I added the |
|
Well, just remove |
|
@indutny updated! |
|
@indutny ping |
| throw new TypeError('localPort should be a number: ' + localPort); | ||
|
|
||
| if (port <= 0 || port > 65535) | ||
| throw new RangeError('Port should be > 0 and < 65536'); |
There was a problem hiding this comment.
Might as well follow the pattern and include what the request port was
`Socket.prototype.connect()` establishes a connection synchronously in some cases and asynchronously in others. This commit makes it consistently asynchronous. This commit also removes hard coded IPv4 addresses, and throws on bad inputs immediately, instead of potentially after an asynchronous operation.
|
@tjfontaine updated based on your comments |
|
@tjfontaine Anything else holding this up? @cjihrig ping back on Friday if no other response. |
|
@trevnorris Friday ping |
| if (addressType === 6 || addressType === 4) { | ||
| port = port | 0; | ||
| if (port <= 0 || port > 65535) | ||
| throw new RangeError('Port should be > 0 and < 65536'); |
There was a problem hiding this comment.
Sorry I didn't notice this before. Why removal of the port range check?
|
@cjihrig Things LGTM. Go ahead and squash it, add the usual commit fields and land it. Thanks. :) |
Socket.prototype.connect() sometimes throws on bad inputs after an asynchronous operation. This commit makes the input validation synchronous. This commit also removes some hard coded IP addresses. PR-URL: #8180 Fixes: #8140 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
|
Landed in b636ba8 |
Socket.prototype.connect()follows a synchronous code path in some cases and an asynchronous one in others. This commit makes it consistently asynchronous. This commit also removes hard coded IPv4 addresses, and throws on bad inputs immediately, instead of potentially after an asynchronous operation. Closes #8140EDIT: I might have worded this slightly wrong. The connection is made asynchronously. I was referring to the fact that one current code path involved a DNS lookup, while the other two completed synchronously.