Skip to content
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.

Sign up for GitHub

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

Jump to bottom

http: reduce likelihood of race conditions on keep-alive timeout #54863

Closed
wants to merge 1 commit into from
Closed

http: reduce likelihood of race conditions on keep-alive timeout #54863

wants to merge 1 commit into from

Conversation

jazelly
Copy link
Contributor

@jazelly jazelly commented Sep 9, 2024 •
edited
Loading

This is the continued work from #52653 to fix many similar issues when request is near socket keepAliveTimeout threshold. Added 1 second internal timeout to account for network latency for both server and client side.

Fixes: #52649
Refs: #54293

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Sep 9, 2024
ShogunPanda
ShogunPanda requested changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor nit but I think it looks fine.

lib/_http_agent.js Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Sep 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (e1e312d) to head (5f6ab97).
Report is 45 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54863      +/-   ##
==========================================
- Coverage   87.90%   87.89%   -0.01%     
==========================================
  Files         651      651              
  Lines      183343   183387      +44     
  Branches    35710    35720      +10     
==========================================
+ Hits       161165   161189      +24     
- Misses      15466    15469       +3     
- Partials     6712     6729      +17
Files with missing lines Coverage Δ
lib/_http_agent.js 98.19% <100.00%> (+0.03%) ⬆️
lib/_http_server.js 96.65% <100.00%> (+0.01%) ⬆️

... and 50 files with indirect coverage changes

LiviaMedeiros
LiviaMedeiros approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM.

@ShogunPanda The "changes requested" here was a misclick, I would assume?

Pinging @zanettea as author of original patch.

Also, i'm not sure where does the Co-authored-by email come from. git log --pretty=format:"Committer: %ce | Author: %ae" 2c3d3832f573fc59baec28d3e6249b055ebfd2b6 shows different addresses (perhaps the author email is the most correct to use here?).

@jazelly
Copy link
Contributor Author

jazelly commented Sep 10, 2024 •
edited
Loading

@LiviaMedeiros I grabbed @zanettea's email from one of his repo, but that one is probably incorrect, as github doesn't link back to his account. I'll try these emails and see which one is corrrect.

Update: yep, the author email is correct. TIL

@jazelly @zanettea
http: reduce likelihood of race conditions on keep-alive timeout
5f6ab97 
Fixes: nodejs#52649
Refs: nodejs#54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
@nikwen
Copy link
Contributor

nikwen commented Sep 11, 2024

Thanks for picking up the work, @jazelly! Appreciate it! 🙌

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 12, 2024 •
edited by jakecastelli
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/62345/ 💚

jasnell pushed a commit that referenced this pull request Sep 13, 2024
@jazelly @zanettea @jasnell
http: reduce likelihood of race conditions on keep-alive timeout
05ad947 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
@jasnell
Copy link
Member

jasnell commented Sep 13, 2024

Landed in 05ad947

1 similar comment
@jasnell

This comment was marked as duplicate.

Sign in to view
@jasnell jasnell closed this Sep 13, 2024
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@jazelly @zanettea @RafaelGSS
http: reduce likelihood of race conditions on keep-alive timeout
241d047 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@jazelly @zanettea @RafaelGSS
http: reduce likelihood of race conditions on keep-alive timeout
7542cbd 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@jazelly @zanettea @RafaelGSS
http: reduce likelihood of race conditions on keep-alive timeout
7123bf7 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
targos pushed a commit that referenced this pull request Sep 22, 2024
@jazelly @zanettea @targos
http: reduce likelihood of race conditions on keep-alive timeout
db524d8 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
targos pushed a commit that referenced this pull request Sep 26, 2024
@jazelly @zanettea @targos
http: reduce likelihood of race conditions on keep-alive timeout
a2eff02 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
@targos targos mentioned this pull request Sep 30, 2024
Open
targos pushed a commit that referenced this pull request Oct 2, 2024
@jazelly @zanettea @targos
http: reduce likelihood of race conditions on keep-alive timeout
4990ac8 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
targos pushed a commit that referenced this pull request Oct 2, 2024
@jazelly @zanettea @targos
http: reduce likelihood of race conditions on keep-alive timeout
596940c 
Fixes: #52649
Refs: #54293

Co-authored-by: Arrigo Zanette <zanettea@gmail.com>
PR-URL: #54863
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig left review comments

@jasnell jasnell jasnell approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

@jakecastelli jakecastelli jakecastelli approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

@ShogunPanda ShogunPanda Awaiting requested review from ShogunPanda

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

http: keepalive race condition near timeout
9 participants
@jazelly @nodejs-github-bot @nikwen @jasnell @ShogunPanda @anonrig @Ethan-Arrowood @jakecastelli @LiviaMedeiros