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

server: block GracefulStop on method handlers and make blocking optional for Stop #6922

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Jan 16, 2024

Fixes #6921

This reverts a behavior change in 1.60 for GracefulStop. Stop never blocked for the method handlers, so this would introduce a behavior change for Stop if we did it by default; instead we add a ServerOption to control the behavior.

RELEASE NOTES:

  • server: fix GracefulStop to block until all method handlers return (v1.60 regression); add WaitForHandlers ServerOption (experimental) to cause Stop to also block until method handlers return (new feature).

@dfawley dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Jan 16, 2024
@dfawley dfawley added this to the 1.61 Release milestone Jan 16, 2024
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Merging #6922 (c61a4d7) into master (953d12a) will decrease coverage by 0.12%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head c61a4d7 differs from pull request most recent head 8fc625b. Consider uploading reports for the commit 8fc625b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6922      +/-   ##
==========================================
- Coverage   83.72%   83.60%   -0.12%     
==========================================
  Files         287      287              
  Lines       30835    30839       +4     
==========================================
- Hits        25816    25783      -33     
- Misses       3964     3989      +25     
- Partials     1055     1067      +12     
Files Coverage Δ
server.go 81.43% <100.00%> (-0.03%) ⬇️

... and 20 files with indirect coverage changes

@dfawley dfawley added Type: Bug and removed Type: Behavior Change Behavior changes not categorized as bugs labels Jan 17, 2024
@ginayeh ginayeh requested a review from arvindbr8 January 17, 2024 18:11
@ginayeh ginayeh assigned ginayeh and arvindbr8 and unassigned ginayeh Jan 17, 2024
Copy link

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

LGTM, tested this out as well with my repro from issue #6921 and looks like GracefulStop works as expected and Stop works as expected when the new WaitForHandlers(true) option is passed to the server (but does not wait for handlers if that option is not passed).

@dfawley dfawley changed the title server: block Stop and GracefulStop until method handlers return server: block GracefulStop on method handlers and make blocking optional for Stop Jan 17, 2024
@dfawley
Copy link
Member Author

dfawley commented Jan 17, 2024

@sunjayBhatia great, thank you for confirming!

server.go Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Jan 18, 2024
@dfawley dfawley merged commit 61eab37 into grpc:master Jan 18, 2024
12 checks passed
@dfawley dfawley deleted the wg branch January 18, 2024 16:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming RPC handlers are left running after client connections closed and server is stopped
4 participants