Do not close HTTP/2 connections while a graceful GOAWAY is being drained

[senid231]

Summary

When a server sends a graceful GOAWAY (error code NO_ERROR), it is telling us two things:
it will not accept new streams, and it is still processing the streams at or below
last_stream_id and will send their responses (RFC 9113 §6.8).

Today the client only honours the first half. GOAWAY closes the connection immediately, the
background reader stops reading, and every stream still waiting for a response is failed with:

EOFError: Connection closed with 15 active stream(s)!

Those streams are exactly the ones the server accepted and is about to answer. For a POST
this is unrecoverable: Protocol::HTTP::Request#retry! is false for non-idempotent methods,
so the request fails locally even though the server processed it and its side effects already
happened.

This is not an edge case. nginx sends a graceful GOAWAY on the keepalive_requests-th
request of every HTTP/2 connection (default 1000), on keepalive_time (default 1h), and on
every nginx -s reload. Any client that keeps a persistent HTTP/2 connection with requests in
flight loses a burst of them each time.

Together with socketry/protocol-http2#31 this makes the client drain such a connection: it is
retired from the pool immediately, but stays open until the streams the server accepted have
completed.

Reproduction

Reproduced against real nginx 1.22.1 (and 1.20.2) with async-http 0.101.0 +
protocol-http2 0.26.2. keepalive_requests is lowered to 40 so the event happens every 40
requests instead of every 1000; nothing else is unusual about the setup.

nginx.conf

events {}
error_log /dev/stderr info;
http {
  keepalive_requests 40;
  log_format repro '$time_iso8601 status=$status';
  access_log /dev/stdout repro;
  upstream backend { server 127.0.0.1:8081; }
  server {
    listen 127.0.0.1:8443 http2;
    location / {
      proxy_pass http://backend;
      proxy_read_timeout 20;
    }
  }
}

backend.rb — stands in for the application server: sleeps 300ms, answers 201, logs what it received and what it finished

require 'socket'
require 'json'

server = TCPServer.new('127.0.0.1', 8081)
$stdout.sync = true

loop do
  socket = server.accept
  Thread.new(socket) do |s|
    head = +''
    head << s.readpartial(4096) until head.include?("\r\n\r\n")
    headers, rest = head.split("\r\n\r\n", 2)
    length = headers[/^content-length:\s*(\d+)/i, 1].to_i
    body = rest.b
    body << s.read(length - body.bytesize) while body.bytesize < length
    seq = JSON.parse(body)['seq']

    puts "recv seq=#{seq}"
    sleep 0.3
    s.write "HTTP/1.1 201 Created\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"
    puts "done seq=#{seq}"
  ensure
    s.close rescue nil
  end
end

client.rb — one persistent HTTP/2 connection, 4 bursts of 25 concurrent POSTs

require 'async'
require 'async/http'
require 'json'

$stdout.sync = true

endpoint = Async::HTTP::Endpoint.parse('http://127.0.0.1:8443', protocol: Async::HTTP::Protocol::HTTP2)
client = Async::HTTP::Client.new(endpoint)

ok = []
failed = Hash.new{|h, k| h[k] = []}
seq = 0

Sync do |task|
  4.times do
    tasks = 25.times.map do
      seq += 1
      n = seq
      task.async do
        response = client.post('/', {'content-type' => 'application/json'}, [{seq: n}.to_json])
        response.read
        ok << n
      rescue => error
        failed["#{error.class}: #{error.message}"] << n
      end
    end
    tasks.each(&:wait)
    sleep 0.5
  end
ensure
  client.close
end

puts "ok=#{ok.size} failed=#{failed.values.sum(&:size)}"
failed.each{|message, seqs| puts "FAILED #{seqs.size}x #{message} seqs=#{seqs.sort.inspect}"}

docker run -d --name h2repro --network host -v "$PWD/nginx.conf:/etc/nginx/nginx.conf:ro" nginx:1.22.1
ruby backend.rb > backend.log 2>&1 &

bundle exec ruby client.rb
docker logs h2repro 2>&1 | grep -c 'status=499'   # requests the client hung up on
grep -c 'done seq' backend.log                    # requests the backend actually completed

Before

ok=80 failed=20
FAILED 15x EOFError: Connection closed with 15 active stream(s)! seqs=[26, 27, ..., 40]
FAILED  5x EOFError: Connection closed with  5 active stream(s)! seqs=[76, 77, ..., 80]
nginx  201: 80  499: 20  'client prematurely closed connection': 2
backend received: 100  finished: 100

The failed sequence numbers are exactly the streams accepted before the 40th request on each
connection, and backend finished: 100 shows every one of them was fully processed. nginx logs
them as 499 (client closed the connection) and, at info level,
client prematurely closed connection while processing HTTP/2 connection.

After (this PR + socketry/protocol-http2#31)

ok=100 failed=0
nginx  201: 100  499: 0  'client prematurely closed connection': 0
backend received: 100  finished: 100

What actually happens

1. nginx reaches keepalive_requests while opening stream N. It sets h2c->goaway, queues
   GOAWAY(last_stream_id = N, NO_ERROR), and still creates and processes stream N.
   Later HEADERS frames on that connection are skipped without RST_STREAM, per RFC 9113
   §6.8. It only starts its lingering close once its in-flight stream count drops to zero.
   That is a textbook graceful shutdown.
2. Protocol::HTTP2::Connection#receive_goaway calls close!, so closed? becomes true right
   away.
3. The background reader loop is while !self.closed?, so it stops reading, and its ensure
   calls Connection#close(nil). With streams still registered and no error given, that raises
   EOFError.new("Connection closed with #{@streams.size} active stream(s)!") into all of them
   and closes the socket.
4. The pool reaches the same outcome by a second path: each refused stream releases the
   connection, reusable? is false because the connection is closed, so the pool retires it —
   and retire calls close, killing whatever is still in flight.
5. EOFError is not retried for POST/PATCH, so the request surfaces as a failure.

The refusal half already works correctly: streams above last_stream_id are closed with
Protocol::HTTP::RefusedError and resent on a new connection. It is the accepted streams that
are lost.

The fix

Server side (protocol-http2, socketry/protocol-http2#31): a graceful GOAWAY no longer
closes the connection while it still has accepted streams; the connection closes when the last
one completes. That PR adds Connection#goaway_received?; the remaining lifecycle is expressed
by the concrete closed? state and registered streams.

This PR is the client half:

• Connection#reusable? / #viable? are false once a GOAWAY has been received, so the
  pool retires the connection and never hands it to another request while it drains.
• Client#call raises Protocol::HTTP::RefusedError if a request does reach a connection
  which is going away (it can be acquired from the pool just before the GOAWAY arrives).
  Nothing has been written at that point, so the request is safely retried on a new connection,
  including non-idempotent ones. This is checked before closed?, because a GOAWAY which
  finds no streams to drain closes the connection in the same step, and the pre-existing
  Protocol::HTTP2::Error raised for a closed connection is not retried.
• async-pool 0.12 ownership removes a non-reusable connection from availability immediately,
  but retains it while existing users remain. The final release retires and synchronously closes
  the connection. Connection#close therefore keeps its normal synchronous contract; graceful
  retention belongs to the pool rather than to a stateful close operation.
• Connection#close_if_drained! stops the background reader when the drain finishes in
  another task. The last drained stream can complete on the sending side — the response
  arrived first and the request body was still being written — and the reader is then parked in
  a blocking read where it would never notice the connection is closed, leaking the task and
  the socket.
• Connection#close no longer stops the reader task when it is the reader task. That
  path is now common — the last drained stream completes inside the reader, releases the
  connection, and the pool retires it — and Async::Task#stop on the current task raises
  Async::Cancel in the middle of close, so super never ran and the socket was left open.
  The reader unwinds on its own instead, since closed? is true by then.

Tests

test/async/http/protocol/http2/graceful_goaway.rb:

• an end-to-end test with a raw HTTP/2 server that accepts three requests, sends
  GOAWAY(last_stream_id = first), and only then answers the accepted one: all three requests
  succeed, the accepted one on the original connection and the other two retried on a new one;
• a connection which received GOAWAY is not reusable or viable and refuses new requests with
  RefusedError; a pool-level regression verifies that it is removed from availability after
  one release, retained while another user remains, and closed by the final release;
• a connection whose GOAWAY left nothing to drain still refuses new requests with
  RefusedError rather than failing them;
• a connection closes itself when the last drained stream completes on the sending side.

Notes

• This depends on async-pool 0.12 and protocol-http2 0.27, both of which have been released.
• The same mechanism applies to any server that shuts a connection down gracefully — nginx
  keepalive_requests / keepalive_time / reload, and Envoy or gRPC servers, which send an
  advisory GOAWAY(2^31-1) first and the real last_stream_id later.

Types of Changes

Bug fix

Contribution

• I added tests for my changes.
• I tested my changes locally.
• I agree to the Developer's Certificate of Origin 1.1.

[senid231]

if you need a proof that it works i can temporary add protocol-http2 from PR branch to gems.rb and verify that tests are green

gem "protocol-http2", git: "https://github.com/senid231/protocol-http2.git", branch: "graceful-goaway-drain"

[samuel-williams-shopify]

LGTM.

[senid231]

@samuel-williams-shopify thx for quick response

i tested it with nginx - fix works fine. when you plan to release it?

[ioquatix]

Probably in the next 24 hours I'm trying to fix one more issue but if it looks problematic, I'll give up and just release this fix.