Skip to content

net: persist net.Socket options before connect - #1518

Closed
evanlucas wants to merge 0 commit into
nodejs:masterfrom
evanlucas:net-persistent-opts
Closed

net: persist net.Socket options before connect#1518
evanlucas wants to merge 0 commit into
nodejs:masterfrom
evanlucas:net-persistent-opts

Conversation

@evanlucas

Copy link
Copy Markdown
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

@mscdexmscdex added the net Issues and PRs related to the net subsystem. label Apr 24, 2015
Comment threadlib/net.js Outdated

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@evanlucas
evanlucasforce-pushed the net-persistent-opts branch from 102a993 to 6051035CompareMay 19, 2015 14:24
@evanlucas

Copy link
Copy Markdown
ContributorAuthor

Copy link
Copy Markdown
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')?

@evanlucas
evanlucasforce-pushed the net-persistent-opts branch from 6051035 to a903fc1CompareMay 19, 2015 16:01

Copy link
Copy Markdown
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
evanlucasforce-pushed the net-persistent-opts branch from a903fc1 to 88734aeCompareMay 19, 2015 16:04
@evanlucas

Copy link
Copy Markdown
ContributorAuthor

Tests have been updated

Copy link
Copy Markdown
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
Copy Markdown
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
evanlucasforce-pushed the net-persistent-opts branch from 88734ae to dcb4756CompareMay 19, 2015 17:52
@evanlucas

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
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
Copy Markdown
Member

LGTM with a request.

@evanlucas
evanlucasforce-pushed the net-persistent-opts branch from dcb4756 to b65602fCompareMay 19, 2015 18:17
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#8572Fixes: nodejs/node-v0.x-archive#7077Fixes: nodejs/node-v0.x-archive#8572
PR-URL: #1518
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
@evanlucas
evanlucasforce-pushed the net-persistent-opts branch from b65602f to 85d9983CompareMay 19, 2015 18:24
@evanlucas
evanlucas deleted the net-persistent-opts branch May 19, 2015 18:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

netIssues and PRs related to the net subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@evanlucas@silverwind@bnoordhuis@chrisdickinson@Fishrock123@brendanashworth@mscdex