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?
There was a problem hiding this comment.
Thinking about this, we should probably keep emitting
HTTP/1.1andHTTP/2since some folks might be using regular expressions on these values. Can we have anifblock that determines the value based on theresp.versionvalue?
I am one who had tests that looked for the entire debug log line with the HTTP/1.1 and now they are breaking on version 2.2.2. Leaving the dot out for version 1.1 is invalid so it would be better for the long haul to bring that back.
There was a problem hiding this comment.
What are you seeing exactly? It was supposed to still be HTTP/1.1. Also, would you mind opening a new issue?
There was a problem hiding this comment.
I'm not specifying everything in my requirements file, but when 2.2.2 became available and was installed I see.
https://localhost:8082 "PUT /activeUser?force=true HTTP/11" 200 4
When I explicitly revert to 2.2.1 in the requirements file and rebuild the environment I see the previous behavior.
https://localhost:8082 "PUT /activeUser?force=true HTTP/1.1" 200 4
I can open a issue.
|
I don't think the requests failure is related but I am not sure yet. |
|
Confirmed the issue reproduces on main: https://github.com/urllib3/urllib3/actions/runs/9036580373/job/24833736784?pr=3391. Merging, thanks! |
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?