-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
@@ -935,12 +958,16 @@ Socket.prototype.connect = function(options, cb) { | |||
Socket.prototype.ref = function() { | |||
if (this._handle) | |||
this._handle.ref(); | |||
else | |||
this._handleOptions.ref = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that ref
should actually be an int and we should ++
and --
it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scratch that. I thought that's what libuv does, but it's just a flag
I have a similar solution for |
@cjihrig I'd be happy to refactor it to work similar to your PR. Perhaps we should open nodejs/node-v0.x-archive#9115 here as well to make it complete for both client and server? |
@evanlucas sure, I'll open it over here. |
+1 I've run into this issue before when setting up a socket before calling |
db308b3
to
9fa5e6a
Compare
I went ahead and changed over to keeping the flags separate. |
if (this._handle) | ||
this._handle.ref(); | ||
}; | ||
|
||
|
||
Socket.prototype.unref = function() { | ||
this._unref = new Date(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using Date.now()
, not big deal just own opinion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably just be true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup true
would be okay, i guess @evanlucas just wanna record the time of last calling unref()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had originally used a date after looking at nodejs/node-v0.x-archive#9115, but I went ahead and changed it to just using a boolean
9fa5e6a
to
7eccc69
Compare
I added the fix for |
}; | ||
|
||
|
||
Socket.prototype.setKeepAlive = function(setting, msecs) { | ||
if (this._handle && this._handle.setKeepAlive) | ||
this._handle.setKeepAlive(setting, ~~(msecs / 1000)); | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I commented on this before, but maybe not. By using an if-else
, don't you take the chance of the _setKeepAlive
flag having a different value from what is actually being used by the handle (same for _setNoDelay
)? For example, if you call setKeepAlive()
after the handle is created, then _setKeepAlive
will always be null
. It's not a huge deal since it's internal, but it could be confusing during debugging.
What if you did:
this._setKeepAlive = [setting, ~~(msecs / 1000)];
if (this._handle && this._handle.setKeepAlive)
this._handle.setKeepAlive(setting, ~~(msecs / 1000));
7eccc69
to
ac3fbc1
Compare
Requested changes made :] |
@@ -121,6 +121,9 @@ function Socket(options) { | |||
this._hadError = false; | |||
this._handle = null; | |||
this._host = null; | |||
this._unref = false; | |||
this._setNoDelay = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this default to false
?
This also needs a test for |
if (this._handle && this._handle.setNoDelay) | ||
this._handle.setNoDelay(enable === undefined ? true : !!enable); | ||
this._handle.setNoDelay(this._setNoDelay); | ||
}; |
There was a problem hiding this comment.
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); })
.
ac3fbc1
to
d0df008
Compare
Ok, I went ahead and went the route @bnoordhuis suggested. It seems cleaner to me. Thoughts? Also added test for |
any objections to the changed implementation of this? |
if (this._handle) | ||
this._handle.ref(); | ||
if (!this._handle) { | ||
this.once('connect', this.ref.bind(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .bind(this)
is superfluous, just this.once('connect', this.ref)
will do.
Left some comments. I don't know about the tests, they really only verify that the method is called, not that the action associated with the method is actually carried out. |
Will get the comments addressed. As far as the tests go, I'm having trouble reliably testing the functionality. Will keep working on it now though |
b96e2a0
to
87212d4
Compare
Which functionality are you having issues testing? |
tbh, the reliably testing |
This commit: - fixes development branch (v1.x -> master) - updates stability index wording - use iojs binary instead of node PR-URL: nodejs#1466 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This reverts commit 06cfff9. Reverted because it introduced a regression where (because options were modified in the later functionality) options.host and options.port would be overridden with values provided in other, supported ways. PR-URL: nodejs#1467 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This commit adds a test to ensure all options are NOT modified after passing them to http.request. Specifically options.host and options.port are the most prominent that would previously error, but add the other options that have default values. options.host and options.port were overridden for the one-argument net.createConnection(options) call. PR-URL: nodejs#1467 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This brings in the '%PYTHON%' revert, and restores the correct NODE_MODULE_VERSION. PR-URL: nodejs#1482 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Update AUTHORS list using tools/update-authors.sh PR-URL: nodejs#1476 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#1503 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
as per nodejs#1502 PR-URL: nodejs#1507 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
tls.connect(options) with no options.host should accept a certificate with CN: 'localhost'. Fix Error: Hostname/IP doesn't match certificate's altnames: "Host: undefined. is not cert's CN: localhost" 'localhost' is not added directly to defaults because that is not always desired (for example, when using options.socket) PR-URL: nodejs#1493 Fixes: nodejs#1489 Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com> Reviewed-By: Roman Reiss <me@silverwind.io>
If `$NODE_PATH` contains trailing separators, `Module.globalPaths` will contains empty strings. When `Module` try to resolve a module's path, `path.resolve('', 'index.js')` will boil down to `$PWD/index.js`, which makes sub modules can access global modules and get unexpected result. PR-URL: nodejs#1488 Reviewed-By: Roman Reiss <me@silverwind.io>
Update the remaining markdown files to refer to the master branch. PR-URL: nodejs#1511 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
When buffer list less than 2, no need to calculate the length. The change's benchmark result is here: https://gist.github.com/JacksonTian/2c9e2bdec00018e010e6 It improve 15% ~ 25% speed when list only have one buffer, to other cases no effect. PR-URL: nodejs#1437 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
enable ? this.setNoDelay : this.setNoDelay.bind(this, enable)); | ||
return; | ||
} | ||
|
||
// backwards compatibility: assume true when `enable` is omitted | ||
if (this._handle && this._handle.setNoDelay) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this._handle
would always be truthy at this point, right? Is the check still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
Separates out the lookup logic for net.Socket. In the event the `host` property is an IP address, the lookup is skipped. PR-URL: nodejs#1505 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
Allows customization of the lookup function used when Socket.prototype.connect is called using a hostname. PR-URL: nodejs#1505 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
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
87212d4
to
102a993
Compare
I opened a new PR pointed at master. #1518 |
Remember 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:
I'm not thrilled with how
test/parallel/test-net-persistent-nodelay.js
is implemented, but I have been having trouble reliably testing the functionality of settingsetNoDelay()
prior to handle creation.Related: nodejs/node-v0.x-archive#7077 and nodejs/node-v0.x-archive#8572
R= @cjihrig @bnoordhuis