Automatically attempt HTTP/2 prior knowledge without flag when required - #4680
Conversation
| in a Buf module in the current directory, using an empty request message: | ||
|
|
||
| $ buf curl --schema . --protocol grpc --http2-prior-knowledge \ | ||
| $ buf curl --schema . --protocol grpc \ |
There was a problem hiding this comment.
Clearest demonstration of the win
| // | ||
| // Without this, the CLI's error interceptor would render this without details as: | ||
| // | ||
| // Failure: the server hosted at that remote is unavailable. |
There was a problem hiding this comment.
While this function is a bit hacky, it felt worth it to improve on this error message
| !strings.Contains(err.Error(), "frame header looked like an HTTP/1.1 header") { | ||
| return err | ||
| } | ||
| // Format with %v rather than %w on purpose: the CLI's error interceptor |
There was a problem hiding this comment.
This seemed reasonable to me, the alternative was going to be much more invasive
| // Without this, the CLI's error interceptor would render this without details as: | ||
| // | ||
| // Failure: the server hosted at that remote is unavailable. | ||
| func wrapPlainTextHTTP2Error(err error, f *flags, host string, isSecure bool) error { |
There was a problem hiding this comment.
should this function taken in f or a bool with the value of f.HTTP2PriorKnowledge?
There was a problem hiding this comment.
It already does doesn't it (read on 898)?
Oops got it makes sense
| // The stdlib does not expose a structured way of knowing the error is from a | ||
| // HTTP/1.1-like response so do a string match, meaning this function is | ||
| // best-effort across Go versions. | ||
| !strings.Contains(err.Error(), "frame header looked like an HTTP/1.1 header") { |
There was a problem hiding this comment.
I wonder if using a structured error for this case would be a good contribution to the go standard library/(or really, golang.org/x/net). There are two places in the standard library that would need to be changed and one new exported error type as a sentinel.
2 participants
Currently,
--http2-prior-knowledgemust always be set with http urls where HTTP/2 is required. The reality is it's generally well known if a request will require HTTP/2 or not, so this change detects those cases and implicitly enables the flag, for gRPC protocol, server reflection enabled, or bidi streaming.After this change, users should generally not need to set
--http2-prior-knowledgeanymore.Fixes #4629