Repository navigation
Support graceful connection close #1181
Description
Activity
I'm not familiar enough with the code to know the answer to this, but I'm happy to review a patch if you research it.
Code in question: https://github.com/python-hyper/hyper-h2/blob/570dc7daa480d34bcf0676e88d241112c51b1796/h2/connection.py#L1825-L1844
TL;DR as far as I understand the RFC:
- having received GOAWAY with error code
0:- we can't create more streams
- we are free to send and receive on the existing streams (up to GOAWAY's last stream id)
- higher level library is free to retry requests with higher stream ids (peer either never saw them, or never parsed them)
- having receive GOAWAY with non-zero error:
- we can't create new streams
- we can't send more data on existing streams
- we should probably keep the data already received for our user
- we should treat this as ConnectionError and [dunno about response] and close the connection
Cross-link: https://github.com/encode/httpx/issues/828
- having received GOAWAY with error code
Would it help to split the input two ways:
RECV_GOAWAYfor errorserror_code != 0RECV_GRACEFUL_SHUTDOWNorRECV_LAST_STREAM_IDforerror_code == 0?
Reacted by Yan Kevych and Jameel Al-Aziz+1 on this issue
Since such software like HAProxy terminates connections by sending GOAWAY first and HEADERS/DATA frames right after - the current transition states are unable to handle this.
Reacted by Dima Tisnek, Yan Kevych, Jameel Al-Aziz, Gen Xu, Jeff Hale and Lucas PedrozaAny updates on this?
Reacted by Robert Fink, Jeff Hale, Jack P and Mark Jan van KampenI've looked at fixing this and it seems like it'd be a lot of work. I'm very hesitant to dig into it without guidance from maintainers. It seems this is further complicated by two-stage GOAWAY handling but that does not appear to be well supported in general, for example https://mailman.nginx.org/pipermail/nginx-devel/2021-April/013956.html.
Coming at this from mitmproxy, where it turns up as mitmproxy/mitmproxy#7879. A fair number of sites, or really the load balancer in front of them, send a graceful GOAWAY and then finish the response they'd already started. h2 raises
Invalid input ConnectionInputs.RECV_DATA in state ConnectionState.CLOSEDat that point and the request dies.I read back through the thread and dimaqq's summary of the semantics matches my reading of RFC 9113, so I had a go at implementing it. I've got something working locally, suite passes, added tests for the draining cases. But I'm not confident I got the design right, and since the design looks like where this stalled before, I'd rather ask than open a PR that goes nowhere.
The part I'm unsure about is that I took a shortcut. Instead of adding a
RECV_GRACEFUL_SHUTDOWNinput and a draining state like dimaqq suggested, I left the connection inCLOSEDand recorded the boundary on the state machine, then let DATA/HEADERS/WINDOW_UPDATE/RST_STREAM through the KeyError path when the stream id is at or below it. My reasoning was that it wouldn't disturb anything downstream checkingstate is CLOSED.Then I actually went and looked, and that reasoning might be backwards. fetchling has
while connection.state_machine.state is not ConnectionState.CLOSEDaround its read loop, so with my version it stops reading on a graceful GOAWAY even though h2 would accept the frames. Those consumers might genuinely need a way to tell draining apart from dead, which argues for doing it with a real state instead. I don't know this codebase's history well enough to call it.Two smaller things I'd value an opinion on.
process_inputdoesn't take a stream id today and mine needs one to apply the boundary, so stream context leaks into what the docs describe as a deliberately coarse, high-level connection machine. And on two-stage GOAWAY, keeping the smallestlast_stream_idseen made it fall out for free, but zanieb's nginx link suggests it isn't widely supported, so I'm not sure it's worth designing for.So how would you want this done? Happy to put the work in either way, and happy to be told the shortcut is wrong.
One thing I noticed while testing, in case it's useful when you're deciding: #1324 drops every non-GOAWAY frame once the connection is CLOSED, which stops the exception but the in-flight body goes with it. Ran the mitmproxy repro against it and got a 200 with an empty body. That one targets #1199 though, so it isn't trying to cover this.
@Kriechi sorry to pull you into a seven year old issue nobody's touched since 2022. Mainly after a steer on direction before I write anything up.
Reacted by Apoorva Bhagwat
RFC 7540 section 6.8 seems to indicate that traffic can continue flowing in both directions after a GOAWAY frame, with the only restriction being that the recipient can't open new streams after receiving the GOAWAY:
But, h2's connection state machine immediately transitions to CLOSED upon sending or receiving a GOAWAY, effectively prohibiting any further traffic. Would you be interested in a patch to fix this, or is there some disconnect I'm missing between the wording of the RFC and actual implementations?