Fix HTTP version in debug log #3316
Conversation
pquentin
added
the
Skip Changelog
This trick was used in the hyper HTTP/2 client, and has two advantages: 1. it is impossible to use the connection without acquiring the lock 2. passing the connection to another object only requires one parameter
| self.scheme, | ||
| self.host, | ||
| self.port, | ||
| method, | ||
| url, | ||
| # HTTP version | ||
| http_version, | ||
| response.version, |
There was a problem hiding this comment.
Thoughts on using *divmod(response.version, 10)?
There was a problem hiding this comment.
It would give HTTP/2.0 which is less wrong but still weird? I think I would prefer storing the version string in the response? It can be done without any computation and only costs a few bytes
There was a problem hiding this comment.
Yeah, I do like HTTP/2 in the logs better too.
There was a problem hiding this comment.
Thinking about this, we should probably keep emitting HTTP/1.1 and HTTP/2 since some folks might be using regular expressions on these values. Can we have an if block that determines the value based on the resp.version value?
There was a problem hiding this comment.
There's a way to avoid ifs, what do you think of a1f18b0?
| @@ -482,6 +482,7 @@ def getresponse( # type: ignore[override] | |||
| headers=headers, | |||
| status=httplib_response.status, | |||
| version=httplib_response.version, | |||
| version_string=getattr(self, "_http_vsn_str", "HTTP/?"), | |||
There was a problem hiding this comment.
Emscripten has its own connection class, so I don't need the getattr any more:
| version_string=getattr(self, "_http_vsn_str", "HTTP/?"), | |
| version_string=self._http_vsn_str, |
There was a problem hiding this comment.
But I do need to add version_string to
Addresses #3297 (comment). It isn't great, but it will log HTTP/0 for emscripten, HTTP/20 for HTTP/2 and HTTP/11 for HTTP/1.1. Is it time to add an actual
http_versionorversion_stringinBaseHTTPResponsealongsideversion?