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.

Sign up for GitHub

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

Jump to bottom

trigger-http: Log TLS startup errors instead of propagating #2230

Merged
merged 1 commit into from
Jan 25, 2024
Merged

trigger-http: Log TLS startup errors instead of propagating #2230

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions crates/trigger-http/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,10 @@ impl HttpTrigger {

loop {
let (stream, addr) = listener.accept().await?;
let stream = acceptor.accept(stream).await?;
Self::serve_connection(self_.clone(), stream, addr);
match acceptor.accept(stream).await {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If acceptor.accept is a bad state where it will always error this might just turn into an infinite loop no? Should we log and return the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that should be an issue with the TLS acceptor. Note that the TCP listener just above does still bail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know enough about the TLS acceptor to say whether this is fine to do or not. I can't really give too much more of opinion without knowing why we don't want to propagate errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The TLS acceptor handles the TLS handshake. It could fail because of an underlying TCP error but it can also fail because the handshake cannot be completed (for any number of reasons, some of which aren't under the server's control). Any error that is permanently fatal to the TCP listener should fail on the next loop iteration's call to listener.accept.

Ok(stream) => Self::serve_connection(self_.clone(), stream, addr),
Err(err) => tracing::error!(?err, "Failed to start TLS session"),
}
}
}
}
Expand Down
Loading