search

kazu-yamamoto / http2

HTTP/2.0 library including HPACK
BSD 3-Clause "New" or "Revised" License
86 stars 22 forks source link

GOAWAY shouldn't immediately close the connection #102

Closed akshaymankar closed 2 weeks ago

akshaymankar commented 11 months ago

From Section about GOAWAY in the spec:

The GOAWAY frame (type=0x07) is used to initiate shutdown of a connection or to signal serious error conditions. GOAWAY allows an endpoint to gracefully stop accepting new streams while still finishing processing of previously established streams. This enables administrative actions, like server maintenance.

There is an inherent race condition between an endpoint starting new streams and the remote peer sending a GOAWAY frame. To deal with this case, the GOAWAY contains the stream identifier of the last peer-initiated stream that was or might be processed on the sending endpoint in this connection. For instance, if the server sends a GOAWAY frame, the identified stream is the highest-numbered stream initiated by the client.

Here the Receiver just throws an exception when it receives a GOAWAY and so the response can never be recieved:

https://github.com/kazu-yamamoto/http2/blob/07af719d98da6edb30ef818a93b5fce15368329b/Network/HTTP2/H2/Receiver.hs#L335-L339

wilfreddenton commented 7 months ago

This also results in "ConnectionIsClosed" being printed to stdout constantly in my servant (warp, warp-tls) server setup.

kazu-yamamoto commented 6 months ago

Sorry. I did not notice this issue.

Is threadDelay before throwIO good enough?

kazu-yamamoto commented 6 months ago

@akshaymankar @wilfreddenton Gentle ping.

wilfreddenton commented 6 months ago

Hey so I solved my problem by adding a custom exception handler to my warp server that logs any exceptions bubbling up that far instead of printing them to stdout. So I don't have any strong recommendations in regards to how to proceed with this issue.

For me though, and this is without any contextual knowledge, it seems weird that there is a check for the existence of an error and, when it doesn't exist, this exception gets thrown. I'm assuming this is done because you want calling threads to be able to catch and handle the closing of a connection. Maybe exceptions aren't the right abstraction for this though?

akshaymankar commented 6 months ago

Yeah, I also think exception is not the right way to communicate graceful closing of the connection. The threadDelay is also a bit hopeful in terms of a response being served and consumed in a given amount of time.

kazu-yamamoto commented 6 months ago

The purpose of the exception is to break cocurrently_ of the async package.

kazu-yamamoto commented 5 months ago

I understand this issue deeply now. I will try to think how to fix this issue.

kazu-yamamoto commented 4 months ago

This should be fixed in the current main. When GOAWAY is received, Receiver waits until all workers are finished and sends CFinish to Sender.