ninenines / gun

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

Fail fast when request is queued while closing if reconnect is off #252

Closed zuiderkwast closed 3 years ago

zuiderkwast commented 3 years ago

If a request is created when a connection is in state 'closing', e.g. after receiving an HTTP/2 GOAWAY frame or an HTTP/1.1 Connection: close header, an error message is sent back to the caller immediately, if reconnect is off (i.e. if the option retry is set to 0).

This allows an application to retry the request on another connection without waiting for all streams on the current connection to complete.

essen commented 3 years ago

You need to do this for {headers...}, {connect...} and {ws_upgrade...} messages as well.

zuiderkwast commented 3 years ago

You need to do this for {headers...}, {connect...} and {ws_upgrade...} messages as well.

Done.

essen commented 3 years ago

I think this should be tested against HTTP/1.1 as well by sending a connection: close response that doesn't finish immediately.

zuiderkwast commented 3 years ago

I think this should be tested against HTTP/1.1 as well by sending a connection: close response that doesn't finish immediately.

Good idea. I'll do that. Implementation should already be in place, I hope.

essen commented 3 years ago

Shouldn't be too difficult via Gun's gun_test origin functions or via Cowboy setting the connection: close header manually. Whichever fits best.

zuiderkwast commented 3 years ago

I can add it in shutdown_SUITE:http1_response_connection_close_pipeline/1 perhaps.... Just need to add a delay between the server response headers and the response body. It's using the Cowboy option max_keepalive => 1 for this.

essen commented 3 years ago

No make a new test. Delays create flaky tests.

zuiderkwast commented 3 years ago

I've implemented this for HTTP/1.x as well now. Only a test case wasn't enough. I had to add some logic in gun_http where the headers are handled.

essen commented 3 years ago

Thanks, looking into merging this now. If you have further Gun stuff to submit or that I overlooked please send or ping the relevant ticket, I will be releasing 2.0.0-rc.1 next week.

zuiderkwast commented 3 years ago

Great. I don't have anything more for Gun right now, but I'll let you know if I come up with something.

essen commented 3 years ago

Merged, thanks!

filmor commented 3 years ago

This breaks for me with an endpoint that implements Server Sent Events but responds with a Connection: close header. I have no idea whether this is allowed by a spec, but it definitely exists in the wild. Since there is no way to override this behaviour, I had to revert this patch to be able to receive more than one message from the server.

essen commented 3 years ago

Please provide a reproducible test case in a separate ticket or PR.

filmor commented 3 years ago

See #271.