net: warn on keep-alive delays truncated to zero - #65528
Open
zeexzeex wants to merge 1 commit into
Open
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
needs-ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65528 +/- ##
==========================================
- Coverage 90.14% 90.03% -0.11%
==========================================
Files 751 769 +18
Lines 253679 261335 +7656
Branches 47789 49625 +1836
==========================================
+ Hits 228671 235301 +6630
- Misses 16264 17051 +787
- Partials 8744 8983 +239
🚀 New features to boost your workflow:
|
The keep-alive delays are given in milliseconds but the underlying socket options are configured in whole seconds, so a positive value below 1000 ms rounds down to 0. That leaves the system default in place instead of applying the requested timing, and there is nothing to indicate that the value had no effect. Emit a KeepAliveWarning when a positive initialDelay or interval is truncated to zero, and document the behaviour. A value of 0 keeps its documented meaning of leaving the current setting unchanged and does not warn. Two existing tests passed values below 1000 ms that did not match what their comments described; they now use 1000 ms. Refs: nodejs#57712 Signed-off-by: Avocado <[email protected]>
zeexzeex
force-pushed
the
net-keepalive-warning
branch
from
September 4, 2026 01:25
1e4c167 to
cc29daa
Compare
Member
|
I don't think we should be landing a warning for this. I think we should be throwing an error. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
socket.setKeepAlive()takes its delays in milliseconds, but the underlyingsocket options are configured in whole seconds. A positive value below
1000rounds down to
0, which leaves the system default in place instead ofapplying the requested timing:
Nothing indicates that the value had no effect. There is no exception, no
warning, and the return value is the socket either way, so the caller has no
way to tell that keep-alive was not configured as asked.
Sub-second timings cannot be supported:
uv_tcp_keepalive()takes seconds andrejects a delay below 1. This makes the truncation visible instead.
Changes
KeepAliveWarningwhen a positiveinitialDelayorintervalistruncated to zero.
0keeps its documented meaning of leaving the currentsetting unchanged and does not warn, and nothing is reported when keep-alive
is being disabled.
net.md. The rounding itself wasalready described; what was missing was that a value below
1000ends up notbeing applied at all.
1000ms which did not matchwhat their comments described.
Refs: #57712