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 2 commits
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 @@ -400,6 +400,12 @@ The `connectListener` parameter will be added as a listener for the
As [`socket.connect(options\[, connectListener\])`][`socket.connect(options, connectListener)`],
with options either as either `{port: port, host: host}` or `{path: path}`.

### socket.connecting

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

Ensures that no more I/O activity happens on this socket. Only necessary in
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);

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);
});