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

workaround for graceful_exit / wait_closed #189

Closed
wants to merge 2 commits into from

Conversation

abe-winter
Copy link

@abe-winter abe-winter commented May 1, 2024

What changed

  • add new _single_stage kwarg in graceful_exit which runs first_ and second_stage in sequence, rather than waiting for a second signal
  • new test

Why

We use grpclib in a scenario where #107 is a problem. We currently workaround by SIGTERM-ing twice. I'm hoping to add this kwarg so that we can trigger more aggressive shutdown behavior.

@vmagamedov
Copy link
Owner

Thank you for your effort but graceful exit with immediate second stage doesn't look like graceful exit, it is just exit :)

I tried to implement graceful exit without GOAWAY frame in #190. Here the first stage is more aggressive, should exit predictably and as soon as possible. And the only way to reach the second stage should be some slow cancellation logic inside a method handler.

In this implementation when graceful exit begins server starts to reject incoming requests on existing connections using REFUSED_STREAM error code while connection is not completely drained.

Can you install grpclib from graceful-exit branch and check if it works for you?

@abe-winter
Copy link
Author

abe-winter commented May 20, 2024

Thank you very much for looking at this.

I tried your graceful-exit branch as well as a few other versions (current master, 0.4.6, 0.4.7).

It looks like our specific shutdown issue was fixed between 0.4.6 and 0.4.7.

Looked deeper: removing server.py L739 causes 0.4.7 to behave like 0.4.6

(removing await _server_closed_fut in Server.wait_closed)

I should have tested on the latest release before submitting a fix -- I'm sorry about that.

Do you want me to do any more investigation? Or should I close this because it only affects an old version.

@abe-winter
Copy link
Author

closing, guessing not needed

@abe-winter abe-winter closed this Jun 6, 2024
@abe-winter abe-winter deleted the less-graceful-exit branch June 6, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants