Fix delayed server shutdown if clients are connected to the Watch server-streaming RPC of the Health API #2573
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…althChecks for dependency reasons
….HealthChecks namespace
…ating "Watch" streams on server shutdown
|
|
Thanks for the PR. This is a good improvement but I don't like duplicating the implementation of the health checks service. I've made a new PR that adds the same capability but with a different implementaiton: #2582 |
|
Thank you, @JamesNK, for addressing the issue. I agree that duplicating the health check service wasn't the most elegant approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2572
Changes
Grpc.HealthCheck.HealthServiceImplclass to theGrpc.AspNetCore.HealthChecksnamespace. This was necessary because theHealthServiceImplnow requires theIHostApplicationLifetime, which is not available in theGrpc.HealthCheckpackage. I assumed you prefer not to modify references of that package. I did not change the original implementation to ensure backwards compatibility.ApplicationStoppingcancellation token in theGrpc.AspNetCore.HealthChecks.HealthServiceImplclass to gracefully terminateWatchstreams during server shutdown. TheApplicationStoppingtoken was passed to theWaitToReadAsyncmethod to allow running calls to finish when the server is stopping. In the catch block, aNotServingresponse is written to the response stream, though anRpcExceptioncould be thrown if preferred.Testing
The following screenshot shows that the server now stops within a second, even if clients are connected to the
WatchRPC at the time of shutdown. The server was stopped at 12:09:17 (via the terminal).