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

Send GOAWAY to server on Client Transport Shutdown #7015

Merged
merged 12 commits into from
Apr 17, 2024
Merged

Send GOAWAY to server on Client Transport Shutdown #7015

merged 12 commits into from
Apr 17, 2024

Conversation

aranjans
Copy link
Contributor

@aranjans aranjans commented Mar 4, 2024 •
edited by arvindbr8
Loading

Fixes #460

RELEASE NOTES:

  • client: client sends GOAWAY to server on graceful shutdown

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Mar 4, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@aranjans aranjans marked this pull request as draft March 4, 2024 18:37
@aranjans aranjans marked this pull request as ready for review March 4, 2024 18:37
@dfawley dfawley added this to the 1.63 Release milestone Mar 4, 2024
@ginayeh ginayeh requested a review from arvindbr8 March 4, 2024 23:22
@ginayeh
Copy link
Contributor

ginayeh commented Mar 5, 2024

@aranjans can you check test failures?

arvindbr8
arvindbr8 requested changes Mar 6, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

I've taken a first pass at the PR. Let me know if you have any questions

internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/transport_test.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 removed their assignment Mar 6, 2024
@arvindbr8
Copy link
Member

Also the tests are failing due to a panic on a nil pointer reference. Seems like its the goAwaySent event that was added.

@aranjans aranjans force-pushed the aranjans_460 branch 4 times, most recently from a9ab1a7 to 4d6f00f Compare March 7, 2024 05:58
@codecov Codecov
Copy link

codecov bot commented Mar 7, 2024 •
edited
Loading

Codecov Report

Merging #7015 (01b6d47) into master (adf976b) will increase coverage by 1.11%.
Report is 9 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 01b6d47 differs from pull request most recent head 01e6d7f. Consider uploading reports for the commit 01e6d7f to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7015      +/-   ##
==========================================
+ Coverage   81.24%   82.35%   +1.11%     
==========================================
  Files         345      305      -40     
  Lines       33941    31405    -2536     
==========================================
- Hits        27574    25865    -1709     
+ Misses       5202     4466     -736     
+ Partials     1165     1074      -91
Files Coverage Δ
internal/transport/controlbuf.go 88.32% <ø> (-0.17%) ⬇️
internal/transport/http2_client.go 91.21% <100.00%> (+0.08%) ⬆️
internal/transport/http2_server.go 90.28% <100.00%> (-0.29%) ⬇️

... and 62 files with indirect coverage changes

@arvindbr8 arvindbr8 self-requested a review March 7, 2024 16:42
@arvindbr8 arvindbr8 assigned arvindbr8 and unassigned aranjans Mar 7, 2024
@shashkin
Copy link
Contributor

shashkin commented Mar 8, 2024

@aranjans vet-proto test should be fixed after #7028 so you need to pull updated master

arvindbr8
arvindbr8 requested changes Mar 11, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

The changes to transport looks right to me. However I think we are missing something in the test. Could you take another look?

internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
test/goaway_test.go Outdated Show resolved Hide resolved
test/goaway_test.go Outdated Show resolved Hide resolved
arvindbr8
arvindbr8 requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

The changes to transport looks right to me. However I think we are missing something in the test. Could you take another look?

test/goaway_test.go Outdated Show resolved Hide resolved
test/goaway_test.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned aranjans and unassigned arvindbr8 Mar 12, 2024
@aranjans aranjans force-pushed the aranjans_460 branch 2 times, most recently from c8b2b7e to cc59944 Compare March 12, 2024 06:56
arvindbr8
arvindbr8 requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

I think we need to iterate over the test once again. Left a few comments.

test/goaway_test.go Outdated Show resolved Hide resolved
test/goaway_test.go Show resolved Hide resolved
test/goaway_test.go Outdated Show resolved Hide resolved
test/goaway_test.go Outdated Show resolved Hide resolved
test/goaway_test.go Outdated Show resolved Hide resolved
dfawley
dfawley approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM modulo the requested change to the last stream ID field in the GOAWAY.

Thanks and congrats on your first PR!

@dfawley dfawley assigned aranjans and unassigned dfawley Apr 17, 2024
@aranjans aranjans assigned dfawley and unassigned aranjans and dfawley Apr 17, 2024
@aranjans aranjans force-pushed the aranjans_460 branch 2 times, most recently from 3f6e180 to dffaae0 Compare April 17, 2024 17:10
arvindbr8
arvindbr8 approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM. Congrats on the first PR!

@arvindbr8 arvindbr8 merged commit f268126 into grpc:master Apr 17, 2024
12 checks passed
@fpetkovski fpetkovski mentioned this pull request Jun 12, 2024
Closed
@aranjans aranjans mentioned this pull request Jul 10, 2024
Merged
@aranjans aranjans mentioned this pull request Aug 20, 2024
Merged
@easwars easwars mentioned this pull request Oct 3, 2024
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

@arvindbr8 arvindbr8 arvindbr8 approved these changes

Assignees

@aranjans aranjans

Labels
Type: Bug
Projects
None yet
Milestone
1.64 Release
Development

Successfully merging this pull request may close these issues.

Send GOAWAY on client shutdown
6 participants
@aranjans @ginayeh @arvindbr8 @shashkin @dfawley @zasweq