net: persist net.Socket options before connect #1518
Conversation
| @@ -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)); | |||
There was a problem hiding this comment.
Is there a reason why the ternary is here instead of just passing in enable? Is it the .bind perf?
There was a problem hiding this comment.
Yes. That was @bnoordhuis recommendation.
|
Regarding tests: I'm not sure if it's possibly to test |
|
I'll LGTM this one. Given the fact that both |
|
Ok. I'll land tomorrow provided @bnoordhuis is ok with it also |
evanlucas
force-pushed
the
net-persistent-opts
branch
from
102a993
to
6051035
Compare
| var echoServer = net.createServer(function(connection) { | ||
| serverConnection = connection; | ||
| connection.setTimeout(0); | ||
| assert.notEqual(connection.setKeepAlive, undefined); |
There was a problem hiding this comment.
Maybe assert.equal(typeof connection.setKeepAlive, 'function')?
evanlucas
force-pushed
the
net-persistent-opts
branch
from
6051035
to
a903fc1
Compare
| TCPWrap.prototype.unref = function() { | ||
| unref.call(this); | ||
| unrefed = true; | ||
| }; |
There was a problem hiding this comment.
Can be made more rigorous if you increment/decrement a refCount variable and check that it has the expected value inside the method call (0 for ref, 1 for unref.)
evanlucas
force-pushed
the
net-persistent-opts
branch
from
a903fc1
to
88734ae
Compare
|
Tests have been updated |
| }); | ||
|
|
||
| process.on('exit', function() { | ||
| assert.ok(callCount); |
There was a problem hiding this comment.
Can you use assert.equal here?
|
@evanlucas Would it be possible for you to check that the tests conform to the linting rules in #1721? That would help avoid cleanup commits. |
evanlucas
force-pushed
the
net-persistent-opts
branch
from
88734ae
to
dcb4756
Compare
|
Alright, they pass linting now. Also added the requested changes |
|
|
||
| sock1.setNoDelay(); | ||
| sock1.connect(common.PORT); | ||
| sock1.on('end', process.exit); |
There was a problem hiding this comment.
Sorry, didn't notice this before: can you explicitly close the open sockets here and the next test? Calling process.exit() in tests has made bugs go unnoticed the past so I'm kind of weary of it.
|
LGTM with a request. |
evanlucas
force-pushed
the
net-persistent-opts
branch
from
dcb4756
to
b65602f
Compare
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 Fixes: nodejs/node-v0.x-archive#7077 Fixes: nodejs/node-v0.x-archive#8572 PR-URL: #1518 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io>
evanlucas
force-pushed
the
net-persistent-opts
branch
from
b65602f
to
85d9983
Compare
|
Landed in 85d9983. Thanks! |
|
Also, made the requested change before merging. Thanks |
|
🎉 |
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 Fixes: nodejs/node-v0.x-archive#7077 Fixes: nodejs/node-v0.x-archive#8572 PR-URL: nodejs/node#1518 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io>
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:
Related: nodejs/node-v0.x-archive#7077 and
nodejs/node-v0.x-archive#8572
Repointing at master branch. See #880 for original PR