-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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: Socket.prototype.connect should accept args like net.connect #11761 #11762
Conversation
6305ab4
to
3d2a8b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs test coverage
@sam-github just added test |
const args = new Array(arguments.length); | ||
for (var i = 0; i < arguments.length; i++) | ||
args[i] = arguments[i]; | ||
// TODO(joyeecheung): use destructuring when V8 is fast enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung Are you ok with this comment being added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is same code as original line number 62 to 68.
and while doing this, i also considered exports.createConnection
this function to just create new Socket
and call connect
by just forwarding arguments (for higher code coverage, if it is okay to move options.timeout
also within Socket.prototype.connect
.
e.g.
...
exports.connect = exports.createConnection = function(options) {
const socket = new Socket(options);
return Socket.prototype.connect.apply(socket, arguments);
};
...
Socket.prototype.connect = function() {
const args = new Array(arguments.length);
for (var i = 0; i < arguments.length; i++)
args[i] = arguments[i];
// TODO(joyeecheung): use destructuring when V8 is fast enough
const normalized = normalizeArgs(args);
const options = normalized[0];
const cb = normalized[1];
debug('Socket.connect', normalized);
if (options.timeout)
this.setTimeout(options.timeout);
if (this.write !== Socket.prototype.write)
this.write = Socket.prototype.write;
...
what do you think @joyeecheung @mscdex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind. i think this code cannot be merged easily since new Socket
requires normalized options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be OK with that if it's just copy-pasting.
The timeouts are added in #8101, I can't see any harm adding them to Socket.prototype.connect
, so +1 to reduce code duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. but anyway we need to normalizeArgs
on exports.createConnection
since arguments of new Socket
needs normalized arguments.
I will just add one more commit to move timeout from exports.createConnection
to Socket.prototype.connect
.
After that, maybe it should be good to merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vhain (hit the button too soon)..but yeah you can push it here(should be semver-patch) or add it in a separate PR(semver-minor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung oops.. was too fast. already added commit to this PR. do you want me to revert that commit only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vhain I don't think it needs to be reverted if no one objects to / spots a bug caused by moving it down though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung right. it needs to be different branch and PR. i'm on it.
it's done ccb807e
const args = new Array(arguments.length); | ||
for (var i = 0; i < arguments.length; i++) | ||
args[i] = arguments[i]; | ||
// TODO(joyeecheung): use destructuring when V8 is fast enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be OK with that if it's just copy-pasting.
The timeouts are added in #8101, I can't see any harm adding them to Socket.prototype.connect
, so +1 to reduce code duplicate.
New CI after revert: https://ci.nodejs.org/job/node-test-pull-request/6761/ |
FWIW, to make this pull request easier to find via google/github search - here is the stack trace we observed in LoopBack tests when calling
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit.
const server = net.createServer((conn) => { | ||
conn.end(); | ||
server.close(); | ||
}).listen(0, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback here needs to be wrapped in common.mustCall()
.
}).listen(0, () => { | ||
const client = new net.Socket(); | ||
|
||
client.on('connect', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback here needs to be wrapped in common.mustCall()
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am on it. but then test/parallel/test-net-socket-connecting.js
also need to be changed in future?
anyway for this PR, i will only wrap test-net-socket-connect-without-cb.js
callback with common.mustCall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evanlucas commit is added. Thanks for the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the missing comment in the test file, LGTM. It also needs to be rebased and squashed.
'use strict'; | ||
const common = require('../common'); | ||
const net = require('net'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit <--after squashing, commit message should be reworked to follow guidelines (it shouldn't have the PR number in the subject, and doesn't need PR-URL in the description, but it should have a |
7e8a552
to
48924fc
Compare
@sam-github comment added to test to match guideline, as well as squashing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@nodejs/ctc this fixes a breaking bug in recent 7.x, it would be good to get a fix out |
@nodejs/release ^^^ |
One of us will be able to cut one next week. We try to release on Tuesday or Wednesday generally. |
@evanlucas The callbacks are wrapped in |
Arguments of Socket.prototype.connect should be also normalized, causing error when called without callback. Changed Socket.prototype.connect's code same as net.connect and added test. Fixes: #11761 PR-URL: #11762 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
ARM and FreeBSD failures are false positives. Landed in c6cbbf9, again, sorry for the inconvenience and thanks for the fix! |
Arguments of Socket.prototype.connect should be also normalized, causing error when called without callback. Changed Socket.prototype.connect's code same as net.connect and added test. Fixes: nodejs#11761 PR-URL: nodejs#11762 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
Notable changes: * module: fix loading from global folders on Windows (Richard Lau) [nodejs#9283](nodejs#9283) * net: allow missing callback for Socket.connect (Juwan Yoo) [nodejs#11762](nodejs#11762) PR-URL: nodejs#11831
Notable changes: * module: The [module loading global fallback] (https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders) to the Node executable's directory now works correctly on Windows. (Richard Lau) [nodejs#9283](nodejs#9283) * net: `Socket.prototype.connect` now once again functions without a callback. (Juwan Yoo) [nodejs#11762](nodejs#11762) * url: `URL.prototype.origin` now properly specified an opaque return of `'null'` for `file://` URLs. (Brian White) [nodejs#11691](nodejs#11691) PR-URL: nodejs#11831
Notable changes: * module: The [module loading global fallback] (https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders) to the Node executable's directory now works correctly on Windows. (Richard Lau) [nodejs#9283](nodejs#9283) * net: `Socket.prototype.connect` now once again functions without a callback. (Juwan Yoo) [nodejs#11762](nodejs#11762) * url: `URL.prototype.origin` now properly specified an opaque return of `'null'` for `file://` URLs. (Brian White) [nodejs#11691](nodejs#11691) PR-URL: nodejs#11831
Notable changes: * module: The [module loading global fallback] (https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders) to the Node executable's directory now works correctly on Windows. (Richard Lau) [#9283](nodejs/node#9283) * net: `Socket.prototype.connect` now once again functions without a callback. (Juwan Yoo) [#11762](nodejs/node#11762) * url: `URL.prototype.origin` now properly specified an opaque return of `'null'` for `file://` URLs. (Brian White) [#11691](nodejs/node#11691) PR-URL: nodejs/node#11831 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Arguments of Socket.prototype.connect should be also normalized, causing error when called without callback. Changed Socket.prototype.connect's code same as net.connect and added test. Fixes: nodejs#11761 PR-URL: nodejs#11762 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
Notable changes: * module: The [module loading global fallback] (https://nodejs.org/dist/latest-v6.x/docs/api/modules.html#modules_loading_from_the_global_folders) to the Node executable's directory now works correctly on Windows. (Richard Lau) [nodejs#9283](nodejs#9283) * net: `Socket.prototype.connect` now once again functions without a callback. (Juwan Yoo) [nodejs#11762](nodejs#11762) * url: `URL.prototype.origin` now properly specified an opaque return of `'null'` for `file://` URLs. (Brian White) [nodejs#11691](nodejs#11691) PR-URL: nodejs#11831
should this be backported? |
Only if we decide to backport #11667, as this is a fix for that. |
#11667 depends on a 7.x semver major patch so this one should not be backported either. |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
net.Socket.prototype.connect
#11761