fix: Return error on non https uri instead of panic #838
Conversation
This file contains 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
LucioFranco
reviewed
Dec 8, 2021
| #[cfg(not(feature = "tls"))] | ||
| { | ||
| if is_https { | ||
| return Err("connecting to HTTPS without TLS enabled".to_owned().into()); |
There was a problem hiding this comment.
Should we make this an actual error type?
There was a problem hiding this comment.
Seemed unlikely to me that code would be handling this, but I'm happy to change it if you prefer it the other way.
There was a problem hiding this comment.
Yeah lets make it a concrete error or error variant?
djc
force-pushed
the
no-tls-error
branch
2 times, most recently
from
dcca865 to
18a4fef
Compare
djc
force-pushed
the
no-tls-error
branch
2 times, most recently
from
747fc87 to
f478ecb
Compare
LucioFranco
approved these changes
Feb 15, 2022
LucioFranco
changed the title
|
This never panicked for me? The |
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.
Motivation
In getting started with tonic, I ran into an issue where issuing a first request with the default features and no explicit TLS configuration set would just result in a H2
GoAwayframe without further context. Given that I was providing ahttpsURL to theEndpoint, it seems reasonable that tonic should be able to give me a more meaningful error in this case.Solution
If the URI scheme is
httpsand thetlsfeature is disabled, return an error from the connector. If the feature is enabled but there is no TLS config present, also return an error.