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

TLS connection establishment errors can cause the server to shutdown since tonic v0.12.2 #1989

Open
krispraws opened this issue Oct 10, 2024 · 3 comments

Comments

@krispraws
Copy link
Contributor

Bug Report

#1882 that was part of tonic v0.12.2 introduced a behavior change where errors during TLS connection establishment terminate the accept loop, and thus shutdown the server.

Version

v0.12.2

Platform

Linux 5.10.210-201.852.amzn2.aarch64

Crates

tonic

Description

We run a Rust GRPC server in production that uses TLS. Since upgrading to tonic v0.12.2, we started seeing the server shutdown randomly. After adding logging, we root caused it to various errors in establishing TLS connections.
We have been frequently updating our tonic versions as the TLS accept loop error handling has been refined:
#1885
#1938
#1948
#1972
Today we had an incident involving a currently unhandled error

2024-10-10T16:06:24.447565645+00:00 - DEBUG tonic::transport::server::incoming - accept loop error error=No route to host (os error 113)

Prior to #1882, any errors from tls.accept(stream).await? would be swallowed, as compared to the current behavior of breaking out of the accept loop unless the errors match a specific list. Based on the description of the PR in question, this behavioral change seems inadvertent, and has been addressed by adding errors piecemeal to handle_accept_error, as noted above.
We cannot think of a scenario where we'd want to shutdown the server because an individual incoming TLS connection failed to be established. As such, it seems like the correct behavior should be to separate TCP and TLS error handling and return to swallowing TLS errors.

@krispraws
Copy link
Contributor Author

@djc , @tottoto - We are working on a PR to address this and will submit it shortly.

@eric-seppanen
Copy link

Is this issue solved? If so, it would be lovely to get this fix in a patch release.

@krispraws
Copy link
Contributor Author

Is this issue solved? If so, it would be lovely to get this fix in a patch release.

@eric-seppanen , yes, the fix was merged. I'd also like to get a release.

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

No branches or pull requests

2 participants