-
-
Notifications
You must be signed in to change notification settings - Fork 33k
http: unbreak keepAliveTimeoutBuffer #59784
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
Conversation
Review requested:
|
5fce091
to
804bbe7
Compare
Adds a guard.
804bbe7
to
0abd988
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59784 +/- ##
==========================================
+ Coverage 89.95% 89.96% +0.01%
==========================================
Files 667 667
Lines 196776 197212 +436
Branches 38409 38530 +121
==========================================
+ Hits 177006 177427 +421
- Misses 12198 12204 +6
- Partials 7572 7581 +9
🚀 New features to boost your workflow:
|
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/17531940843 |
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 guess converting keepAliveTimeout
and keepAliveTimeoutBuffer
into a throwing setter is seen too breaking.
Personally I would prefer to fail on set instead of silently ignoring garbage set by user.
const keepAliveTimeout = NumberIsFinite(server.keepAliveTimeout) && server.keepAliveTimeout >= 0 ? | ||
server.keepAliveTimeout : 0; | ||
const keepAliveTimeoutBuffer = NumberIsFinite(server.keepAliveTimeoutBuffer) && server.keepAliveTimeoutBuffer >= 0 ? | ||
server.keepAliveTimeoutBuffer : 1e3; |
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.
Maybe move the 1e3
into a shared const and use it at all places.
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.
Because I don't want to break. It was 1e3 pre keepAliveTimeoutBuffer
.
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 understand your comment. Why is using a shard constant instead of duplicating the literal breaking?
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 7d0f6be |
Adds a guard. PR-URL: #59784 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
If keepAliveTimeoutBuffer is set to undefined (for whatever reason) or if not set at all we will throw.
This adds a guard.