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: persist net.Socket options before connect #880

Closed
wants to merge 15 commits into from
Closed
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
Prev Previous commit
net: persist net.Socket options before connect
Remembers net.Socket options called before connect and retroactively
applies them after the handle has been created.

This change makes the following function calls more user-friendly:

- setKeepAlive()
- setNoDelay()
- ref()
- unref()

Related: nodejs/node-v0.x-archive#7077 and
nodejs/node-v0.x-archive#8572
evanlucas committed Apr 24, 2015
commit 102a993ade492bfd3a4365fd06371110abe78709
31 changes: 25 additions & 6 deletions lib/net.js
Original file line number Diff line number Diff line change
@@ -319,14 +319,25 @@ Socket.prototype._onTimeout = function() {


Socket.prototype.setNoDelay = function(enable) {
if (!this._handle) {
this.once('connect',
enable ? this.setNoDelay : this.setNoDelay.bind(this, enable));
return;
}

// backwards compatibility: assume true when `enable` is omitted
if (this._handle && this._handle.setNoDelay)
if (this._handle.setNoDelay)
this._handle.setNoDelay(enable === undefined ? true : !!enable);
};
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need new flags for this:

Socket.prototype.setNoDelay = function(enable) {
  if (this._connected === false) {
    this.once('connect', this.setNoDelay.bind(this, enable));
    return;
  }
  // ...
};

The use of Function#bind() is intentional, it avoids the unconditional closure allocation that would happen if you wrote the uncommon path as this.once('connect', function() { this.setNoDelay(enable); }).



Socket.prototype.setKeepAlive = function(setting, msecs) {
if (this._handle && this._handle.setKeepAlive)
if (!this._handle) {
this.once('connect', this.setKeepAlive.bind(this, setting, msecs));
return;
}

if (this._handle.setKeepAlive)
this._handle.setKeepAlive(setting, ~~(msecs / 1000));
};

@@ -968,14 +979,22 @@ function connectErrorNT(self, err, options) {


Socket.prototype.ref = function() {
if (this._handle)
this._handle.ref();
if (!this._handle) {
this.once('connect', this.ref);
return;
}

this._handle.ref();
};


Socket.prototype.unref = function() {
if (this._handle)
this._handle.unref();
if (!this._handle) {
this.once('connect', this.unref);
return;
}

this._handle.unref();
};


32 changes: 32 additions & 0 deletions test/parallel/test-net-persistent-keepalive.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
var common = require('../common');
var assert = require('assert');
var net = require('net');

var serverConnection;
var echoServer = net.createServer(function(connection) {
serverConnection = connection;
connection.setTimeout(0);
assert.notEqual(connection.setKeepAlive, undefined);
connection.on('end', function() {
connection.end();
});
});
echoServer.listen(common.PORT);

echoServer.on('listening', function() {
var clientConnection = new net.Socket();
// send a keepalive packet after 1000 ms
// and make sure it persists
clientConnection.setKeepAlive(true, 400);
clientConnection.connect(common.PORT);
clientConnection.setTimeout(0);

setTimeout(function() {
// make sure both connections are still open
assert.equal(serverConnection.readyState, 'open');
assert.equal(clientConnection.readyState, 'open');
serverConnection.end();
clientConnection.end();
echoServer.close();
}, 600);
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this test actually tests that TCP keep-alive works...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of it was taken directly from test/parallel/test-net-keepalive.js.

33 changes: 33 additions & 0 deletions test/parallel/test-net-persistent-nodelay.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
var common = require('../common');
var assert = require('assert');
var net = require('net');
var TCPWrap = process.binding('tcp_wrap').TCP;

var echoServer = net.createServer(function(connection) {
connection.end();
});
echoServer.listen(common.PORT);

var setTrue = 0;

var Socket = net.Socket;
var setNoDelay = TCPWrap.prototype.setNoDelay;

TCPWrap.prototype.setNoDelay = function(enable) {
setNoDelay.call(this, enable);
setTrue++;
};

echoServer.on('listening', function() {
var sock1 = new Socket();
// setNoDelay before the handle is created
// there is probably a better way to test this

sock1.setNoDelay();
sock1.connect(common.PORT);
sock1.on('end', process.exit);
});

process.on('exit', function() {
assert.ok(setTrue);
});
39 changes: 39 additions & 0 deletions test/parallel/test-net-persistent-ref-unref.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
var common = require('../common');
var assert = require('assert');
var net = require('net');
var TCPWrap = process.binding('tcp_wrap').TCP;

var echoServer = net.createServer(function(conn) {
conn.end();
});

var ref = TCPWrap.prototype.ref;
var unref = TCPWrap.prototype.unref;

var refed = false;
var unrefed = false;

TCPWrap.prototype.ref = function() {
ref.call(this);
refed = true;
};

TCPWrap.prototype.unref = function() {
unref.call(this);
unrefed = true;
};

echoServer.listen(common.PORT);

echoServer.on('listening', function() {
var sock = new net.Socket();
sock.unref();
sock.ref();
sock.connect(common.PORT);
sock.on('end', process.exit);
});

process.on('exit', function() {
assert.ok(refed);
assert.ok(unrefed);
});