Skip to content

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

Merged
arvindbr8 merged 1 commit intogrpc:v1.66.xfrom
aranjans:goAway-cherry-picked
Aug 20, 2024
Merged

cherry-pick: transport: add timeout for writing GOAWAY on http2Client.Close() #7371#7540
arvindbr8 merged 1 commit intogrpc:v1.66.xfrom
aranjans:goAway-cherry-picked

Conversation

@aranjans
Copy link
Contributor

@aranjans aranjans commented Aug 20, 2024 •
edited
Loading

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
@codecov
Copy link

codecov bot commented Aug 20, 2024 •
edited
Loading

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
Contributor

arvindbr8 commented Aug 20, 2024

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

aranjans commented Aug 20, 2024

@arvindbr8 updated the description

@arvindbr8
Copy link
Contributor

arvindbr8 commented Aug 20, 2024

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
1 check 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
Contributor

arvindbr8 commented Aug 22, 2024

cc: @aranjans

@aranjans
Copy link
Contributor Author

aranjans commented Aug 23, 2024

Acknowledged, updated the release note.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@arvindbr8 arvindbr8 arvindbr8 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Type: Bug

Projects

None yet

Milestone

1.66 Release

Development

Successfully merging this pull request may close these issues.

3 participants

@aranjans @arvindbr8 @dfawley