net: Track state of setKeepAlive and prevent unnecessary system calls #31551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasnell
reviewed
Feb 3, 2020
rustyconover
force-pushed
the
fix-suppress-multiple-set-keepalive
branch
from
4b2477c to
96043fc
Compare
The state of .setKeepAlive() is now tracked and code will prevent repeated system calls to setsockopt() when the value has already been set to the desired value for the socket.
rustyconover
force-pushed
the
fix-suppress-multiple-set-keepalive
branch
from
96043fc to
f3fac17
Compare
ronag
reviewed
Mar 2, 2020
| if (msecs != null && typeof msecs !== 'number') | ||
| throw new ERR_INVALID_ARG_TYPE('msecs', 'number', msecs); | ||
|
|
||
| if (msecs === 0) { |
ronag
reviewed
Mar 2, 2020
| if (!this._handle) { | ||
| this.once('connect', () => this.setKeepAlive(setting, msecs)); | ||
| return this; | ||
| } | ||
|
|
||
| if (this._handle.setKeepAlive) | ||
| if (this._handle.setKeepAlive && | ||
| this[kSetKeepAlive] !== setting) { |
There was a problem hiding this comment.
Doesn't this need to include the msec setting as well?
BridgeAR
force-pushed
the
master
branch
2 times, most recently
from
8ae28ff to
2935f72
Compare
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
Let's leave this open until someone contributes a test or decides to merge without. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The state of .setKeepAlive() is now tracked and code will prevent repeated
system calls to setsockopt() when the value has already been set to the
desired value for the socket.
make -j4 test(UNIX), orvcbuild test(Windows) passesRationale
The reason for this change is that if keep alive is set for a HTTP Agent, there will
be repeated calls to
net.Socket.setKeepAlive()which in turn will cause many calls tosetsockopt() that are unnecessary since the socket has already been set to
send keep alive packets.
This is much like #31543