ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Better error code when attempting to contact HTTP/1 server over HTTP/2 #216

Closed DmitryKakurin closed 4 years ago

DmitryKakurin commented 4 years ago

When an attempt is made to access HTTP/1 server with gun using HTTP/2 protocol the server (correctly) returns HTTP/1.1 400 Bad Request. But when gun is unable to parse this response (expecting HTTP/2 reply), it converts it to {closed, The connection was lost."}. It would be very helpful to return a distinct error in this case to indicate protocols mismatch.

essen commented 4 years ago

Oh yeah good idea.

DmitryKakurin commented 4 years ago

Hi @essen, to clarify :-): do you plan to make the change, or do you prefer me to give it a try?

essen commented 4 years ago

You can try and experiment as I'm busy elsewhere at the moment.

Question is what should be done, should we switch back to gun_http when this happens? Or just parse and close the connection? Need to check the RFCs.

DmitryKakurin commented 4 years ago

I think closing connection is the only option, as the receiving HTTP1 server would not know where the original HTTP2 request ends and the next HTTP1 request starts.

DmitryKakurin commented 4 years ago

Now the error is: {connection_error, protocol_error, 'HTTP/1 response to HTTP/2 request'}

essen commented 4 years ago

Can you clarify on how Gun is used by the way? Do you set protocols => [gun_http2]? And is this over TLS? To be honest I don't think the solution is that simple depending on how you configure Gun and how the connection is performed.

From what I can guess you are doing the configuration I mentioned and connecting over TCP (so direct HTTP/2 over TCP, known as "prior knowledge") so the solution is appropriate BUT we would only want to do so in this particular case, so it's missing a check on the transport at the very least. I think we should only do this check for stream 1 as well.

DmitryKakurin commented 4 years ago

Yes, it's the "prior knowledge" scenario. I'll add the check, thanks.

essen commented 4 years ago

I have pushed 1bdc4fdb8f8634075944671ae00d14dadeba89df fixing the problem in master. A few comments:

I intend to apply the same changes to 1.3.x once you confirm that it works out for you.

essen commented 4 years ago

I've pushed the 1.3.x equivalent as well.

essen commented 4 years ago

We do a best effort detection of HTTP/1 responses now and give a different error message in both branches.

essen commented 4 years ago

Tagged 1.3.2 and should arrive to hex.pm soon-ish (the MachineGun author has the upload rights). Thanks!

DmitryKakurin commented 4 years ago

I can see it on hex.pm, thanks a lot for the quick fix!