x/net/http2: regression in Transport's reuse of connections

[bradfitz]

The fix to #59690 (https://go-review.googlesource.com/c/net/+/486156) broke our load balancer.

That change made any RoundTripper caller's cancelation ends up tainting the underlying HTTP/2 connection (setting its do-not-reuse bit), causing our load balancer to create new conns forever, rather than reuse existing totally fine ones.

Of note: our application only uses ClientConn.RoundTrip (not Transport.RoundTrip), so moving up the connection tainting up a level (to where it arguably belongs in Transport & its conn pool) would fix us, but still isn't the proper fix for others.

The original issue was about dead backend connections and Transport reusing them forever. It fixed it, but a bit too aggressively. We shouldn't mark good connections (in-use, recent traffic, able to reply to pings, etc) as do-not-reuse. This is already addressed if you enable pings on your backend connections, but that's not the default, probably because it's not network chatty to do by default for idle connections.

@neild and I discussed a handful of options but I'm not we've decided on any particular path(s). Some:

• move the tainting up a level (again, fixes us, but not everybody). but maybe do this first anyway, because it's not ClientConn's job to blindly mark itself as broken if the caller cancels its context after a nanosecond.
• do PING frames to the backend even when the periodic pinger is not enabled under certain circumstances: like when the health of the ClientConn is in question. (on errors, on a new request after N seconds have elapsed since last hearing from the backend?) but have at most one ping outstanding at a time per ClientConn.

[rhysh]

• do PING frames to the backend even when the periodic pinger is not enabled under certain circumstances: like when the health of the ClientConn is in question. (on errors, on a new request after N seconds have elapsed since last hearing from the backend?) but have at most one ping outstanding at a time per ClientConn.

An option like this sounds great. What's not to love?

One of the great strengths of the HTTP/2 protocol is that implementations can cancel a request without throwing away a TCP+TLS connection. But one of the great strengths of HTTP/1.x is that it minimizes the blast radius of a single bad connection, allowing it to harm only a single request. Reusing connections is important for application stability—otherwise when CPU starvation in a client causes it to perceive its outbound calls as timing out, an application that reacts by closing its TCP+TLS connections and then dialing new ones will make its problem worse by spending any remaining CPU time on new TLS handshakes.

I think Go's implementation can give the best of both, and through that reduce the operational risks associated with moving from HTTP/1 to HTTP/2.

Here's how I'd like to deploy HTTP/2:

• When the caller cancels a request (either via a timeout, or electively), send a PING frame on that connection and put the connection into quarantine (unavailable for new requests).
• If a connection sits in the quarantine pool for longer than the timeout we'd use for a fresh dial, close the connection.
• Otherwise, if we get back a response to our PING, return the connection to the regular pool. (Maybe there are other frames that could prove a bit sooner that the peer is still responsive.)

And:

• Allow the client to set a limit on the number of concurrent streams the transport will send on each connection. Setting to "1" gets the same level of sharing we'd see in HTTP/1.1.

Maybe those settings aren't the right choice for everyone, but today it's very hard to configure that behavior.

[btasker]

do PING frames to the backend even when the periodic pinger is not enabled under certain circumstances: like when the health of the ClientConn is in question. (on errors, on a new request after N seconds have elapsed since last hearing from the backend?) but have at most one ping outstanding at a time per ClientConn.

That sounds like a reasonable fix to me too.

The only thing I would note - I don't know if it's changed - but as recently as 2021, AWS Application Load Balancers lacked support for HTTP2 Ping. So a ping based mitigation won't work if writing into something fronted by an ALB.

My original PR had checked whether the failed stream was the only one for the connection - that (I think) would also have addressed this issue, but adding pings should be much more robust if there's upstream support.

[dmitshur]

@neild and I discussed a handful of options but I'm not we've decided on any particular path(s).

Just checking, have you also considered the option of rolling back CL 486156 here in x/net, reopening #59690, and trying to fix again? Or is this issue comparatively smaller than #59690 and better to fix forward?

@neild There are currently open backport requests of that issue to Go 1.20 and 1.19 (#60301 and #60662). Should the decision to backport be revisited given the new information in this report, or are they okay to proceed as is? Thanks.

[btasker]

The only thing I would note - I don't know if it's changed - but as recently as 2021, AWS Application Load Balancers lacked support for HTTP2 Ping. So a ping based mitigation won't work if writing into something fronted by an ALB.

I kicked together a quick Python script to test

import socket
import ssl
import h2.connection
import h2.events
import certifi
import sys


SERVER_NAME = sys.argv[1]
SERVER_PORT = 443

socket.setdefaulttimeout(15)
ctx = ssl.create_default_context(cafile=certifi.where())
ctx.set_alpn_protocols(['h2'])

s = socket.create_connection((SERVER_NAME, SERVER_PORT))
s = ctx.wrap_socket(s, server_hostname=SERVER_NAME)

c = h2.connection.H2Connection()
c.initiate_connection()
c.ping(b'ffffffff')
s.sendall(c.data_to_send())

response_stream_ended = False
while not response_stream_ended:
    data = s.recv(65536 * 1024)
    if not data:
            break
    events = c.receive_data(data)
    for event in events:
        if isinstance(event, h2.events.PingAckReceived):
            print(event)
            response_stream_ended = True
            break

I'm getting ACK's back from AWS ALB's, so it looks like they've added support.

A lack of upstream support would still be a potential issue, but it at least wouldn't be quite so widespread as if ALB's didn't support it.

[gopherbot]

Change https://go.dev/cl/507395 mentions this issue: http2: revert Transport change from CL 486156