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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 9 additions & 15 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,24 +899,18 @@ function connect(self, address, port, addressType, localAddress, localPort) {
}


Socket.prototype.connect = function(options, cb) {
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
Copy link
Contributor

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?

Copy link
Contributor Author

@vhain vhain Mar 9, 2017 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Member

@joyeecheung joyeecheung Mar 9, 2017 •
edited
Loading

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

@vhain vhain Mar 9, 2017 •
edited
Loading

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 normalized = normalizeArgs(args);
const options = normalized[0];
const cb = normalized[1];

if (this.write !== Socket.prototype.write)
this.write = Socket.prototype.write;

if (options === null || typeof options !== 'object') {
// Old API:
// connect(port[, host][, cb])
// connect(path[, cb]);
const args = new Array(arguments.length);
for (var i = 0; i < arguments.length; i++)
args[i] = arguments[i];
const normalized = normalizeArgs(args);
const normalizedOptions = normalized[0];
const normalizedCb = normalized[1];
return Socket.prototype.connect.call(this,
normalizedOptions, normalizedCb);
}

if (this.destroyed) {
this._readableState.reading = false;
this._readableState.ended = false;
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-net-socket-connect-without-cb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';
const common = require('../common');

// This test ensures that socket.connect can be called without callback
// which is optional.

const net = require('net');

const server = net.createServer(common.mustCall(function(conn) {
conn.end();
server.close();
})).listen(0, common.mustCall(function() {
const client = new net.Socket();

client.on('connect', common.mustCall(function() {
client.end();
}));

client.connect(server.address());
}));