Add check for h2.connection.ConnectionState.CLOSED in AsyncHTTP2Connection.is_available

[zanieb]

Addresses encode/httpx#2112 and PrefectHQ/prefect#7442

The issue here is that the underlying h2 connection can reached a CLOSED state without the httpcore connection being marked as CLOSED. This causes subsequent requests to continue to reuse the closed connection but they will always fail with a protocol error. By adding a check to is_available, we'll avoid using this bad connection after the first request fails.

It seems likely that we could check the h2 connection state somewhere else as well i.e. before handling every request. I'd appreciate some guidance from other maintainers.

Ideally we'd avoid sending any events when there's a closed connection which would avoid the LocalProtocolError in the first place, but first let's avoid holding closed connections in the pool.

[zanieb]

You can see how this changes things in my reproduction at https://github.com/madkinsz/httpcore-h2-closed

Without this change, when the NGINX proxy is reset a httpcore.RemoteProtocolError error is raised and all subsequent requests fail with h2.exceptions.ProtocolError: Invalid input ConnectionInputs.SEND_HEADERS in state ConnectionState.CLOSED

With this change, the first error is raised but subsequent requests succeed.

[tomchristie]

Looks like a good start, thanks! 💛

Ideally we'd avoid sending any events when there's a closed connection which would avoid the LocalProtocolError in the first place, but first let's avoid holding closed connections in the pool.

Hrm, yes it's not immediately obvious what the most graceful approach is here but let's start at the start.

It might(?) be worth rounding out this PR a bit with a test case in:

• https://github.com/encode/httpcore/blob/master/tests/_sync/test_http2.py
• https://github.com/encode/httpcore/blob/master/tests/_async/test_http2.py

(I can help out if needed.)

A test there wouldn't really be sufficient because all we can demonstrate at that level is "if we send these frames, then the connection interface will behave like X", but it'd be something.

[zanieb]

Thanks for the response Tom! I'll take a swing at the test and ping you when it's added or if I run into any problems.

[zanieb]

We can add a test like

def test_http2_connection_closed_unexpectedly():
    origin = Origin(b"https", b"example.com", 443)
    stream = MockStream(
        [
            hyperframe.frame.SettingsFrame().serialize(),
        ]
    )
    with HTTP2Connection(origin=origin, stream=stream, keepalive_expiry=5.0) as conn:
        conn._h2_state.close_connection()

        with pytest.raises(ProtocolError):
            conn.request("GET", "https://example.com/")

        assert not conn.is_available()

If you want to add something that doesn't reach into the underlying h2_state I may need some guidance.

[tomchristie]

Maybe a test where the server sends one response and then sends a hyperframe.frame.GoAwayFrame?

[zanieb]

@tomchristie yep that works to reproduce the issue, added in 00f98c5

[tomchristie]

Looking good!

I verified locally that without the implementation change, the test fails with the connection stuck in an .is_available() == True state.

We can improve on this further by ensuring that requests after a GOAWAY frame get a ConnectionNotAvailable() exception instead of a RemoteProtocolError(), which will allow them to gracefully retry(*). Would you prefer to tackle that as part of this pull request, or should we accept this, and then make further incremental improvements?

_{* Ideally we'd also allow existing streams to complete after a GOAWAY frame, so long as they are stream_id <= GO_AWAY_FRAME.last_stream_id, but the python-hyper/h2 state machine doesn't support that.}

[zanieb]

We can improve on this further by ensuring that requests after a GOAWAY frame get a ConnectionNotAvailable() exception instead of a RemoteProtocolError(), which will allow them to gracefully retry(*). Would you prefer to tackle that as part of this pull request, or should we accept this, and then make further incremental improvements?

Great that makes sense to me! Let's merge this and I'll open a follow up that raises the ConnectionNotAvailable error — a graceful retry seems like the ideal behavior here.

Unfortunate that there's no traction in that h2 issue.