Skip to content

Fix delayed server shutdown if clients are connected to the Watch server-streaming RPC of the Health API#2573

Closed
freibenjamin wants to merge 6 commits into
grpc:masterfrom
freibenjamin:fix/health-check-shutdown
Closed

Fix delayed server shutdown if clients are connected to the Watch server-streaming RPC of the Health API#2573
freibenjamin wants to merge 6 commits into
grpc:masterfrom
freibenjamin:fix/health-check-shutdown

Conversation

@freibenjamin

Copy link
Copy Markdown

Fixes #2572

Changes

  • Copied the Grpc.HealthCheck.HealthServiceImpl class to the Grpc.AspNetCore.HealthChecks namespace. This was necessary because the HealthServiceImpl now requires the IHostApplicationLifetime, which is not available in the Grpc.HealthCheck package. I assumed you prefer not to modify references of that package. I did not change the original implementation to ensure backwards compatibility.
  • Used the ApplicationStopping cancellation token in the Grpc.AspNetCore.HealthChecks.HealthServiceImpl class to gracefully terminate Watch streams during server shutdown. The ApplicationStopping token was passed to the WaitToReadAsync method to allow running calls to finish when the server is stopping. In the catch block, a NotServing response is written to the response stream, though an RpcException could be thrown if preferred.
  • Minor changes to comments.

Testing

The following screenshot shows that the server now stops within a second, even if clients are connected to the Watch RPC at the time of shutdown. The server was stopped at 12:09:17 (via the terminal).
Screenshot 2024-11-15 120952

@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@JamesNK

JamesNK commented Dec 9, 2024

Copy link
Copy Markdown
Member

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

@JamesNK JamesNK closed this Dec 9, 2024
@freibenjamin

Copy link
Copy Markdown
Author

Thank you, @JamesNK, for addressing the issue. I agree that duplicating the health check service wasn't the most elegant approach.

@freibenjamin freibenjamin deleted the fix/health-check-shutdown branch December 9, 2024 09:22
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.

Running server-streaming calls on HealthCheck API delay server shutdown

2 participants