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: introduce Socket#connecting property #6404

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions doc/api/net.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,12 @@ socket as reported by the operating system. Returns an object with
three properties, e.g.
`{ port: 12346, family: 'IPv4', address: '127.0.0.1' }`

### socket.connecting
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be below socket.bufferSize to keep them in alphabetical order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.


If `true` - [`socket.connect(options\[, connectListener\])`][] was called and
haven't yet finished. Will be set to `false` before emitting `connect` event
and/or calling [`socket.connect(options\[, connectListener\])`][]'s callback.

### socket.bufferSize

`net.Socket` has the property that `socket.write()` always works. This is to
Expand Down
2 changes: 1 addition & 1 deletion lib/_tls_legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ CryptoStream.prototype._done = function() {
// readyState is deprecated. Don't use it.
Object.defineProperty(CryptoStream.prototype, 'readyState', {
get: function() {
if (this._connecting) {
if (this.connecting) {
return 'opening';
} else if (this.readable && this.writable) {
return 'open';
Expand Down
10 changes: 5 additions & 5 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ function TLSSocket(socket, options) {

this._init(socket, wrap);

// Make sure to setup all required properties like: `_connecting` before
// Make sure to setup all required properties like: `connecting` before
// starting the flow of the data
this.readable = true;
this.writable = true;
Expand Down Expand Up @@ -466,9 +466,9 @@ TLSSocket.prototype._init = function(socket, wrap) {
this._parent = socket;

// To prevent assertion in afterConnect() and properly kick off readStart
this._connecting = socket._connecting || !socket._handle;
this.connecting = socket.connecting || !socket._handle;
socket.once('connect', function() {
self._connecting = false;
self.connecting = false;
self.emit('connect');
});
}
Expand All @@ -480,7 +480,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
});
} else {
assert(!socket);
this._connecting = true;
this.connecting = true;
}
};

Expand Down Expand Up @@ -581,7 +581,7 @@ TLSSocket.prototype._finishInit = function() {
};

TLSSocket.prototype._start = function() {
if (this._connecting) {
if (this.connecting) {
this.once('connect', function() {
this._start();
});
Expand Down
35 changes: 21 additions & 14 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ const BYTES_READ = Symbol('bytesRead');
function Socket(options) {
if (!(this instanceof Socket)) return new Socket(options);

this._connecting = false;
this.connecting = false;
this._hadError = false;
this._handle = null;
this._parent = null;
Expand Down Expand Up @@ -202,7 +202,7 @@ Socket.prototype._unrefTimer = function unrefTimer() {
// so that only the writable side will be cleaned up.
function onSocketFinish() {
// If still connecting - defer handling 'finish' until 'connect' will happen
if (this._connecting) {
if (this.connecting) {
debug('osF: not yet connected');
return this.once('connect', onSocketFinish);
}
Expand Down Expand Up @@ -367,9 +367,16 @@ Socket.prototype.address = function() {
};


Object.defineProperty(Socket.prototype, '_connecting', {
get: function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a deprecation message here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChALkeR Can you check how many people are using ._connecting in the wild?

Copy link
Member Author

Choose a reason for hiding this comment

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

@evanlucas it should be, will do it in a follow-up PR, because it won't be backported.

return this.connecting;
}
});


Object.defineProperty(Socket.prototype, 'readyState', {
get: function() {
if (this._connecting) {
if (this.connecting) {
return 'opening';
} else if (this.readable && this.writable) {
return 'open';
Expand Down Expand Up @@ -397,7 +404,7 @@ Object.defineProperty(Socket.prototype, 'bufferSize', {
Socket.prototype._read = function(n) {
debug('_read');

if (this._connecting || !this._handle) {
if (this.connecting || !this._handle) {
debug('_read wait for connection');
this.once('connect', () => this._read(n));
} else if (!this._handle.reading) {
Expand Down Expand Up @@ -430,7 +437,7 @@ function maybeDestroy(socket) {
if (!socket.readable &&
!socket.writable &&
!socket.destroyed &&
!socket._connecting &&
!socket.connecting &&
!socket._writableState.length) {
socket.destroy();
}
Expand Down Expand Up @@ -465,7 +472,7 @@ Socket.prototype._destroy = function(exception, cb) {
return;
}

this._connecting = false;
this.connecting = false;

this.readable = this.writable = false;

Expand Down Expand Up @@ -648,7 +655,7 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) {
// If we are still connecting, then buffer this for later.
// The Writable logic will buffer up any more writes while
// waiting for this one to be done.
if (this._connecting) {
if (this.connecting) {
this._pendingData = data;
this._pendingEncoding = encoding;
this.once('connect', function() {
Expand Down Expand Up @@ -803,7 +810,7 @@ function connect(self, address, port, addressType, localAddress, localPort) {
// TODO return promise from Socket.prototype.connect which
// wraps _connectReq.

assert.ok(self._connecting);
assert.ok(self.connecting);

var err;

Expand Down Expand Up @@ -913,7 +920,7 @@ Socket.prototype.connect = function(options, cb) {

this._unrefTimer();

this._connecting = true;
this.connecting = true;
this.writable = true;

if (pipe) {
Expand Down Expand Up @@ -952,7 +959,7 @@ function lookupAndConnect(self, options) {
var addressType = exports.isIP(host);
if (addressType) {
process.nextTick(function() {
if (self._connecting)
if (self.connecting)
connect(self, host, port, addressType, localAddress, localPort);
});
return;
Expand Down Expand Up @@ -980,7 +987,7 @@ function lookupAndConnect(self, options) {
// It's possible we were destroyed while looking this up.
// XXX it would be great if we could cancel the promise returned by
// the look up.
if (!self._connecting) return;
if (!self.connecting) return;

if (err) {
// net.createConnection() creates a net.Socket object and
Expand Down Expand Up @@ -1048,8 +1055,8 @@ function afterConnect(status, handle, req, readable, writable) {

debug('afterConnect');

assert.ok(self._connecting);
self._connecting = false;
assert.ok(self.connecting);
self.connecting = false;
self._sockname = null;

if (status == 0) {
Expand All @@ -1065,7 +1072,7 @@ function afterConnect(status, handle, req, readable, writable) {
self.read(0);

} else {
self._connecting = false;
self.connecting = false;
var details;
if (req.localAddress && req.localPort) {
details = req.localAddress + ':' + req.localPort;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-net-connect-buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ tcp.listen(common.PORT, function() {
connectHappened = true;
});

console.log('_connecting = ' + socket._connecting);
console.log('_connecting = ' + socket.connecting);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the text also be changed to 'connecting = '?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't really matter, IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the text might save someone a cycle or two if the test fails and they start poking around trying to figure out why... probably slightly more time than it took me to write this (almost none, but non-zero).

Copy link
Member Author

Choose a reason for hiding this comment

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

@rmg I disagree, but will change it. Ack.


assert.equal('opening', socket.readyState);

Expand Down
21 changes: 21 additions & 0 deletions test/parallel/test-net-socket-connecting.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const net = require('net');

const server = net.createServer((conn) => {
conn.end();
server.close();
}).listen(common.PORT, () => {
const client = net.connect(common.PORT, () => {
assert.equal(client.connecting, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

strictEqual would be better I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.


// Legacy getter
assert.equal(client._connecting, false);
client.end();
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this pass linter? I think semicolon is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't, sorry! I added test after doing lint, so it didn't show it.

assert.equal(client.connecting, true);

// Legacy getter
assert.equal(client._connecting, true);
});