Open okeuday opened 1 year ago
Good catch, but tough one. The request is HTTP/1.1 so I think we want to keep that around (for example for logs). Only the response is HTTP/2. Perhaps we need a token 'HTTP/1.1 -> HTTP/2'
or something to properly reflect the HTTP/1.1 request HTTP/2 response.
As a workaround for the time being you can check the Connection/Upgrade/HTTP2-Settings header to identify that the upgrade was requested.
@essen I think 'HTTP/1.1 -> HTTP/2'
is an idea separate from the cowboy_req:version/1
value that is closer to a state machine transition, so something that could be returned from a function that could trace the protocol's state machine as it progresses in the function call path. The reason I say that, is because if you want 'HTTP/1.1 -> HTTP/2'
as a return value, you likely want a websocket related return value too and when you have both, you are really representing the protocol's state machine's execution path.
I interpret the cowboy_req:version/1
function as the request's current version and you could always have a function like cowboy_req:version_initial/1
(your naming may prefer cowboy_req:initial_version/1
) to return 'HTTP/1.1'
while cowboy_req:version/1
could return 'HTTP/2'
after the upgrade. My view of:
GET /cloudi/api/rpc/logging_status.erl HTTP/1.1
...
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
is that it is 'HTTP/1.1'
which really isn't sure it wants 'HTTP/2'
because it isn't started as a 'HTTP/2'
request, until the server says its ok to be a 'HTTP/2'
request based on its logic because it is able to support the newer protocol (a "'HTTP/1.1'
XOR 'HTTP/2'
request"). So, I still think it is better to have cowboy_req:version/1
return 'HTTP/2'
.
What you decide is fine. I am not going to work-around it (don't like extra parsing of the Connection/Upgrade/HTTP2-Settings header stuff) because no one has complained about the problem yet. I will just wait for what exists in the future, unless someone complains.
Well the request's version is HTTP/1.1, it is only the response that is HTTP/2. The request's version is HTTP/1.1, it's not changing afterwards. So either we have a special token, or a second field identifying the protocol used in the response. A separate field likely makes more sense. But I need to think about it for a while.
The problem doesn't occur in Websocket because we don't provide the Req after the Websocket upgrade, only before when we are still HTTP/1.1.
When using cowboy 2.10.0, a HTTP/1.1 connection that gets upgraded to HTTP/2 doesn't have its cowboy_req:version/1 data updated after the upgrade (i.e.,
'HTTP/1.1'
is returned instead of the expected'HTTP/2'
). Certain response headers are invalid for HTTP/2 use (based on curl use) likeX-Content-Type-Options
andX-XSS-Protection
, so it is best to be aware of the current protocol used for the HTTP response headers. The version could be changed to'HTTP/2'
incowboy_http:http2_upgrade/4
orcowboy_http2:init/12
.A way to replicate this problem is with https://github.com/CloudI/CloudI/commit/f279724bc862602ed8ca759f2e86c65b657fb81c using:
Using the curl argument
--http2-prior-knowledge
instead of--http2
doesn't cause this error, due to initiating the connection as HTTP/2 (no HTTP/1.1 upgrade occurs).