net: Socket.prototype.connect should accept args like net.connect #11761 by vhain · Pull Request #11762 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

net.Socket.prototype.connect

needs test coverage

@joyeecheung Are you ok with this comment being added?

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

never mind. i think this code cannot be merged easily since new Socket requires normalized options.

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.

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?

@vhain I think it's already good to merge since it fixes #11761 without moving the timeout option handling.

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

@joyeecheung oops.. was too fast. already added commit to this PR. do you want me to revert that commit only?

@vhain I don't think it needs to be reverted if no one objects to / spots a bug caused by moving it down though.

@joyeecheung right. it needs to be different branch and PR. i'm on it.

it's done ccb807e

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.

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

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 net.createConnection(port) (see strongloop/loopback#3262)

Uncaught TypeError: "listener" argument must be a function
    at Socket.connect (net.js:943:10)
    at Socket.connect (node_modules/async-listener/index.js:76:27)
    at Object.exports.connect.exports.createConnection (net.js:76:35)
    at sendHttpRequestInOnePacket (test/integration.test.js:77:24)
    at Server.<anonymous> (test/integration.test.js:19:7)
    at emitListeningNT (net.js:1302:10)
    at node_modules/async-listener/glue.js:188:31
    at _combinedTickCallback (internal/process/next_tick.js:77:11)
    at process._tickDomainCallback [as _tickCallback] (internal/process/next_tick.js:128:9)

LGTM with a nit.

The callback here needs to be wrapped in common.mustCall().

The callback here needs to be wrapped in common.mustCall() as well

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

@evanlucas commit is added. Thanks for the comment.

other than the missing comment in the test file, LGTM. It also needs to be rebased and squashed.

https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-4-5

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 Fixes: https://github.com/nodejs/node/pull/11761 in the description)

@sam-github comment added to test to match guideline, as well as squashing.
Again, thank you for the comments.

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 common.mustCall now, LGTY?

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

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) [#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.

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 9, 2017

vhain force-pushed the net-socket-connect-cb-fix branch from 6305ab4 to 3d2a8b3 Compare March 9, 2017 03:54

sam-github suggested changes Mar 9, 2017

View reviewed changes

mscdex reviewed Mar 9, 2017

View reviewed changes

joyeecheung approved these changes Mar 9, 2017

View reviewed changes

joyeecheung mentioned this pull request Mar 9, 2017

Fix CI on Node 7.7.2 strongloop/loopback#3262

Merged

mcollina approved these changes Mar 9, 2017

View reviewed changes

cjihrig approved these changes Mar 9, 2017

View reviewed changes

evanlucas suggested changes Mar 9, 2017

View reviewed changes

sam-github suggested changes Mar 9, 2017

View reviewed changes

vhain force-pushed the net-socket-connect-cb-fix branch from 7e8a552 to 48924fc Compare March 9, 2017 19:39

mcollina approved these changes Mar 9, 2017

View reviewed changes

This was referenced Mar 10, 2017

TypeError: "listener" argument must be a function keymetrics/pm2-logrotate#48

Closed

TypeError: "listener" argument must be a function opbeat/opbeat-node#129

Closed

Gambit is crashing when New Relic is enabled DoSomethingArchive/gambit-content#811

Closed

evanlucas approved these changes Mar 13, 2017

View reviewed changes

joyeecheung closed this Mar 13, 2017

italoacasas mentioned this pull request Mar 13, 2017

7.7.3 Release Proposal #11831

Merged

BarryCarlyon mentioned this pull request Mar 14, 2017

Can't create a redis Client under Node v7.7+ redis/node-redis#1210

Closed

joyeecheung mentioned this pull request Mar 14, 2017

cb may be undefined as well #11848

Closed

2 tasks

bajtos mentioned this pull request Mar 15, 2017

Run the latest Node.js 7 version on Travis again strongloop/loopback#3285

Merged

lindehoff mentioned this pull request Mar 15, 2017

TypeError('"listener" argument must be a function'); mscdex/node-imap#599

Closed

MylesBorins added dont-land-on-v6.x and removed dont-land-on-v6.x labels Apr 17, 2017

joyeecheung mentioned this pull request May 9, 2017

net: refactor overloaded argument handling #11667

Closed

3 tasks

joyeecheung added dont-land-on-v6.x and removed lts-watch-v6.x labels May 10, 2017

net: Socket.prototype.connect should accept args like net.connect #11761 by vhain · Pull Request #11762 · nodejs/node · GitHub

Uh oh!

Uh oh!

net: Socket.prototype.connect should accept args like net.connect #11761 - #11762

net: Socket.prototype.connect should accept args like net.connect #11761#11762vhain wants to merge 1 commit intonodejs:masternodejs/node:masterfrom vhain:net-socket-connect-cb-fixvhain/node:net-socket-connect-cb-fixCopy head branch name to clipboard

Conversation

vhain commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

vhain commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhain Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhain Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 9, 2017

Uh oh!

joyeecheung commented Mar 9, 2017

Uh oh!

bajtos commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Mar 9, 2017

Uh oh!

vhain commented Mar 9, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Mar 11, 2017

Uh oh!

addaleax commented Mar 11, 2017

Uh oh!

net: Socket.prototype.connect should accept args like net.connect #11761#11762
vhain wants to merge 1 commit into
nodejs:masterfrom
vhain:net-socket-connect-cb-fix

vhain commented Mar 9, 2017 •
edited
Loading

vhain Mar 9, 2017 •
edited
Loading

joyeecheung Mar 9, 2017 •
edited
Loading

vhain Mar 9, 2017 •
edited
Loading

joyeecheung Mar 9, 2017 •
edited
Loading

bajtos commented Mar 9, 2017 •
edited
Loading

joyeecheung commented Mar 13, 2017 •
edited
Loading