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 #1518

Closed
wants to merge 0 commits into from

Conversation

evanlucas
Copy link
Contributor

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

Repointing at master branch. See #880 for original PR

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Apr 24, 2015
@@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the ternary is here instead of just passing in enable? Is it the .bind perf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That was @bnoordhuis recommendation.

@silverwind
Copy link
Contributor

Regarding tests: I'm not sure if it's possibly to test TCP_NODELAY reliably on a localhost test. There's just no measureable latency difference in this situation. Are we fine with landing this with testing just for the call to setNoDelay happening?

@silverwind
Copy link
Contributor

I'll LGTM this one.

Given the fact that both setNoDelay and setKeepAlive functionality are hard if not impossible to test on a localhost scenario, I'm fine with verifying that the call happened.

@evanlucas
Copy link
Contributor Author

Ok. I'll land tomorrow provided @bnoordhuis is ok with it also

@evanlucas
Copy link
Contributor Author

var echoServer = net.createServer(function(connection) {
serverConnection = connection;
connection.setTimeout(0);
assert.notEqual(connection.setKeepAlive, undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe assert.equal(typeof connection.setKeepAlive, 'function')?

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Tests have been updated

});

process.on('exit', function() {
assert.ok(callCount);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use assert.equal here?

@bnoordhuis
Copy link
Member

@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
Copy link
Contributor Author

Alright, they pass linting now. Also added the requested changes


sock1.setNoDelay();
sock1.connect(common.PORT);
sock1.on('end', process.exit);
Copy link
Member

Choose a reason for hiding this comment

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

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.

@bnoordhuis
Copy link
Member

LGTM with a request.

evanlucas added a commit that referenced this pull request May 19, 2015
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 evanlucas closed this May 19, 2015
@evanlucas evanlucas deleted the net-persistent-opts branch May 19, 2015 18:24
@evanlucas
Copy link
Contributor Author

Landed in 85d9983. Thanks!

@evanlucas
Copy link
Contributor Author

Also, made the requested change before merging. Thanks

@silverwind
Copy link
Contributor

🎉

@rvagg rvagg mentioned this pull request May 23, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants