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.

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

cherry-pick: transport: add timeout for writing GOAWAY on http2Client.Close() #7371 #7540

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

aranjans
Copy link
Contributor

@aranjans aranjans commented Aug 20, 2024

fixes #7314

Problem
In gRPC 1.64, we released a #7015 "Send GOAWAY to server on Client Transport Shutdown", post which in case of network hang when we are not able to write GOAWAY frame from client, closing connections will take a very long time.

What does this PR fix
This PR fixes this issue by having a timeout for loopyWriter to write GOAWAY so that http2Client.Close() doesn't wait for too long.

RELEASE NOTES:

  • client: prevent hanging during ClientConn.Close() when the network is unreachable.

@aranjans aranjans added this to the 1.66 Release milestone Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.86%. Comparing base (63853fd) to head (55bd4bd).
Report is 1 commits behind head on v1.66.x.

Additional details and impacted files
@@             Coverage Diff             @@
##           v1.66.x    #7540      +/-   ##
===========================================
- Coverage    82.05%   81.86%   -0.20%     
===========================================
  Files          360      360              
  Lines        27533    27539       +6     
===========================================
- Hits         22593    22544      -49     
- Misses        3762     3798      +36     
- Partials      1178     1197      +19     
Files Coverage Δ
internal/transport/http2_client.go 91.90% <100.00%> (+0.04%) ⬆️

... and 18 files with indirect coverage changes

@arvindbr8 arvindbr8 changed the title goAway commit cherry-pick for the branch-cut cherry-pick: transport: add timeout for writing GOAWAY on http2Client.Close() #7371 Aug 20, 2024
@arvindbr8
Copy link
Member

The relnotes should talk more about the bug along with what the fix was. Users would be interested to know if they might be impacted by the bug.

@aranjans
Copy link
Contributor Author

@arvindbr8 updated the description

@arvindbr8
Copy link
Member

Also feel free to update the relnotes for the release itself. Now when i think about it, the relnotes in the description here does not get picked up

@arvindbr8 arvindbr8 merged commit 35e915e into grpc:v1.66.x Aug 20, 2024
13 checks passed
@dfawley
Copy link
Member

dfawley commented Aug 21, 2024

No user will understand this release note, either; it's written more like it's for a gRPC developer. The release notes should talk about the user-visible problem is being fixed. "client: prevent hanging during ClientConn.Close() when the network is unreachable" or something like that.

@arvindbr8
Copy link
Member

cc: @aranjans

@aranjans
Copy link
Contributor Author

Acknowledged, updated the release note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants