From e19c26500f728be23baf5dc16e1f9720d6177d68 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:59:44 +0200 Subject: [PATCH 01/31] test(h2client): failing tests for #85, #86, #88 and #89 Every test here fails on main (a89f02d) for the reason its issue states, except TestH2FlowControlCreditsPadding, a guard that passes there and must keep passing once the body count stops counting padding. A scripted HTTP/2 server (prior knowledge or the h2c upgrade) writes each response frame by frame, so the wire shapes are pinned exactly; net/http's own server, answering the same route over HTTP/1.1 and HTTP/2, is the reference for status and byte parity. - #85: a 4xx/5xx whose HEADERS do not end the stream (body, empty END_STREAM DATA, trailers, a 1xx before it, padded+priority HEADERS) must be recorded with its real status, as HTTP/1.1 records it, and so must every status outside the 7 of the HPACK static table, whether its literal names :status by index 8 or by 9-14 (x/net's encoder, and so net/http's server, uses 14) and whether its value is Huffman-coded (401, 502) or not (503); the Benchmarker must report an all-404-with-body route as errors over prior knowledge, h2c upgrade and -mix. - #86: body bytes must be every DATA frame's data, padding excluded, equal to what HTTP/1.1 reports; throughput_bps over 4 x 16 KiB frames must be 65536 per response. - #88: with every deadline more than 30 s ahead warped 1500x (5 min -> 200 ms) through the existing dialTimeoutFunc hook, a connection must arm none and must keep serving with no error and no second connection. - #89: a server that closes without GOAWAY, and one that ends the connection in the middle of a DATA frame, must be redialed; each in-flight stream is one error, requests still queued for a stream are not charged, and torn bodies are credited to nothing. A connection whose writes fail while its reads do not must be ended and redialed too, not left failing every request at once. A connection that dies while a body waits for flow-control window must be torn down, its stream answered exactly once. A GOAWAYed connection must be closed by the client, not leaked. - #88 + #89: a connection the server ends after 300 ms must be redialed, with no lifetime deadline armed on any connection. --- h2client_accounting_test.go | 1039 +++++++++++++++++++++++++++++++++++ 1 file changed, 1039 insertions(+) create mode 100644 h2client_accounting_test.go diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go new file mode 100644 index 0000000..5ba94f6 --- /dev/null +++ b/h2client_accounting_test.go @@ -0,0 +1,1039 @@ +package loadgen + +// Regression tests for what the HTTP/2 client records per response and how +// long its connections live: +// +// - #85: a 4xx/5xx response WITH a body was recorded as status 200 (a +// success); the status must come from the response HEADERS frame. +// - #86: throughput counted only the last DATA frame of each response. +// - #88: every connection carried an absolute 5-minute deadline from dial. +// - #89: a connection that died with a read error (no GOAWAY) was never +// redialed, and its workers hung or spun errors; a failed write left a +// sticky error that failed every later request on the dead connection. +// +// Most tests drive a scripted HTTP/2 server (rawH2Server) that writes every +// response frame by frame, so each wire shape is pinned exactly; the +// status and byte parity tests use net/http's own server, answering the same +// route over HTTP/1.1 and HTTP/2, as the reference. + +import ( + "bufio" + "bytes" + "context" + "encoding/binary" + "errors" + "io" + "net" + "net/http" + "net/textproto" + "strconv" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" + + "golang.org/x/net/http2" + "golang.org/x/net/http2/hpack" +) + +// --------------------------------------------------------------------------- +// Scripted HTTP/2 server + +// rawH2Action is what the scripted server does with a connection after a +// handler has answered (or not answered) a request stream. +type rawH2Action int + +const ( + rawKeep rawH2Action = iota // keep serving the connection + rawClose // close the TCP connection, no GOAWAY + rawGoAway // send GOAWAY(last stream = this one), keep the connection open +) + +// rawH2Handler answers one request stream by writing frames on c. +type rawH2Handler func(c *rawH2Conn, streamID uint32, path string) rawH2Action + +// rawH2Server speaks just enough HTTP/2, over prior knowledge or the RFC 7540 +// §3.2 h2c upgrade, for a handler to script each response frame by frame. +type rawH2Server struct { + ln net.Listener + handler rawH2Handler + killAfter time.Duration // > 0: the server closes each connection (no GOAWAY) this long after accepting it + closeAfterData int // > 0: the server closes a connection (no GOAWAY) once it has read this many DATA bytes on it + + accepted atomic.Int64 // connections accepted + killed atomic.Int64 // connections the server ended (rawClose or killAfter) + clientClosed atomic.Int64 // connections the client ended first + dataSent atomic.Int64 // DATA frame payload bytes written, padding included + windowCredit atomic.Int64 // connection WINDOW_UPDATE increments received, minus each handshake's own + answeredLate atomic.Int64 // requests answered on a connection other than the first one + + mu sync.Mutex + conns []net.Conn + wg sync.WaitGroup +} + +// rawH2Conn is one server-side connection, as a handler sees it. +type rawH2Conn struct { + srv *rawH2Server + nc net.Conn + fr *http2.Framer + enc *hpack.Encoder + hbuf bytes.Buffer + index int64 // accept order, from 1 + requests int // request streams seen on this connection, the current one included + held int // streams a handler started answering and left open + dataRecv int // DATA payload bytes read on this connection +} + +// rawH2Opts are the rawH2Server options of the same names. +type rawH2Opts struct { + killAfter time.Duration + closeAfterData int +} + +func startRawH2(t *testing.T, h rawH2Handler) *rawH2Server { + return startRawH2With(t, rawH2Opts{}, h) +} + +func startRawH2With(t *testing.T, opts rawH2Opts, h rawH2Handler) *rawH2Server { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + s := &rawH2Server{ln: ln, handler: h, killAfter: opts.killAfter, closeAfterData: opts.closeAfterData} + s.wg.Add(1) + go func() { + defer s.wg.Done() + for { + nc, err := ln.Accept() + if err != nil { + return + } + s.mu.Lock() + s.conns = append(s.conns, nc) + s.mu.Unlock() + s.wg.Add(1) + go s.serve(nc) + } + }() + t.Cleanup(func() { + _ = ln.Close() + s.mu.Lock() + for _, nc := range s.conns { + _ = nc.Close() + } + s.mu.Unlock() + s.wg.Wait() + }) + return s +} + +func (s *rawH2Server) url(path string) string { return "http://" + s.ln.Addr().String() + path } + +func (s *rawH2Server) hostPort() (string, string) { + h, p, _ := net.SplitHostPort(s.ln.Addr().String()) + return h, p +} + +func (s *rawH2Server) serve(nc net.Conn) { + defer s.wg.Done() + index := s.accepted.Add(1) + + // ended is set by whichever side ends the connection first, so a read + // error after the server closed it is not counted as the client's doing. + var ended atomic.Bool + serverEnd := func() { + if ended.CompareAndSwap(false, true) { + s.killed.Add(1) + _ = nc.Close() + } + } + if s.killAfter > 0 { + timer := time.AfterFunc(s.killAfter, serverEnd) + defer timer.Stop() + } + + br := bufio.NewReader(nc) + fr := http2.NewFramer(nc, br) + if !rawH2Handshake(nc, br, fr) { + _ = nc.Close() + return + } + c := &rawH2Conn{srv: s, nc: nc, fr: fr, index: index} + c.enc = hpack.NewEncoder(&c.hbuf) + c.enc.SetMaxDynamicTableSizeLimit(0) // the client advertises SETTINGS_HEADER_TABLE_SIZE=0 + dec := hpack.NewDecoder(4096, nil) + handshakeCredit := true + + for { + f, err := fr.ReadFrame() + if err != nil { + if ended.CompareAndSwap(false, true) { + s.clientClosed.Add(1) + } + _ = nc.Close() + return + } + switch f := f.(type) { + case *http2.SettingsFrame: + if !f.IsAck() { + _ = fr.WriteSettingsAck() + } + case *http2.PingFrame: + if !f.IsAck() { + _ = fr.WritePing(true, f.Data) + } + case *http2.WindowUpdateFrame: + if f.StreamID == 0 { + if handshakeCredit { + handshakeCredit = false // the client's handshake grant, not a credit for DATA we sent + continue + } + s.windowCredit.Add(int64(f.Increment)) + } + case *http2.DataFrame: + c.dataRecv += len(f.Data()) + if s.closeAfterData > 0 && c.dataRecv >= s.closeAfterData { + serverEnd() + return + } + case *http2.HeadersFrame: + c.requests++ + path := "" + if fields, err := dec.DecodeFull(f.HeaderBlockFragment()); err == nil { + for _, hf := range fields { + if hf.Name == ":path" { + path = hf.Value + } + } + } + switch s.handler(c, f.StreamID, path) { + case rawClose: + serverEnd() + return + case rawGoAway: + _ = fr.WriteGoAway(f.StreamID, http2.ErrCodeNo, nil) + } + } + } +} + +// rawH2Handshake reads the client connection preface, performing the h2c +// upgrade first when the connection starts with an HTTP/1.1 request, and +// sends the server's SETTINGS. +func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer) bool { + head, err := br.Peek(len(http2.ClientPreface)) + if err != nil { + return false + } + if string(head) != http2.ClientPreface { + tp := textproto.NewReader(br) + if _, err := tp.ReadLine(); err != nil { + return false + } + if _, err := tp.ReadMIMEHeader(); err != nil { + return false + } + if _, err := io.WriteString(nc, "HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: h2c\r\n\r\n"); err != nil { + return false + } + if err := fr.WriteSettings(); err != nil { + return false + } + if head, err = br.Peek(len(http2.ClientPreface)); err != nil || string(head) != http2.ClientPreface { + return false + } + _, err = br.Discard(len(http2.ClientPreface)) + return err == nil + } + if _, err := br.Discard(len(http2.ClientPreface)); err != nil { + return false + } + return fr.WriteSettings() == nil +} + +// headers writes one HEADERS frame carrying fields, given as name, value pairs. +func (c *rawH2Conn) headers(streamID uint32, endStream bool, fields ...string) { + c.headersFrame(http2.HeadersFrameParam{StreamID: streamID, EndStream: endStream, EndHeaders: true}, fields...) +} + +func (c *rawH2Conn) headersFrame(p http2.HeadersFrameParam, fields ...string) { + c.hbuf.Reset() + for i := 0; i+1 < len(fields); i += 2 { + _ = c.enc.WriteField(hpack.HeaderField{Name: fields[i], Value: fields[i+1]}) + } + p.BlockFragment = c.hbuf.Bytes() + _ = c.fr.WriteHeaders(p) +} + +// data writes one DATA frame with n body bytes and, when pad > 0, pad bytes +// of padding. +func (c *rawH2Conn) data(streamID uint32, endStream bool, n, pad int) { + var padding []byte + length := n + if pad > 0 { + padding = make([]byte, pad) + length += 1 + pad // the Pad Length field plus the padding + } + _ = c.fr.WriteDataPadded(streamID, endStream, make([]byte, n), padding) + c.srv.dataSent.Add(int64(length)) +} + +// tornData writes the 9-byte header of a DATA frame announcing declared bytes +// and only the first sent of them, so the connection ends mid-frame. +func (c *rawH2Conn) tornData(streamID uint32, declared, sent int) { + hdr := []byte{byte(declared >> 16), byte(declared >> 8), byte(declared), 0x0 /* DATA */, 0, 0, 0, 0, 0} + binary.BigEndian.PutUint32(hdr[5:], streamID) + _, _ = c.nc.Write(append(hdr, make([]byte, sent)...)) +} + +// ok answers a stream with 200 and the 2-byte body "OK", in one DATA frame. +func (c *rawH2Conn) ok(streamID uint32) { + c.headers(streamID, false, ":status", "200") + c.data(streamID, true, len(rawOKBody), 0) + if c.index > 1 { + c.srv.answeredLate.Add(1) + } +} + +const rawOKBody = "OK" + +func respondOK(c *rawH2Conn, streamID uint32, _ string) rawH2Action { + c.ok(streamID) + return rawKeep +} + +// --------------------------------------------------------------------------- +// Helpers + +// runBench runs a Benchmarker and fails the test if Run does not return. +func runBench(t *testing.T, cfg Config) *Result { + t.Helper() + if cfg.Method == "" { + cfg.Method = "GET" + } + b, err := New(cfg) + if err != nil { + t.Fatalf("New: %v", err) + } + type outcome struct { + res *Result + err error + } + done := make(chan outcome, 1) + go func() { + res, err := b.Run(context.Background()) + done <- outcome{res, err} + }() + select { + case o := <-done: + if o.err != nil { + t.Fatalf("Run: %v", o.err) + } + return o.res + case <-time.After(cfg.Warmup + cfg.Duration + 20*time.Second): + t.Fatal("Benchmarker.Run did not return: a worker is hung") + return nil + } +} + +// h2Once sends one request on a fresh prior-knowledge client for path. +func h2Once(t *testing.T, host, port, path string) (int, error) { + t.Helper() + cl, err := newH2Client(host, port, path, testH2Cfg("GET", nil, nil, 1, 10)) + if err != nil { + t.Fatalf("newH2Client(%s): %v", path, err) + } + defer cl.Close() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return cl.DoRequest(ctx, 0) +} + +// h1Once sends one request on a fresh HTTP/1.1 client for path. +func h1Once(t *testing.T, host, port, path string) (int, error) { + t.Helper() + cl, err := newH1Client(host, port, path, Config{ + Method: "GET", Workers: 1, Connections: 1, PoolSize: 1, + DialTimeout: 5 * time.Second, ReadBufferSize: 64 << 10, WriteBufferSize: 64 << 10, MaxResponseSize: -1, + }) + if err != nil { + t.Fatalf("newH1Client(%s): %v", path, err) + } + defer cl.Close() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return cl.DoRequest(ctx, 0) +} + +// h2Status is the status an HTTP/2 DoRequest error reports, 0 for a success. +func h2Status(err error) int { + var se *HTTP2StatusError + if errors.As(err, &se) { + return se.Status + } + if err != nil { + return -1 + } + return 0 +} + +// startReferenceServer serves mux over HTTP/1.1 and prior-knowledge HTTP/2 +// cleartext on one listener, using net/http's own HTTP/2 server. +func startReferenceServer(t *testing.T, mux *http.ServeMux) (host, port string) { + t.Helper() + var protos http.Protocols + protos.SetHTTP1(true) + protos.SetUnencryptedHTTP2(true) + srv := &http.Server{Handler: mux, Protocols: &protos, ReadHeaderTimeout: 5 * time.Second} + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + go func() { _ = srv.Serve(ln) }() + t.Cleanup(func() { _ = srv.Close() }) + host, port, _ = net.SplitHostPort(ln.Addr().String()) + return host, port +} + +// warpThreshold and warpFactor define the deadline warp: a deadline armed +// more than warpThreshold ahead is a connection-lifetime bound, not a +// handshake one (those are 10 s), and fires warpFactor times sooner, so the +// 5-minute bound of #88 fires after 200 ms. +const ( + warpThreshold = 30 * time.Second + warpFactor = 1500 +) + +// warpConn applies the deadline warp and counts the deadlines it warped. +type warpConn struct { + net.Conn + armed *atomic.Int64 +} + +func (c warpConn) warp(t time.Time) time.Time { + if t.IsZero() { + return t + } + d := time.Until(t) + if d <= warpThreshold { + return t + } + c.armed.Add(1) + return time.Now().Add(d / warpFactor) +} + +func (c warpConn) SetDeadline(t time.Time) error { return c.Conn.SetDeadline(c.warp(t)) } +func (c warpConn) SetReadDeadline(t time.Time) error { return c.Conn.SetReadDeadline(c.warp(t)) } +func (c warpConn) SetWriteDeadline(t time.Time) error { return c.Conn.SetWriteDeadline(c.warp(t)) } + +// warpLongDeadlines routes every loadgen dial through warpConn for the rest +// of the test and returns the count of warped (long) deadlines armed. +func warpLongDeadlines(t *testing.T) *atomic.Int64 { + t.Helper() + armed := new(atomic.Int64) + dial := dialTimeoutFunc + t.Cleanup(func() { dialTimeoutFunc = dial }) + dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { + c, err := dial(network, addr, timeout) + if err != nil { + return nil, err + } + return warpConn{Conn: c, armed: armed}, nil + } + return armed +} + +// --------------------------------------------------------------------------- +// #85: the status comes from the response HEADERS, whether or not a body follows + +// TestH2StatusMatchesH1 answers each route over HTTP/1.1 and HTTP/2 from the +// same net/http handler and requires the HTTP/2 client to classify every +// response as the HTTP/1.1 client does: status >= 400 is an error. +func TestH2StatusMatchesH1(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/ok", func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("OK")) }) + mux.HandleFunc("/unauthorized", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) }) + mux.HandleFunc("/bad-gateway", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, _ = w.Write([]byte("upstream failed\n")) + }) + mux.HandleFunc("/unavailable", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte("try later\n")) + }) + host, port := startReferenceServer(t, mux) + + for _, tc := range []struct { + path string + status int // 0: a success + shape string + }{ + {"/ok", 0, "200, body"}, + {"/missing", 404, "http.NotFound: 404, then a 19-byte body"}, + {"/unauthorized", 401, "401, no body; net/http Huffman-codes the status"}, + {"/bad-gateway", 502, "502, then a body; Huffman-coded status"}, + {"/unavailable", 503, "503, then a body; literal status"}, + } { + _, h1err := h1Once(t, host, port, tc.path) + _, h2err := h2Once(t, host, port, tc.path) + t.Logf("%-13s h1 err=%v | h2 status=%d err=%v", tc.path, h1err, h2Status(h2err), h2err) + if (h1err != nil) != (tc.status != 0) { + t.Fatalf("%s: the HTTP/1.1 control disagrees with the route: err=%v", tc.path, h1err) + } + if got := h2Status(h2err); got != tc.status { + t.Errorf("%s (%s): HTTP/2 recorded status %d (err=%v), HTTP/1.1 recorded %d (err=%v): the HTTP/2 client must take the status from the response HEADERS", + tc.path, tc.shape, got, h2err, tc.status, h1err) + } + } +} + +// TestH2StatusRecordedFromHeaders pins the frame layouts that carry a status +// on a HEADERS frame that does not end the stream. +func TestH2StatusRecordedFromHeaders(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, path string) rawH2Action { + switch path { + case "/404-body": + c.headers(sid, false, ":status", "404") + c.data(sid, true, 19, 0) + case "/500-body-empty-end": + c.headers(sid, false, ":status", "500") + c.data(sid, false, 30, 0) + c.data(sid, true, 0, 0) + case "/503-body-trailers": + c.headers(sid, false, ":status", "503") + c.data(sid, false, 30, 0) + c.headers(sid, true, "x-checksum", "abc") + case "/502-no-body": + c.headers(sid, true, ":status", "502") // Huffman-coded: 16 bits beat the 3 literal bytes + case "/401-body": + c.headers(sid, false, ":status", "401") // Huffman-coded + c.data(sid, true, 12, 0) + case "/103-then-404": + c.headers(sid, false, ":status", "103") // interim response, then the final one + c.headers(sid, false, ":status", "404") + c.data(sid, true, 19, 0) + case "/404-padded-priority": + c.headersFrame(http2.HeadersFrameParam{StreamID: sid, EndHeaders: true, PadLength: 7, + Priority: http2.PriorityParam{Weight: 200}}, ":status", "404") + c.data(sid, true, 19, 0) + case "/103-then-200": + c.headers(sid, false, ":status", "103") + c.headers(sid, false, ":status", "200") + c.data(sid, true, 19, 0) + default: + c.ok(sid) + } + return rawKeep + }) + host, port := srv.hostPort() + + for _, tc := range []struct { + path string + status int // 0: a success + }{ + {"/404-body", 404}, + {"/500-body-empty-end", 500}, + {"/503-body-trailers", 503}, + {"/502-no-body", 502}, + {"/401-body", 401}, + {"/103-then-404", 404}, + {"/404-padded-priority", 404}, + {"/103-then-200", 0}, + {"/ok", 0}, + } { + _, err := h2Once(t, host, port, tc.path) + t.Logf("%-21s status=%d err=%v", tc.path, h2Status(err), err) + if got := h2Status(err); got != tc.status { + t.Errorf("%s: recorded status %d (err=%v), want %d: the status must come from the final response HEADERS, whatever frames follow it", + tc.path, got, err, tc.status) + } + } +} + +// TestExtractStatusEveryEncoding decodes every status 100-599 in every way an +// HPACK encoder may send it when the dynamic table is off: a literal naming +// :status by any of the static indices 8-14 (x/net's encoder, and so +// net/http's server, uses 14), without indexing, never indexed or with +// incremental indexing, and with a plain or a Huffman-coded value (RFC 7541 +// §5.2; an encoder picks Huffman whenever it is shorter, as it is for 401, +// 502, 103 and every code with two digits from {0, 1, 2}). +func TestExtractStatusEveryEncoding(t *testing.T) { + var buf bytes.Buffer + enc := hpack.NewEncoder(&buf) + enc.SetMaxDynamicTableSizeLimit(0) // as the client advertises + missed, total := 0, 0 + check := func(desc string, block []byte, status int) { + total++ + if got := extractStatus(block); got != status { + missed++ + if missed <= 8 { + t.Errorf("extractStatus(%s, % x) = %d, want %d", desc, block, got, status) + } + } + } + for status := 100; status < 600; status++ { + value := strconv.Itoa(status) + buf.Reset() + _ = enc.WriteField(hpack.HeaderField{Name: ":status", Value: value}) + check("x/net hpack encoder", bytes.Clone(buf.Bytes()), status) + + huff := hpack.AppendHuffmanString(nil, value) + for idx := byte(8); idx <= 14; idx++ { + for _, first := range []byte{idx, 0x10 | idx, 0x40 | idx} { // without indexing, never indexed, incremental + check("literal "+value, append([]byte{first, byte(len(value))}, value...), status) + check("Huffman "+value, append([]byte{first, 0x80 | byte(len(huff))}, huff...), status) + } + } + } + if missed > 0 { + t.Errorf("%d of %d encodings of a status were not decoded", missed, total) + } +} + +// TestH2ErrorStatusWithBodyCountsAsError is #85 as the issue measured it: the +// Benchmarker on an all-404-with-body route must report errors and no +// requests, over prior knowledge, the h2c upgrade and a -mix of both. +func TestH2ErrorStatusWithBodyCountsAsError(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.headers(sid, false, ":status", "404") + c.data(sid, true, 19, 0) + return rawKeep + }) + opts := HTTP2Options{Connections: 1, MaxStreams: 4} + for _, tc := range []struct { + name string + cfg Config + }{ + {"prior-knowledge", Config{HTTP2: true}}, + {"h2c-upgrade", Config{H2CUpgrade: true}}, + {"mix", Config{Mix: &MixRatio{H2: 1, Upgrade: 1}}}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.cfg + cfg.URL = srv.url("/missing") + cfg.Duration = 150 * time.Millisecond + cfg.Workers = 4 + cfg.HTTP2Options = opts + res := runBench(t, cfg) + t.Logf("requests=%d errors=%d mix=%+v", res.Requests, res.Errors, res.Mix) + if res.Requests != 0 || res.Errors == 0 { + t.Errorf("requests=%d errors=%d, want 0 requests and some errors: every response was a 404 with a body", res.Requests, res.Errors) + } + if m := res.Mix; m != nil { + if m.H2Conns == 0 || m.UpgradeConns == 0 { + t.Fatalf("mix did not exercise both HTTP/2 paths: %+v", *m) + } + if m.H2Requests != 0 || m.UpgradeRequests != 0 || m.H2Errors == 0 || m.UpgradeErrors == 0 { + t.Errorf("mix stats %+v: want every H2 and upgrade response counted as an error", *m) + } + } + }) + } +} + +// --------------------------------------------------------------------------- +// #86: every DATA frame's payload counts, padding excluded + +// TestH2BodyBytesMatchH1 compares the body bytes each client reports for the +// same net/http route. net/http's HTTP/2 server ends a body it has already +// flushed with an empty DATA frame; HTTP/1.1 counts the Content-Length (or +// the chunk sizes), so HTTP/2 must count every DATA frame's data. +func TestH2BodyBytesMatchH1(t *testing.T) { + body64k := bytes.Repeat([]byte("x"), 64<<10) + body1k := bytes.Repeat([]byte("y"), 1<<10) + mux := http.NewServeMux() + mux.HandleFunc("/ok", func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("OK")) }) + mux.HandleFunc("/64k", func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(body64k) }) + mux.HandleFunc("/1k-flushed", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(body1k) + w.(http.Flusher).Flush() + }) + host, port := startReferenceServer(t, mux) + + for _, tc := range []struct { + path string + want int + }{ + {"/ok", 2}, + {"/64k", 64 << 10}, + {"/1k-flushed", 1 << 10}, + } { + n1, err1 := h1Once(t, host, port, tc.path) + n2, err2 := h2Once(t, host, port, tc.path) + t.Logf("%-11s h1 bytes=%d | h2 bytes=%d", tc.path, n1, n2) + if err1 != nil || err2 != nil { + t.Fatalf("%s: h1 err=%v h2 err=%v", tc.path, err1, err2) + } + if n1 != tc.want { + t.Fatalf("%s: the HTTP/1.1 control read %d body bytes, want %d", tc.path, n1, tc.want) + } + if n2 != n1 { + t.Errorf("%s: HTTP/2 counted %d body bytes, HTTP/1.1 counted %d: every DATA frame of the response must count", tc.path, n2, n1) + } + } +} + +// TestH2BodyBytesCountEveryDATAFrame pins the DATA frame layouts, including +// padding (never counted as body, as HTTP/1.1 never counts chunk framing) +// and trailers. +func TestH2BodyBytesCountEveryDATAFrame(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, path string) rawH2Action { + switch path { + case "/four-frames": // the usual 16 KiB framing, END_STREAM on the last non-empty frame + c.headers(sid, false, ":status", "200") + for i := range 4 { + c.data(sid, i == 3, 16<<10, 0) + } + case "/empty-end": // what net/http sends for a flushed body + c.headers(sid, false, ":status", "200") + c.data(sid, false, 64<<10, 0) + c.data(sid, true, 0, 0) + case "/trailers": + c.headers(sid, false, ":status", "200") + c.data(sid, false, 1000, 0) + c.data(sid, false, 1000, 0) + c.headers(sid, true, "x-checksum", "abc") + case "/padded": + c.headers(sid, false, ":status", "200") + for i := range 3 { + c.data(sid, i == 2, 1000, 16) + } + default: + c.ok(sid) + } + return rawKeep + }) + host, port := srv.hostPort() + + for _, tc := range []struct { + path string + want int + }{ + {"/four-frames", 64 << 10}, + {"/empty-end", 64 << 10}, + {"/trailers", 2000}, + {"/padded", 3000}, + {"/ok", 2}, + } { + n, err := h2Once(t, host, port, tc.path) + t.Logf("%-12s bytes=%d want=%d err=%v", tc.path, n, tc.want, err) + if err != nil { + t.Fatalf("%s: %v", tc.path, err) + } + if n != tc.want { + t.Errorf("%s: counted %d body bytes, want %d", tc.path, n, tc.want) + } + } +} + +// TestH2FlowControlCreditsPadding guards the other side of #86: the body +// count leaves padding out, but the connection flow-control window must +// still be credited with every DATA payload byte received, padding included +// (RFC 9113 §6.9.1), or padded responses slowly close the window. +func TestH2FlowControlCreditsPadding(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.headers(sid, false, ":status", "200") + for i := range 3 { + c.data(sid, i == 2, 1000, 16) + } + return rawKeep + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 10)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + for range 5 { + if _, err := cl.DoRequest(ctx, 0); err != nil { + t.Fatal(err) + } + } + // The client flushes its WINDOW_UPDATEs within a millisecond or so. + deadline := time.Now().Add(2 * time.Second) + for srv.windowCredit.Load() != srv.dataSent.Load() && time.Now().Before(deadline) { + time.Sleep(2 * time.Millisecond) + } + credit, sent := srv.windowCredit.Load(), srv.dataSent.Load() + t.Logf("DATA payload sent (padding included)=%d, connection window credited=%d", sent, credit) + if credit != sent { + t.Errorf("the client credited %d bytes of connection window for %d bytes of DATA payload: flow control must count padding too", credit, sent) + } +} + +// TestH2ThroughputCountsEveryDATAFrame is #86 as the issue measured it: +// throughput_bps over a 4 x 16 KiB response must be the whole body per +// request, not a quarter of it. +func TestH2ThroughputCountsEveryDATAFrame(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.headers(sid, false, ":status", "200") + for i := range 4 { + c.data(sid, i == 3, 16<<10, 0) + } + return rawKeep + }) + res := runBench(t, Config{URL: srv.url("/"), Duration: 150 * time.Millisecond, Workers: 1, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 4}}) + t.Logf("requests=%d errors=%d rps=%.0f throughput_bps=%.0f bytes/response=%.1f", + res.Requests, res.Errors, res.RequestsPerSec, res.ThroughputBPS, res.ThroughputBPS/res.RequestsPerSec) + if res.Requests == 0 || res.Errors != 0 { + t.Fatalf("requests=%d errors=%d", res.Requests, res.Errors) + } + if perReq := res.ThroughputBPS / res.RequestsPerSec; perReq < 65535.5 || perReq > 65536.5 { + t.Errorf("throughput_bps / requests_per_sec = %.1f bytes per response, want 65536", perReq) + } +} + +// --------------------------------------------------------------------------- +// #88: no lifetime deadline + +// TestH2ConnectionHasNoLifetimeDeadline runs one H2 connection past the +// connection-lifetime bound #88 put on it (5 minutes, warped to 200 ms). Like +// HTTP/1.1, which sets no deadline, the connection must keep serving: no +// errors, no second connection. +func TestH2ConnectionHasNoLifetimeDeadline(t *testing.T) { + armed := warpLongDeadlines(t) + srv := startRawH2(t, respondOK) + res := runBench(t, Config{URL: srv.url("/"), Duration: 600 * time.Millisecond, Workers: 1, MaxRPS: 400, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 4}}) + t.Logf("long deadlines armed=%d requests=%d errors=%d connect_errors=%d connections=%d", + armed.Load(), res.Requests, res.Errors, res.ConnectErrors, srv.accepted.Load()) + if n := armed.Load(); n != 0 { + t.Errorf("%d deadline(s) more than %v ahead were armed on the connection: an H2 connection must not carry a lifetime deadline", n, warpThreshold) + } + if res.Errors != 0 || res.ConnectErrors != 0 || srv.accepted.Load() != 1 { + t.Errorf("errors=%d connect_errors=%d connections=%d, want 0, 0 and 1: the connection died at a lifetime deadline (5 min, warped here to %v)", + res.Errors, res.ConnectErrors, srv.accepted.Load(), 5*time.Minute/warpFactor) + } + if res.Requests == 0 { + t.Error("no request completed") + } +} + +// --------------------------------------------------------------------------- +// #89: a connection that dies without a GOAWAY is redialed + +// TestH2RedialsAfterServerClosesWithoutGOAWAY: the server answers one request +// per connection and closes it, with no GOAWAY. The client must redial and +// keep going, count at most the one in-flight stream of each dead +// connection as an error, and not charge the workers queued for a stream. +func TestH2RedialsAfterServerClosesWithoutGOAWAY(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.ok(sid) + return rawClose + }) + // 8 workers queue for 1 stream: the ones waiting when the connection + // dies never reached it, so they are moved to the redialed connection. + res := runBench(t, Config{URL: srv.url("/"), Duration: 300 * time.Millisecond, Workers: 2, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 1}}) + accepted := srv.accepted.Load() + t.Logf("connections=%d requests=%d errors=%d connect_errors=%d", accepted, res.Requests, res.Errors, res.ConnectErrors) + if accepted < 3 { + t.Fatalf("the server accepted %d connection(s) in %v and requests=%d errors=%d connect_errors=%d: the client did not redial after the server closed without GOAWAY", + accepted, 300*time.Millisecond, res.Requests, res.Errors, res.ConnectErrors) + } + if res.Requests < accepted-1 { + t.Errorf("requests=%d over %d connections: each connection answers one request", res.Requests, accepted) + } + if res.Errors > accepted { + t.Errorf("errors=%d over %d connections with 1 stream each: more than one error per dead connection means requests that never reached it were charged, or a worker spun", + res.Errors, accepted) + } +} + +// TestH2ReadErrorMidStreamFailsEachInFlightStreamOnce: on each connection the +// server answers 3 requests, then starts 4 responses (HEADERS and part of the +// body) and ends the connection in the middle of the 4th response's DATA +// frame, so the client's read fails with an unexpected EOF. Each of those 4 +// streams is exactly one error, the client redials, and the torn bodies are +// credited to nothing. (The server closes rather than resets: on Darwin 27 a +// reset was seen to reach the client's read ~334 ms late, a kernel delay +// that would make this test's count depend on the platform.) +func TestH2ReadErrorMidStreamFailsEachInFlightStreamOnce(t *testing.T) { + const answered, inFlight = 3, 4 + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + if c.requests <= answered { + c.ok(sid) + return rawKeep + } + c.headers(sid, false, ":status", "200") + if c.held++; c.held == inFlight { + c.tornData(sid, 100, 40) // the connection ends 40 bytes into a 100-byte frame + return rawClose + } + c.data(sid, false, 100, 0) // the body starts and never ends + return rawKeep + }) + res := runBench(t, Config{URL: srv.url("/"), Duration: 400 * time.Millisecond, Workers: 1, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: inFlight}}) + accepted, killed := srv.accepted.Load(), srv.killed.Load() + t.Logf("connections=%d torn=%d requests=%d errors=%d connect_errors=%d bytes/response=%.3f", + accepted, killed, res.Requests, res.Errors, res.ConnectErrors, res.ThroughputBPS/res.RequestsPerSec) + if accepted < 3 { + t.Fatalf("the server accepted %d connection(s) in %v and requests=%d errors=%d: the client did not redial after a read error mid-stream", + accepted, 400*time.Millisecond, res.Requests, res.Errors) + } + // The last torn connection can race the end of the run, which is not an error. + if lo, hi := inFlight*(killed-1), inFlight*killed; res.Errors < lo || res.Errors > hi { + t.Errorf("errors=%d after %d torn connections with %d streams in flight each: want %d..%d, one error per in-flight stream", + res.Errors, killed, inFlight, lo, hi) + } + if perReq := res.ThroughputBPS / res.RequestsPerSec; perReq < float64(len(rawOKBody))-0.01 || perReq > float64(len(rawOKBody))+0.01 { + t.Errorf("throughput_bps / requests_per_sec = %.3f bytes per response, want %d: the partial bodies of failed streams leaked into the count", + perReq, len(rawOKBody)) + } +} + +// failWritesConn fails every Write after the first okWrites on the +// connection with ECONNRESET, writing nothing, while reads go on: the +// connection's death shows on the write side only, as when the server is gone +// but its reset has not reached the client's read. +type failWritesConn struct { + net.Conn + okWrites int + writes atomic.Int64 +} + +func (c *failWritesConn) Write(p []byte) (int, error) { + if c.writes.Add(1) > int64(c.okWrites) { + return 0, &net.OpError{Op: "write", Net: "tcp", Err: syscall.ECONNRESET} + } + return c.Conn.Write(p) +} + +// TestH2WriteErrorFailsTheConnection: each connection's writes fail from the +// 6th on (3 go to the handshake) while the server stays silent and keeps the +// connection open, so no read ever fails. The failed write alone must end the +// connection and be redialed. On main a failed flush left bufio's sticky +// error behind, and every later request failed at once on the same dead +// connection: a spin of errors that never redialed. +func TestH2WriteErrorFailsTheConnection(t *testing.T) { + dial := dialTimeoutFunc + t.Cleanup(func() { dialTimeoutFunc = dial }) + dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { + c, err := dial(network, addr, timeout) + if err != nil { + return nil, err + } + return &failWritesConn{Conn: c, okWrites: 5}, nil + } + srv := startRawH2(t, respondOK) + const streams = 4 + res := runBench(t, Config{URL: srv.url("/"), Duration: 300 * time.Millisecond, Workers: 1, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: streams}}) + accepted := srv.accepted.Load() + t.Logf("connections=%d requests=%d errors=%d connect_errors=%d", accepted, res.Requests, res.Errors, res.ConnectErrors) + if accepted < 3 { + t.Fatalf("the server accepted %d connection(s) in %v and requests=%d errors=%d: a connection whose writes fail was not ended and redialed", + accepted, 300*time.Millisecond, res.Requests, res.Errors) + } + if res.Errors > streams*accepted { + t.Errorf("errors=%d over %d connections with %d streams each: requests kept failing on a dead connection", res.Errors, accepted, streams) + } +} + +// TestH2InFlightStreamAnsweredOnceWhenConnectionDiesMidBody: a POST whose +// body is larger than the send window waits in writeBodyFlowControlled for a +// WINDOW_UPDATE the server never sends; the server reads the 65,535 bytes +// the window allowed and closes the connection. The dead connection must be +// torn down (on main its writer waited for window forever), and the stream +// must be answered exactly once, although both readLoop (the connection +// died) and writeLoop (its body write gave up) fail it. +func TestH2InFlightStreamAnsweredOnceWhenConnectionDiesMidBody(t *testing.T) { + srv := startRawH2With(t, rawH2Opts{closeAfterData: 65535}, func(*rawH2Conn, uint32, string) rawH2Action { + return rawKeep // take the request and its body, never answer, never grant window + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/upload", testH2Cfg("POST", nil, make([]byte, 100<<10), 1, 10)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + hc := cl.conns[0].cur.Load() + + // The request, as DoRequest hands it to the connection, with a response + // channel the test owns and never drains until the connection is gone. + ch := make(chan h2Response, 1) + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: cl.headerBlock, data: cl.dataPayload, hasBody: true, respCh: &ch} + + select { + case <-hc.done: + case <-time.After(3 * time.Second): + t.Fatal("the server read the 65,535 body bytes the window allowed and closed the connection, and the client never tore it down: its writer still waits for window on a dead connection") + } + var first h2Response + select { + case first = <-ch: + case <-time.After(2 * time.Second): + t.Fatal("the in-flight stream was never answered") + } + if first.err == nil { + t.Fatalf("the in-flight stream on a dead connection was answered with success: %+v", first) + } + select { + case second := <-ch: + t.Errorf("the in-flight stream was answered twice: %v, then %v", first.err, second.err) + case <-time.After(200 * time.Millisecond): + } + t.Logf("answered once: %v", first.err) +} + +// TestH2GOAWAYConnectionIsReleased: after a GOAWAY the client redials (it +// always has) and must also close the connection it abandons, instead of +// leaking the socket and its writer goroutine. +func TestH2GOAWAYConnectionIsReleased(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.ok(sid) + return rawGoAway // and leave the connection open for the client to close + }) + res := runBench(t, Config{URL: srv.url("/"), Duration: 300 * time.Millisecond, Workers: 1, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 1}}) + if srv.accepted.Load() < 2 { + t.Fatalf("the server accepted %d connection(s), requests=%d errors=%d: the client did not redial after GOAWAY", srv.accepted.Load(), res.Requests, res.Errors) + } + deadline := time.Now().Add(2 * time.Second) + for srv.clientClosed.Load() < srv.accepted.Load() && time.Now().Before(deadline) { + time.Sleep(5 * time.Millisecond) + } + t.Logf("connections=%d closed by the client=%d requests=%d errors=%d", srv.accepted.Load(), srv.clientClosed.Load(), res.Requests, res.Errors) + if closed, accepted := srv.clientClosed.Load(), srv.accepted.Load(); closed < accepted { + t.Errorf("the client closed %d of the %d connections it opened, after Run returned: the ones the server GOAWAYed are leaked", closed, accepted) + } +} + +// --------------------------------------------------------------------------- +// #88 and #89 together + +// TestH2ServerEndedConnectionIsRedialedWithoutLifetimeDeadline: with the +// lifetime deadline gone (#88), a connection ends only when the server or the +// network ends it, and that must still be redialed (#89). The server closes +// every connection, without GOAWAY, 300 ms after accepting it; the lifetime +// bound would have fired at 200 ms. +func TestH2ServerEndedConnectionIsRedialedWithoutLifetimeDeadline(t *testing.T) { + armed := warpLongDeadlines(t) + srv := startRawH2With(t, rawH2Opts{killAfter: 300 * time.Millisecond}, respondOK) + const streams = 4 + res := runBench(t, Config{URL: srv.url("/"), Duration: 800 * time.Millisecond, Workers: 1, MaxRPS: 400, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: streams}}) + killed := srv.killed.Load() + t.Logf("long deadlines armed=%d connections=%d server closes=%d answered after a redial=%d closed by the client=%d requests=%d errors=%d connect_errors=%d", + armed.Load(), srv.accepted.Load(), killed, srv.answeredLate.Load(), srv.clientClosed.Load(), res.Requests, res.Errors, res.ConnectErrors) + if n := armed.Load(); n != 0 { + t.Errorf("%d deadline(s) more than %v ahead were armed: the connection still has a lifetime deadline", n, warpThreshold) + } + if killed == 0 || srv.accepted.Load() < 2 || srv.answeredLate.Load() == 0 { + t.Fatalf("server closes=%d connections=%d requests answered after a redial=%d (requests=%d errors=%d): the client did not redial the connection the server ended", + killed, srv.accepted.Load(), srv.answeredLate.Load(), res.Requests, res.Errors) + } + if res.Errors > streams*killed { + t.Errorf("errors=%d after %d server closes with at most %d streams in flight each", res.Errors, killed, streams) + } + if srv.clientClosed.Load() > 1 { // the one Close at the end of the run + t.Errorf("the client ended %d connections itself before the server did", srv.clientClosed.Load()) + } +} From 381b8be98177d77799a063af704998feb3d4a569 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:38:55 +0200 Subject: [PATCH 02/31] fix(h2client): record the status the response HEADERS carry (#85) readLoop used the :status of a HEADERS frame only when that frame also ended the stream (h2client.go:782-790 at a89f02d). A response whose stream ended on DATA was dispatched as a hard-coded 200 (:806), so a 404, 405 or 500 with a body was recorded as a success, as HTTP/1.1 never records it. Two more holes in the same status path turned up while testing against net/http's own server: - extractStatus accepted a literal :status only when it named the header by static index 8. Indices 8-14 all name :status, and x/net's encoder, which net/http's server uses, names it by 14, so every status outside the 7 of the static table (401, 403, 405, 429, 502, 503, ...) was parsed as 0 and counted as a success, with or without a body. - extractStatus returned 0 for a Huffman-coded value, which an encoder picks whenever it is shorter: 401, 502, 103, 201, 301 and more. The stream slot now keeps the response state, touched by readLoop only and keyed by stream ID so a reused slot starts clean: the status of the final response HEADERS (an interim 1xx is replaced; a HEADERS frame after the final one carries trailers and keeps the status), dispatched whatever ends the stream. extractStatus accepts name indices 8-14 and decodes a Huffman-coded status with one lookup in a table of the 500 encodings, and HeaderBlockFragment strips the padding and priority fields a server may put on a HEADERS frame. DoRequest's status >= 400 check, the same rule h1client applies, now sees every error status. Fixes the prior-knowledge client, the h2c-upgrade client and the H2 halves of -mix, which share readLoop. --- h2client.go | 83 ++++++++++++++++++++++++++++++++++++++++++----------- h2framer.go | 30 +++++++++++++++++-- 2 files changed, 95 insertions(+), 18 deletions(-) diff --git a/h2client.go b/h2client.go index 653a4ae..fa64152 100644 --- a/h2client.go +++ b/h2client.go @@ -202,8 +202,19 @@ type h2Conn struct { // h2StreamSlot holds an atomic pointer to a response channel. // writeLoop stores; readLoop loads and clears. +// +// The other fields are the response state of the stream that last sent a +// frame for this slot. Only readLoop touches them, so they need no +// synchronisation, and they are keyed by stream ID: the first frame of a new +// stream resets them (see h2Conn.stream), so nothing leaks from a stream +// that used the slot before, whether it finished, was reset or was +// abandoned. type h2StreamSlot struct { ch atomic.Pointer[chan h2Response] + + streamID uint32 + status int // :status of the response; 0 when absent or not parseable + final bool // the final (non-1xx) response HEADERS arrived; a later HEADERS frame carries trailers } // h2Response is the result dispatched from the read loop to a waiting worker. @@ -263,8 +274,11 @@ func extractStatus(headerBlock []byte) int { return h2StaticStatus[b-0x88] } - // Fallback: literal header field with name index 8 (:status). - // Safe because we disabled the HPACK dynamic table (SettingHeaderTableSize=0). + // Fallback: literal header field whose name is a static :status entry. + // Static indices 8-14 all name :status, and encoders pick any of them: + // x/net's (net/http's) names it by the LAST match, 14, so a literal 503 + // arrives as 0x0e 0x03 "503". Safe because we disabled the HPACK dynamic + // table (SettingHeaderTableSize=0). var nameIdx int pos++ @@ -279,26 +293,44 @@ func extractStatus(headerBlock []byte) int { return 0 } - if nameIdx != 8 || pos >= len(headerBlock) { + if nameIdx < 8 || nameIdx > 14 || pos >= len(headerBlock) { return 0 } valueByte := headerBlock[pos] pos++ - // Skip Huffman-encoded values (unlikely for 3-digit status codes) - if valueByte&0x80 != 0 { + valueLen := int(valueByte & 0x7F) + if pos+valueLen > len(headerBlock) { return 0 } + value := headerBlock[pos : pos+valueLen] - valueLen := int(valueByte & 0x7F) - if valueLen != 3 || pos+3 > len(headerBlock) { - return 0 + // A Huffman-coded value (RFC 7541 §5.2). Encoders pick it whenever it is + // shorter than the literal, which for a status is every code with at + // least two digits from {0, 1, 2}: 401, 402, 410, 412, 502, 103, 201, + // 301 and more (403 and 503 are not). net/http's server sends them so. + if valueByte&0x80 != 0 { + return h2HuffmanStatus[string(value)] // 0 when it is not a 3-digit status } - return parseStatusCode(headerBlock[pos : pos+3]) + if valueLen != 3 { + return 0 + } + return parseStatusCode(value) } +// h2HuffmanStatus maps the HPACK Huffman encoding of every status code +// 100-599 to the code, so extractStatus decodes a Huffman-coded :status with +// one map lookup (string(b) in a map index does not allocate). +var h2HuffmanStatus = func() map[string]int { + m := make(map[string]int, 500) + for status := 100; status < 600; status++ { + m[string(hpack.AppendHuffmanString(nil, strconv.Itoa(status)))] = status + } + return m +}() + // newH2Client creates a new zero-alloc HTTP/2 client. func newH2Client(host, port, path string, cfg Config) (*h2Client, error) { return newH2ClientWithDialer(host, port, path, cfg, false) @@ -780,13 +812,20 @@ func (hc *h2Conn) readLoop() { switch frame.Type { case frameHeaders: - status := extractStatus(frame.HeaderBlockFragment()) + // The status is the final response's :status, whatever ends the + // stream: this HEADERS frame, a DATA frame, or trailers. An + // interim 1xx response is replaced by the final one; a HEADERS + // frame after the final one carries trailers, not a status. + st := hc.stream(frame.StreamID) + if !st.final { + st.status = extractStatus(frame.HeaderBlockFragment()) + st.final = st.status < 100 || st.status >= 200 + } if frame.StreamEnded() { - idx := (frame.StreamID >> 1) % numSlots - chPtr := hc.streamSlots[idx].ch.Swap(nil) + chPtr := st.ch.Swap(nil) if chPtr != nil { - *chPtr <- h2Response{status: status, bytesRead: 0} + *chPtr <- h2Response{status: st.status, bytesRead: 0} } } @@ -800,10 +839,10 @@ func (hc *h2Conn) readLoop() { } if frame.StreamEnded() { - idx := (frame.StreamID >> 1) % numSlots - chPtr := hc.streamSlots[idx].ch.Swap(nil) + st := hc.stream(frame.StreamID) + chPtr := st.ch.Swap(nil) if chPtr != nil { - *chPtr <- h2Response{status: 200, bytesRead: bytesRead} + *chPtr <- h2Response{status: st.status, bytesRead: bytesRead} } } @@ -857,6 +896,18 @@ func (hc *h2Conn) readLoop() { } } +// stream returns the slot of streamID with its response state reset if the +// slot last held another stream. readLoop only. +func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { + st := &hc.streamSlots[(streamID>>1)%uint32(len(hc.streamSlots))] + if st.streamID != streamID { + st.streamID = streamID + st.status = 0 + st.final = false + } + return st +} + // DoRequest sends an HTTP/2 request and waits for the response. // Fire-and-forget to writeLoop: no resultCh round-trip. Workers wait only on respCh, // which receives from either writeLoop (on error) or readLoop (on response). diff --git a/h2framer.go b/h2framer.go index f006358..91892cc 100644 --- a/h2framer.go +++ b/h2framer.go @@ -22,6 +22,8 @@ const ( flagEndStream = 0x1 flagACK = 0x1 flagEndHeaders = 0x4 + flagPadded = 0x8 // DATA, HEADERS: Pad Length byte first, that many padding bytes last + flagPriority = 0x20 // HEADERS: 5 bytes of stream dependency and weight before the block ) // H2 settings IDs. @@ -51,8 +53,32 @@ func (f *h2Frame) StreamEnded() bool { return f.Flags&flagEndStream != 0 } // IsAck reports whether the ACK flag is set (for SETTINGS and PING). func (f *h2Frame) IsAck() bool { return f.Flags&flagACK != 0 } -// HeaderBlockFragment returns the header block fragment from a HEADERS frame. -func (f *h2Frame) HeaderBlockFragment() []byte { return f.payload } +// HeaderBlockFragment returns the header block fragment from a HEADERS frame, +// without the padding and priority fields a server may add (RFC 9113 §6.2). +func (f *h2Frame) HeaderBlockFragment() []byte { + p := f.unpadded() + if f.Flags&flagPriority != 0 { + if len(p) < 5 { + return nil + } + p = p[5:] + } + return p +} + +// unpadded returns the payload of a DATA or HEADERS frame without its Pad +// Length field and padding. A pad length that does not fit the payload is a +// PROTOCOL_ERROR (RFC 9113 §6.1); nothing in such a payload is usable. +func (f *h2Frame) unpadded() []byte { + p := f.payload + if f.Flags&flagPadded == 0 { + return p + } + if len(p) == 0 || int(p[0]) >= len(p) { + return nil + } + return p[1 : len(p)-int(p[0])] +} // Data returns the data payload from a DATA frame. func (f *h2Frame) Data() []byte { return f.payload } From 5113c3c843d3e05d253e3dc170e5d9d1d55b7d07 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:40:19 +0200 Subject: [PATCH 03/31] fix(h2client): count every DATA frame of a response body (#86) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readLoop forwarded len(frame.Data()) only with the frame that carried END_STREAM (h2client.go:794 and :802-806 at a89f02d); the data of every earlier DATA frame went to the connection WINDOW_UPDATE counter and nowhere else. DoRequest returned that value and RecordSuccess added it to throughput_bps, so a body in 4 x 16 KiB frames counted a quarter of itself, and a body net/http had flushed, which ends with an empty END_STREAM DATA frame, counted 0. A stream that ended on trailers counted 0 too. The stream slot now accumulates the data of every DATA frame of the stream (reset with the rest of the per-stream state when a new stream takes the slot) and dispatches the sum whatever ends the stream, trailers included. Padding is not body: Data() now strips the Pad Length field and the padding, as h1client counts Content-Length or the chunk sizes and never the chunk framing. Flow control is the one place padding still counts (RFC 9113 §6.9.1), so the connection window is credited with frame.Length, the whole payload, exactly what it was credited with before. The bytes of a response that fails (an error status, a reset, a dead connection) are still not recorded: the Benchmarker adds bytes only for a success, for both protocols. README: throughput_bps no longer documents the undercount (added in #84); it states what each protocol counts. --- README.md | 2 +- h2client.go | 21 ++++++++++++++------- h2framer.go | 5 +++-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index b52a503..9f82a34 100644 --- a/README.md +++ b/README.md @@ -391,7 +391,7 @@ empty (`omitempty`). | `errors` | int64 | Errors during the measurement window. | | `duration` | duration (ns) | Wall-clock measurement duration (post-warmup). | | `requests_per_sec` | float64 | `requests / duration.Seconds()`. | -| `throughput_bps` | float64 | Response-body bytes per second. Accurate for H1; the H2 client currently counts only the last DATA frame of each response, so multi-frame H2 bodies are undercounted. | +| `throughput_bps` | float64 | Response-body bytes per second, over successful responses: the Content-Length or the chunk sizes for H1, the data of every DATA frame for H2 (padding excluded, as chunk framing is for H1). | | `latency` | object | `{avg, min, max, p50, p75, p90, p99, p99_9, p99_99}`, each a duration in ns. | | `loadgen_version` | string | loadgen build that produced the run. *(optional)* | | `mode` | string | `"saturation"` or `"rated"`. *(optional)* | diff --git a/h2client.go b/h2client.go index fa64152..9693475 100644 --- a/h2client.go +++ b/h2client.go @@ -215,6 +215,7 @@ type h2StreamSlot struct { streamID uint32 status int // :status of the response; 0 when absent or not parseable final bool // the final (non-1xx) response HEADERS arrived; a later HEADERS frame carries trailers + bytes int // body bytes: the data of every DATA frame so far, padding excluded } // h2Response is the result dispatched from the read loop to a waiting worker. @@ -825,24 +826,29 @@ func (hc *h2Conn) readLoop() { if frame.StreamEnded() { chPtr := st.ch.Swap(nil) if chPtr != nil { - *chPtr <- h2Response{status: st.status, bytesRead: 0} + *chPtr <- h2Response{status: st.status, bytesRead: st.bytes} } } case frameData: - bytesRead := len(frame.Data()) - // Accumulate connection-level WINDOW_UPDATE via atomic counter. // writeLoop flushes this between processing worker requests. - if bytesRead > 0 { - hc.pendingConnWindow.Add(uint32(bytesRead)) + // Flow control counts the whole payload, padding included. + if frame.Length > 0 { + hc.pendingConnWindow.Add(frame.Length) } + // The body is every DATA frame of the stream, not the one that + // ends it: a body split over 16 KiB frames, or followed by an + // empty END_STREAM frame (net/http, after a flush), counts in full. + // Padding is framing, as chunk sizes are in HTTP/1.1: not counted. + st := hc.stream(frame.StreamID) + st.bytes += len(frame.Data()) + if frame.StreamEnded() { - st := hc.stream(frame.StreamID) chPtr := st.ch.Swap(nil) if chPtr != nil { - *chPtr <- h2Response{status: st.status, bytesRead: bytesRead} + *chPtr <- h2Response{status: st.status, bytesRead: st.bytes} } } @@ -904,6 +910,7 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { st.streamID = streamID st.status = 0 st.final = false + st.bytes = 0 } return st } diff --git a/h2framer.go b/h2framer.go index 91892cc..091085e 100644 --- a/h2framer.go +++ b/h2framer.go @@ -80,8 +80,9 @@ func (f *h2Frame) unpadded() []byte { return p[1 : len(p)-int(p[0])] } -// Data returns the data payload from a DATA frame. -func (f *h2Frame) Data() []byte { return f.payload } +// Data returns the data of a DATA frame, without its padding. Flow control +// counts the whole payload, padding included (RFC 9113 §6.9.1): use Length. +func (f *h2Frame) Data() []byte { return f.unpadded() } // ErrCode returns the error code from a RST_STREAM frame (payload[0:4]). func (f *h2Frame) ErrCode() uint32 { From 83d5ccbdcf6f7e2b7e24e4b07777425d31c4ac29 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:41:05 +0200 Subject: [PATCH 04/31] fix(h2client): no lifetime deadline on an HTTP/2 connection (#88) completeH2Handshake armed conn.SetDeadline(now + 5 min) on every connection (h2client.go:616-618 at a89f02d). It was not an idle timeout: New dials every connection of a run together, so 300 s later every read and write on every H2 connection of the run failed with i/o timeout at once, and rps fell to 0 for the rest of it (the prior-knowledge client, the h2c-upgrade client and the H2 halves of -mix alike). The fix gives an HTTP/2 connection the deadline semantics an HTTP/1.1 connection has: none once the handshake is done (the handshake keeps its 10 s read deadline, cleared as before). h1client arms no deadline at all; a connection lives until the server or the network ends it, or the client closes it. No bound is needed for the backstop the comment claimed: - a write blocked on a peer that stopped reading, and a read waiting on a silent peer, are interrupted by Close, which Benchmarker.Run calls after cancelling the run context (tls.Conn.Close skips the close_notify alert when a Write is in flight, so it cannot block either); - writeBodyFlowControlled's wait for window polls done, which Close closes, and never did depend on the deadline; - workers waiting on a response select on ctx and done. A stalled connection therefore stalls until the run ends, as an HTTP/1.1 connection to the same peer does. A progress watchdog (a read deadline armed while streams are outstanding, refreshed per frame) would bound that for both protocols; it is left out so the two stay alike. The test warps any deadline armed more than 30 s ahead to 1/1500 of its distance through the existing dialTimeoutFunc hook (5 min -> 200 ms) and requires none to be armed, and the connection to serve on with no error. --- h2client.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/h2client.go b/h2client.go index 9693475..d23cf44 100644 --- a/h2client.go +++ b/h2client.go @@ -184,7 +184,7 @@ type h2Conn struct { // currently-writing stream's window lives in curStreamWindow keyed by // curStreamID; readLoop replenishes both on WINDOW_UPDATE. Without this a // 64KiB body (post-64k-h2 = 65536 B) exceeds the 65535 window by one byte - // and the request hangs to the 5-min deadline. + // and the request hangs until the run ends. connSendWindow atomic.Int64 curStreamID atomic.Uint32 curStreamWindow atomic.Int64 @@ -644,12 +644,14 @@ func completeH2Handshake(conn net.Conn, br *bufio.Reader, addr string, maxStream } } + // The handshake is done: from here the connection has no deadline, as an + // HTTP/1.1 connection has none. It lives until the server or the network + // ends it or the client closes it, and Close is what interrupts a read or + // a write blocked on a peer that stopped reading. An absolute deadline + // here ended every connection of a run at the same instant, however + // healthy (#88). _ = conn.SetReadDeadline(time.Time{}) - // Set a deadline covering the entire benchmark lifetime. - // Prevents infinite blocks on TCP write if H2 flow control deadlocks. - _ = conn.SetDeadline(time.Now().Add(5 * time.Minute)) - effectiveStreams := min(serverMaxStreams, uint32(maxStreams)) if effectiveStreams < 1 { effectiveStreams = 100 @@ -1004,7 +1006,8 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { // hc.curStreamID; readLoop replenishes connSendWindow/curStreamWindow on // WINDOW_UPDATE. When a window is exhausted we flush the buffered frames (so the // server can consume + replenish) and poll — no extra goroutine/channel. The -// conn's 5-min deadline (completeH2Handshake) is the deadlock backstop. +// wait ends when the connection is closed (done), which Close does at the end +// of a run: that is the backstop against a peer that never grants window. // // Without this, a 64KiB body (post-64k-h2 = 65536 B) either trips FRAME_SIZE_ERROR // (64KiB DATA frame vs a 16384-default server) or overruns the 65535 window. From 7ab43f98d901c2cae745dd7fed778bb6e2a70ed8 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:53:49 +0200 Subject: [PATCH 05/31] fix(h2client): redial a connection that dies without a GOAWAY (#89) When readLoop's ReadFrame failed (h2client.go:766-778 at a89f02d) it failed the streams in flight and returned, but set neither closed nor done. DoRequest redials only a closed connection (:871), so after a close, a reset or a timeout from the server or the network the slot kept the dead connection for the rest of the run. writeLoop kept writing to it: a request whose HEADERS still fit in the bufio.Writer waited for a response nobody would read (a silent hang; failing test: 1 request, 0 errors, 1 connection in 300 ms), a body waiting for flow-control window waited forever, and once a flush failed bufio's sticky error failed every later request at once (a spin; failing test: 73,782 errors on 1 connection in 300 ms). The redial machinery was reached only after a GOAWAY. A connection that dies under the client now ends through failConn: mark it closed (DoRequest redials through reconnectSlot, paced by the slot's existing connectBackoff, a failed dial counted in connect_errors as before), fail every stream in flight on it, then close it (done, which stops writeLoop and wakes waiting workers, and the socket). readLoop calls it on a read error; writeLoop calls it on a write error (writeFailed), because the read does not always see the death: the server may keep its side open, or its reset may not have reached the read yet. Accounting matches h1client, which counts a request whose response cannot be read as an error: - a request handed to the connection is in flight and is exactly one error. Whoever takes its response channel out of the stream slot answers it; writeLoop now takes it with a CompareAndSwap instead of a Store, so a write error racing failStreams cannot answer twice and block writeLoop on the full channel; - a request still waiting for a stream when the connection dies never reached the server, so it is not an error: it is taken to the redialed connection. Each retry needs a connection to have died, and every connection is dialed through the backoff, so nothing spins; ctx ends it. h1client also retries once, transparently, a request whose write fails. An HTTP/2 write is a batch of several streams' frames, and which of them reached the server before it failed is unknown, so those streams count as in flight. Found while fixing this: a GOAWAY set closed but never closed the connection, and closeConn's CompareAndSwap on closed then made every later close a no-op, so each GOAWAYed connection leaked its socket and a writeLoop ticking every millisecond for the life of the process (failing test: the client closed 0 of the 40 connections it opened). The GOAWAY branch now goes through failConn too, and closeConn closes once under a sync.Once whatever set closed. A GOAWAY still fails the streams in flight at once, as before. --- h2client.go | 160 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 115 insertions(+), 45 deletions(-) diff --git a/h2client.go b/h2client.go index d23cf44..6a2a908 100644 --- a/h2client.go +++ b/h2client.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "crypto/tls" + "errors" "fmt" "net" "strconv" @@ -193,10 +194,15 @@ type h2Conn struct { // Concurrency limit streamSem chan struct{} - // Shutdown signal — closed by closeConn to unblock readLoop/writeLoop/workers - done chan struct{} + // Shutdown signal — closed by closeConn (once, under closeOnce) to unblock + // readLoop/writeLoop/workers, whether the client closes the connection or + // readLoop fails it (failConn). + done chan struct{} + closeOnce sync.Once - addr string + addr string + // closed marks a connection that takes no new request: DoRequest + // redials the slot instead. Set before done is closed. closed atomic.Bool } @@ -719,18 +725,18 @@ func (hc *h2Conn) writeLoop() { count++ if count%64 == 0 { hc.flushWindowUpdate() - _ = hc.bufWriter.Flush() // send WINDOW_UPDATE to the network NOW + hc.writeFailed(hc.bufWriter.Flush()) // send WINDOW_UPDATE to the network NOW } default: break drain } } hc.flushWindowUpdate() - _ = hc.bufWriter.Flush() + hc.writeFailed(hc.bufWriter.Flush()) case <-ticker.C: hc.flushWindowUpdate() - _ = hc.bufWriter.Flush() + hc.writeFailed(hc.bufWriter.Flush()) case <-hc.done: // Drain pending requests — send errors to respCh so workers unblock. @@ -748,6 +754,17 @@ func (hc *h2Conn) writeLoop() { } } +// writeFailed ends the connection after a write to it failed (nil: no-op). +// The peer is gone, and readLoop does not always see it: the server may keep +// its side open, or its reset may not have reached the read yet. Without +// this the bufio.Writer keeps the error and fails every later request on the +// dead connection at once, a spin of errors that never redials (#89). +func (hc *h2Conn) writeFailed(err error) { + if err != nil && !hc.closed.Load() { + hc.failConn(fmt.Errorf("connection error: %w", err)) + } +} + func (hc *h2Conn) flushWindowUpdate() { if pending := hc.pendingConnWindow.Swap(0); pending > 0 { _ = hc.framer.WriteWindowUpdate(0, pending) @@ -772,12 +789,14 @@ func (hc *h2Conn) processWriteReq(req h2WriteReq) { if err == nil && req.hasBody { err = hc.writeBodyFlowControlled(streamID, req.data) } - if err != nil { - hc.streamSlots[slotIdx].ch.Store(nil) - if req.respCh != nil { - *req.respCh <- h2Response{err: err} - } + // Whoever takes respCh out of the slot answers it, exactly once: + // readLoop may have taken it first (a response, a reset, or + // failStreams on a connection that died while this was written). + // Answering it twice would block here on the full channel. + if err != nil && req.respCh != nil && hc.streamSlots[slotIdx].ch.CompareAndSwap(req.respCh, nil) { + *req.respCh <- h2Response{err: err} } + hc.writeFailed(err) case h2WriteSettingsAck: _ = hc.framer.WriteSettingsAck() @@ -801,15 +820,12 @@ func (hc *h2Conn) readLoop() { frame, err := hc.framer.ReadFrame() if err != nil { if hc.closed.Load() { - return - } - // Notify all pending streams - for i := range hc.streamSlots { - chPtr := hc.streamSlots[i].ch.Swap(nil) - if chPtr != nil { - *chPtr <- h2Response{err: fmt.Errorf("connection error: %w", err)} - } + return // the client closed it (Close, or reconnectSlot replacing it) } + // The server or the network ended the connection without a + // GOAWAY: a close, a reset, a timeout. Fail it, so its in-flight + // streams are errors and DoRequest redials. + hc.failConn(fmt.Errorf("connection error: %w", err)) return } @@ -880,13 +896,7 @@ func (hc *h2Conn) readLoop() { } case frameGoAway: - hc.closed.Store(true) - for i := range hc.streamSlots { - chPtr := hc.streamSlots[i].ch.Swap(nil) - if chPtr != nil { - *chPtr <- h2Response{err: fmt.Errorf("goaway: code=%d", frame.GoAwayErrCode())} - } - } + hc.failConn(fmt.Errorf("goaway: code=%d", frame.GoAwayErrCode())) return case frameWindowUpdate: @@ -917,33 +927,63 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { return st } +// errH2NotSent is roundTrip's report that the connection died before the +// request was handed to it. Nothing reached the server, so it is not a +// failed request: DoRequest takes the request to the redialed connection. +// Never returned to DoRequest's caller. +var errH2NotSent = errors.New("h2client: connection gone before the request was sent") + // DoRequest sends an HTTP/2 request and waits for the response. // Fire-and-forget to writeLoop: no resultCh round-trip. Workers wait only on respCh, // which receives from either writeLoop (on error) or readLoop (on response). func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { idx := workerID % len(c.conns) slot := c.conns[idx] - hc := slot.cur.Load() - - // A server that closed/GOAWAYed this connection mid-cell leaves the slot - // dead; re-dial (paced) so the next request gets a fresh conn instead of - // spinning closed-conn errors. backoff lives in reconnectSlot. - if hc == nil || hc.closed.Load() { - hc = c.reconnectSlot(ctx, slot) - if hc == nil { - if ctx.Err() != nil { - return 0, ctx.Err() + for { + hc := slot.cur.Load() + + // A server that closed, reset or GOAWAYed this connection mid-cell + // leaves the slot dead (readLoop's failConn); re-dial (paced) so the + // request gets a fresh conn instead of spinning closed-conn errors. + // backoff lives in reconnectSlot. + if hc == nil || hc.closed.Load() { + hc = c.reconnectSlot(ctx, slot) + if hc == nil { + if ctx.Err() != nil { + return 0, ctx.Err() + } + return 0, fmt.Errorf("h2client: conn[%d] reconnect failed", idx) } - return 0, fmt.Errorf("h2client: conn[%d] reconnect failed", idx) } + + n, err := c.roundTrip(ctx, hc, idx) + if err != errH2NotSent { // the sentinel itself, never wrapped + return n, err + } + // The connection died while this request waited for a stream. Each + // pass through here needs a connection to have died, and every + // connection is dialed through reconnectSlot's backoff, so a server + // that keeps closing connections cannot make this loop spin; ctx + // ends it. } +} - // Acquire stream semaphore +// roundTrip sends one request on hc and waits for its response. It returns +// errH2NotSent if hc died before the request was handed to its writer. +func (c *h2Client) roundTrip(ctx context.Context, hc *h2Conn, idx int) (int, error) { + // Acquire a stream. When the connection dies, the workers queued here + // wake through done, or through the tokens its failed streams return. select { case <-hc.streamSem: + case <-hc.done: + return 0, errH2NotSent case <-ctx.Done(): return 0, ctx.Err() } + if hc.closed.Load() { + hc.streamSem <- struct{}{} + return 0, errH2NotSent + } // Get heap-allocated response channel pointer from pool chPtr := hc.chanPool.Get().(*chan h2Response) @@ -967,14 +1007,17 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { case <-hc.done: hc.chanPool.Put(chPtr) hc.streamSem <- struct{}{} - return 0, fmt.Errorf("h2client: conn[%d] connection closing", idx) + return 0, errH2NotSent case <-ctx.Done(): hc.chanPool.Put(chPtr) hc.streamSem <- struct{}{} return 0, ctx.Err() } - // Single wait — receives normal responses (from readLoop) AND write errors (from writeLoop) + // Single wait — receives normal responses (from readLoop) AND write errors (from writeLoop). + // The request is in flight now: if the connection dies, exactly one of + // failStreams, writeLoop (a write error, or its drain of writeCh) and + // done below answers it, so it is one error, never zero or two. select { case resp := <-*chPtr: hc.chanPool.Put(chPtr) @@ -1007,7 +1050,8 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { // WINDOW_UPDATE. When a window is exhausted we flush the buffered frames (so the // server can consume + replenish) and poll — no extra goroutine/channel. The // wait ends when the connection is closed (done), which Close does at the end -// of a run: that is the backstop against a peer that never grants window. +// of a run and failConn when the connection dies: that is the backstop +// against a peer that never grants window. // // Without this, a 64KiB body (post-64k-h2 = 65536 B) either trips FRAME_SIZE_ERROR // (64KiB DATA frame vs a 16384-default server) or overruns the 65535 window. @@ -1065,10 +1109,36 @@ func (c *h2Client) Close() { } } +// closeConn closes the connection: it takes no new request, done unblocks +// workers, readLoop and writeLoop, and the socket is released. Idempotent, +// and safe from any goroutine. func (hc *h2Conn) closeConn() { - if !hc.closed.CompareAndSwap(false, true) { - return // already closed + hc.closed.Store(true) + hc.closeOnce.Do(func() { + close(hc.done) // unblock workers, readLoop, and writeLoop + _ = hc.conn.Close() + }) +} + +// failConn ends a connection that died under the client: the server closed +// or reset it, sent GOAWAY, or a write to it failed. It marks the connection +// closed first, so DoRequest redials instead of queueing onto a connection +// nobody reads, fails every stream in flight on it once, and closes it, +// which stops writeLoop, releases the socket and wakes the workers queued +// for a stream. Called by readLoop and writeLoop; safe from both at once. +func (hc *h2Conn) failConn(err error) { + hc.closed.Store(true) + hc.failStreams(err) + hc.closeConn() +} + +// failStreams answers every stream still registered on the connection with +// err. Each response channel is taken out of its slot before it is answered, +// so no stream is answered twice. +func (hc *h2Conn) failStreams(err error) { + for i := range hc.streamSlots { + if chPtr := hc.streamSlots[i].ch.Swap(nil); chPtr != nil { + *chPtr <- h2Response{err: err} + } } - close(hc.done) // unblock workers, readLoop, and writeLoop - _ = hc.conn.Close() } From 1abbd1c84483efdea5a4afc61c7f86611b83cc95 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 01:32:37 +0200 Subject: [PATCH 06/31] fix(h2client): a response that arrives before the connection closes is its request's outcome (#89) Review finding on the #89 fix (CodeRabbit): readLoop answers a stream before it fails the connection, so a worker can find both its response and the closed connection (done) ready, and select picks at random between them; an answered request then becomes a "connection closing" error. A worker parked in the select is not exposed, because the send hands the response to it before done closes; one that reaches the select after both (descheduled between handing the request over and waiting, as under saturation load) is. #89's failConn makes a close right after a response far more common than the GOAWAY-only path before it. The wait moves into await, whose done branch now takes a response already delivered before it reports the connection closing; finish is the shared result path (status >= 400 is an error, as before). The unit test puts both cases in front of await 200 times; without the check it fails on the first few, since select picks either. Also corrects two comments the #89 commit left naming readLoop as the only caller of failConn; writeLoop calls it too. --- h2client.go | 51 ++++++++++++++++++++++++++----------- h2client_accounting_test.go | 19 ++++++++++++++ 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/h2client.go b/h2client.go index 6a2a908..392c9e3 100644 --- a/h2client.go +++ b/h2client.go @@ -196,7 +196,7 @@ type h2Conn struct { // Shutdown signal — closed by closeConn (once, under closeOnce) to unblock // readLoop/writeLoop/workers, whether the client closes the connection or - // readLoop fails it (failConn). + // it dies under the client (failConn, from readLoop or writeLoop). done chan struct{} closeOnce sync.Once @@ -943,7 +943,7 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { hc := slot.cur.Load() // A server that closed, reset or GOAWAYed this connection mid-cell - // leaves the slot dead (readLoop's failConn); re-dial (paced) so the + // leaves the slot dead (failConn); re-dial (paced) so the // request gets a fresh conn instead of spinning closed-conn errors. // backoff lives in reconnectSlot. if hc == nil || hc.closed.Load() { @@ -1014,21 +1014,18 @@ func (c *h2Client) roundTrip(ctx context.Context, hc *h2Conn, idx int) (int, err return 0, ctx.Err() } - // Single wait — receives normal responses (from readLoop) AND write errors (from writeLoop). - // The request is in flight now: if the connection dies, exactly one of - // failStreams, writeLoop (a write error, or its drain of writeCh) and - // done below answers it, so it is one error, never zero or two. + return hc.await(ctx, chPtr, idx) +} + +// await waits for the answer to a request in flight on hc: a response (from +// readLoop) or an error (from writeLoop, or failStreams). If the connection +// dies, exactly one of failStreams, writeLoop (a write error, or its drain of +// writeCh) and done below answers the request, so it is one error, never +// zero or two. +func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (int, error) { select { case resp := <-*chPtr: - hc.chanPool.Put(chPtr) - hc.streamSem <- struct{}{} - if resp.err != nil { - return 0, resp.err - } - if resp.status >= 400 { - return resp.bytesRead, statusError(resp.status) - } - return resp.bytesRead, nil + return hc.finish(chPtr, resp) case <-ctx.Done(): // Worker exits, abandons chPtr. The heap-allocated channel (~120 bytes) // will be GC'd when the slot is overwritten by the next stream ID at @@ -1037,11 +1034,35 @@ func (c *h2Client) roundTrip(ctx context.Context, hc *h2Conn, idx int) (int, err hc.streamSem <- struct{}{} return 0, ctx.Err() case <-hc.done: + // readLoop answers a stream before it fails the connection, so a + // response may be waiting here too. A worker parked in this select + // gets it by direct handoff before done closes, but one that reaches + // the select after both finds both ready, and select picks at random: + // a request the server answered must not become an error. + select { + case resp := <-*chPtr: + return hc.finish(chPtr, resp) + default: + } hc.streamSem <- struct{}{} return 0, fmt.Errorf("h2client: conn[%d] connection closing", idx) } } +// finish returns the stream's channel and token and turns its answer into +// DoRequest's result: a status >= 400 is an error, as h1client counts it. +func (hc *h2Conn) finish(chPtr *chan h2Response, resp h2Response) (int, error) { + hc.chanPool.Put(chPtr) + hc.streamSem <- struct{}{} + if resp.err != nil { + return 0, resp.err + } + if resp.status >= 400 { + return resp.bytesRead, statusError(resp.status) + } + return resp.bytesRead, nil +} + // writeBodyFlowControlled sends a request body as DATA frames, respecting BOTH // the server's SETTINGS_MAX_FRAME_SIZE and its connection + per-stream send // windows (RFC 7540 §6.9). The writer goroutine is sequential (one body at a diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 5ba94f6..74c9534 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -889,6 +889,25 @@ func TestH2ReadErrorMidStreamFailsEachInFlightStreamOnce(t *testing.T) { } } +// TestH2ResponseWinsOverConnectionClose: a worker that reaches its wait +// after readLoop has both answered its stream and failed the connection finds +// the response and the closed connection ready at once. The response is the +// request's outcome; select alone would pick at random. (Found in review of +// the #89 fix, which makes a close right after a response common.) +func TestH2ResponseWinsOverConnectionClose(t *testing.T) { + hc := &h2Conn{done: make(chan struct{}), streamSem: make(chan struct{}, 1)} + close(hc.done) + for i := range 200 { + ch := make(chan h2Response, 1) + ch <- h2Response{status: 200, bytesRead: 2} + n, err := hc.await(context.Background(), &ch, 0) + <-hc.streamSem // the token await returned + if err != nil || n != 2 { + t.Fatalf("attempt %d: a delivered 200 response on a closed connection returned (%d, %v), want (2, nil)", i, n, err) + } + } +} + // failWritesConn fails every Write after the first okWrites on the // connection with ECONNRESET, writing nothing, while reads go on: the // connection's death shows on the write side only, as when the server is gone From 72a8ac15a00523b6d702484429eaa715b851947c Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:13:23 +0200 Subject: [PATCH 07/31] test(h2client): failing tests for the round-1 review of #91 Five tests for defects the review of PR #91 found in the fixed HTTP/2 client, and a guard for the #88 tests. On the PR head before this commit (2930ded) each new test fails for its stated reason: - TestH2StreamIDExhaustionRedials: request 4 fails with "stream ID exhausted" on a connection with 3 IDs left, and no redial follows. - TestH2WriteErrorWhileBodyWaitsForWindow: the flush made while a body waits for send window fails; the connection is never ended. - TestH2WindowCreditSentWhileBodyWaitsForWindow: 0 of 10,000 received bytes credited while the body waits for window. - TestH2RedialBacksOffOnlyWhenTheServerLooksDown/answered: the redial of a connection that had answered sleeps the backoff (h1client does not). - TestH2ResponseWithoutAReadableStatusIsNotASuccess: a literal-name :status, a response without :status and malformed padding are all recorded as successes. Its subtests "never answered" and "run over" are guards and pass on 2930ded: the backoff still paces a server that drops every connection, and no connection is dialed after the run's context ends. The #88 tests now also require the deadline warp to have seen every connection (requireWarped): armed == 0 proves nothing about a connection dialed around the dialTimeoutFunc hook. --- h2client_accounting_test.go | 338 +++++++++++++++++++++++++++++++++--- 1 file changed, 315 insertions(+), 23 deletions(-) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 74c9534..d89901a 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -58,8 +58,9 @@ type rawH2Handler func(c *rawH2Conn, streamID uint32, path string) rawH2Action type rawH2Server struct { ln net.Listener handler rawH2Handler - killAfter time.Duration // > 0: the server closes each connection (no GOAWAY) this long after accepting it - closeAfterData int // > 0: the server closes a connection (no GOAWAY) once it has read this many DATA bytes on it + killAfter time.Duration // > 0: the server closes each connection (no GOAWAY) this long after accepting it + closeAfterData int // > 0: the server closes a connection (no GOAWAY) once it has read this many DATA bytes on it + settings []http2.Setting // the server's SETTINGS (none: every default) accepted atomic.Int64 // connections accepted killed atomic.Int64 // connections the server ended (rawClose or killAfter) @@ -90,6 +91,7 @@ type rawH2Conn struct { type rawH2Opts struct { killAfter time.Duration closeAfterData int + settings []http2.Setting } func startRawH2(t *testing.T, h rawH2Handler) *rawH2Server { @@ -102,7 +104,7 @@ func startRawH2With(t *testing.T, opts rawH2Opts, h rawH2Handler) *rawH2Server { if err != nil { t.Fatal(err) } - s := &rawH2Server{ln: ln, handler: h, killAfter: opts.killAfter, closeAfterData: opts.closeAfterData} + s := &rawH2Server{ln: ln, handler: h, killAfter: opts.killAfter, closeAfterData: opts.closeAfterData, settings: opts.settings} s.wg.Add(1) go func() { defer s.wg.Done() @@ -157,7 +159,7 @@ func (s *rawH2Server) serve(nc net.Conn) { br := bufio.NewReader(nc) fr := http2.NewFramer(nc, br) - if !rawH2Handshake(nc, br, fr) { + if !rawH2Handshake(nc, br, fr, s.settings...) { _ = nc.Close() return } @@ -223,7 +225,7 @@ func (s *rawH2Server) serve(nc net.Conn) { // rawH2Handshake reads the client connection preface, performing the h2c // upgrade first when the connection starts with an HTTP/1.1 request, and // sends the server's SETTINGS. -func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer) bool { +func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer, settings ...http2.Setting) bool { head, err := br.Peek(len(http2.ClientPreface)) if err != nil { return false @@ -239,7 +241,7 @@ func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer) bool { if _, err := io.WriteString(nc, "HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: h2c\r\n\r\n"); err != nil { return false } - if err := fr.WriteSettings(); err != nil { + if err := fr.WriteSettings(settings...); err != nil { return false } if head, err = br.Peek(len(http2.ClientPreface)); err != nil || string(head) != http2.ClientPreface { @@ -251,7 +253,7 @@ func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer) bool { if _, err := br.Discard(len(http2.ClientPreface)); err != nil { return false } - return fr.WriteSettings() == nil + return fr.WriteSettings(settings...) == nil } // headers writes one HEADERS frame carrying fields, given as name, value pairs. @@ -407,21 +409,29 @@ const ( warpFactor = 1500 ) -// warpConn applies the deadline warp and counts the deadlines it warped. +// warpCounts is what the deadline warp saw: every deadline armed on a warped +// connection (seen) and the long ones it warped (armed). +type warpCounts struct { + seen atomic.Int64 // deadlines armed through the warp, of any length + armed atomic.Int64 // deadlines more than warpThreshold ahead, warped +} + +// warpConn applies the deadline warp and counts the deadlines it saw. type warpConn struct { net.Conn - armed *atomic.Int64 + counts *warpCounts } func (c warpConn) warp(t time.Time) time.Time { if t.IsZero() { return t } + c.counts.seen.Add(1) d := time.Until(t) if d <= warpThreshold { return t } - c.armed.Add(1) + c.counts.armed.Add(1) return time.Now().Add(d / warpFactor) } @@ -430,10 +440,10 @@ func (c warpConn) SetReadDeadline(t time.Time) error { return c.Conn.SetReadDea func (c warpConn) SetWriteDeadline(t time.Time) error { return c.Conn.SetWriteDeadline(c.warp(t)) } // warpLongDeadlines routes every loadgen dial through warpConn for the rest -// of the test and returns the count of warped (long) deadlines armed. -func warpLongDeadlines(t *testing.T) *atomic.Int64 { +// of the test and returns what the warp saw. +func warpLongDeadlines(t *testing.T) *warpCounts { t.Helper() - armed := new(atomic.Int64) + counts := new(warpCounts) dial := dialTimeoutFunc t.Cleanup(func() { dialTimeoutFunc = dial }) dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { @@ -441,9 +451,22 @@ func warpLongDeadlines(t *testing.T) *atomic.Int64 { if err != nil { return nil, err } - return warpConn{Conn: c, armed: armed}, nil + return warpConn{Conn: c, counts: counts}, nil + } + return counts +} + +// requireWarped fails the test unless every connection the server accepted +// armed a deadline through the warp: the handshake's 10 s read deadline, at +// least. armed == 0 proves no lifetime deadline only for a connection the +// warp wraps; one dialed around the dialTimeoutFunc hook would carry a real +// 5-minute deadline that no sub-second test can see fire. +func requireWarped(t *testing.T, w *warpCounts, accepted int64) { + t.Helper() + if seen := w.seen.Load(); seen < accepted { + t.Fatalf("the warp saw %d deadline(s) for %d connection(s): the connections were not dialed through dialTimeoutFunc, so armed=%d proves nothing", + seen, accepted, w.armed.Load()) } - return armed } // --------------------------------------------------------------------------- @@ -797,13 +820,14 @@ func TestH2ThroughputCountsEveryDATAFrame(t *testing.T) { // HTTP/1.1, which sets no deadline, the connection must keep serving: no // errors, no second connection. func TestH2ConnectionHasNoLifetimeDeadline(t *testing.T) { - armed := warpLongDeadlines(t) + warp := warpLongDeadlines(t) srv := startRawH2(t, respondOK) res := runBench(t, Config{URL: srv.url("/"), Duration: 600 * time.Millisecond, Workers: 1, MaxRPS: 400, HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 4}}) - t.Logf("long deadlines armed=%d requests=%d errors=%d connect_errors=%d connections=%d", - armed.Load(), res.Requests, res.Errors, res.ConnectErrors, srv.accepted.Load()) - if n := armed.Load(); n != 0 { + t.Logf("deadlines seen=%d long deadlines armed=%d requests=%d errors=%d connect_errors=%d connections=%d", + warp.seen.Load(), warp.armed.Load(), res.Requests, res.Errors, res.ConnectErrors, srv.accepted.Load()) + requireWarped(t, warp, srv.accepted.Load()) + if n := warp.armed.Load(); n != 0 { t.Errorf("%d deadline(s) more than %v ahead were armed on the connection: an H2 connection must not carry a lifetime deadline", n, warpThreshold) } if res.Errors != 0 || res.ConnectErrors != 0 || srv.accepted.Load() != 1 { @@ -1034,15 +1058,16 @@ func TestH2GOAWAYConnectionIsReleased(t *testing.T) { // every connection, without GOAWAY, 300 ms after accepting it; the lifetime // bound would have fired at 200 ms. func TestH2ServerEndedConnectionIsRedialedWithoutLifetimeDeadline(t *testing.T) { - armed := warpLongDeadlines(t) + warp := warpLongDeadlines(t) srv := startRawH2With(t, rawH2Opts{killAfter: 300 * time.Millisecond}, respondOK) const streams = 4 res := runBench(t, Config{URL: srv.url("/"), Duration: 800 * time.Millisecond, Workers: 1, MaxRPS: 400, HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: streams}}) killed := srv.killed.Load() - t.Logf("long deadlines armed=%d connections=%d server closes=%d answered after a redial=%d closed by the client=%d requests=%d errors=%d connect_errors=%d", - armed.Load(), srv.accepted.Load(), killed, srv.answeredLate.Load(), srv.clientClosed.Load(), res.Requests, res.Errors, res.ConnectErrors) - if n := armed.Load(); n != 0 { + t.Logf("deadlines seen=%d long deadlines armed=%d connections=%d server closes=%d answered after a redial=%d closed by the client=%d requests=%d errors=%d connect_errors=%d", + warp.seen.Load(), warp.armed.Load(), srv.accepted.Load(), killed, srv.answeredLate.Load(), srv.clientClosed.Load(), res.Requests, res.Errors, res.ConnectErrors) + requireWarped(t, warp, srv.accepted.Load()) + if n := warp.armed.Load(); n != 0 { t.Errorf("%d deadline(s) more than %v ahead were armed: the connection still has a lifetime deadline", n, warpThreshold) } if killed == 0 || srv.accepted.Load() < 2 || srv.answeredLate.Load() == 0 { @@ -1056,3 +1081,270 @@ func TestH2ServerEndedConnectionIsRedialedWithoutLifetimeDeadline(t *testing.T) t.Errorf("the client ended %d connections itself before the server did", srv.clientClosed.Load()) } } + +// --------------------------------------------------------------------------- +// Review of the #85-#89 fixes (PR #91, round 1) + +// TestH2StreamIDExhaustionRedials: with no lifetime deadline (#88) a +// connection lives as long as the server keeps it, so a long run can use up +// its 2^30 client stream IDs. RFC 9113 §5.1.1: a client that cannot open a +// new stream opens a new connection. The request that finds the IDs used up +// never reached the server, so it goes to a redialed connection and does not +// fail, and the used-up connection is closed. On 2930ded every request from +// then on failed at once on a connection nothing retired. +func TestH2StreamIDExhaustionRedials(t *testing.T) { + srv := startRawH2(t, respondOK) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 1)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + // Three stream IDs left on the first connection: 2^31-5, 2^31-3, 2^31-1. + cl.conns[0].cur.Load().nextStreamID.Store(0x7FFFFFFB) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + for i := range 8 { + if _, err := cl.DoRequest(ctx, 0); err != nil { + t.Fatalf("request %d: %v (connections=%d): a request that finds the stream IDs used up must go to a new connection, not fail", + i+1, err, srv.accepted.Load()) + } + } + deadline := time.Now().Add(2 * time.Second) + for srv.clientClosed.Load() < 1 && time.Now().Before(deadline) { + time.Sleep(2 * time.Millisecond) + } + t.Logf("8 requests: connections=%d closed by the client=%d", srv.accepted.Load(), srv.clientClosed.Load()) + if n := srv.accepted.Load(); n != 2 { + t.Errorf("connections=%d, want 2: one redial when the first connection's stream IDs ran out", n) + } + if srv.clientClosed.Load() < 1 { + t.Error("the client never closed the connection whose stream IDs ran out") + } +} + +// uploadWindow is the SETTINGS_INITIAL_WINDOW_SIZE of the servers below: a +// request body of 2,000 bytes sends 1,000 and then waits for window that the +// server never grants. +var uploadWindow = []http2.Setting{{ID: http2.SettingInitialWindowSize, Val: 1000}} + +// sendUpload hands one POST with a 2,000-byte body to the client's first +// connection, as DoRequest does, with a response channel the test owns. +func sendUpload(t *testing.T, srv *rawH2Server) (*h2Conn, chan h2Response) { + t.Helper() + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/upload", testH2Cfg("POST", nil, make([]byte, 2000), 1, 10)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(cl.Close) + hc := cl.conns[0].cur.Load() + ch := make(chan h2Response, 1) + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: cl.headerBlock, data: cl.dataPayload, hasBody: true, respCh: &ch} + return hc, ch +} + +// TestH2WriteErrorWhileBodyWaitsForWindow: a POST body waits for send window, +// and the flush it makes while waiting is the connection's first write to +// fail. The server stays silent and keeps the connection open, so no read +// fails. The failed flush must end the connection and answer the stream, +// once, as a failed write anywhere else does (#89). On 2930ded the wait +// discarded the flush error and polled for window until the run ended, with +// every worker of the connection stuck behind it. +func TestH2WriteErrorWhileBodyWaitsForWindow(t *testing.T) { + dial := dialTimeoutFunc + t.Cleanup(func() { dialTimeoutFunc = dial }) + dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { + c, err := dial(network, addr, timeout) + if err != nil { + return nil, err + } + return &failWritesConn{Conn: c, okWrites: 3}, nil // the handshake's 3 writes succeed, every later one fails + } + srv := startRawH2With(t, rawH2Opts{settings: uploadWindow}, func(*rawH2Conn, uint32, string) rawH2Action { + return rawKeep // never answer, never grant window + }) + hc, ch := sendUpload(t, srv) + + select { + case <-hc.done: + case <-time.After(3 * time.Second): + t.Fatal("the flush made while the body waited for window failed, and the client never ended the connection: its writer still polls for window on a connection it cannot write to") + } + var first h2Response + select { + case first = <-ch: + case <-time.After(2 * time.Second): + t.Fatal("the stream whose body could not be written was never answered") + } + if first.err == nil { + t.Fatalf("the stream whose body could not be written was answered with success: %+v", first) + } + select { + case second := <-ch: + t.Errorf("the stream was answered twice: %v, then %v", first.err, second.err) + case <-time.After(200 * time.Millisecond): + } + t.Logf("answered once: %v", first.err) +} + +// TestH2WindowCreditSentWhileBodyWaitsForWindow: while a POST body waits for +// send window, the client must still return receive window to the server. +// The server answers the POST at once with 10,000 bytes of a body it does not +// end, and never grants window for the upload. The client must credit those +// bytes to the connection window although its writer is parked in the +// flow-control wait: a server that waits for that credit before it reads or +// grants more deadlocks with the client otherwise. On 2930ded the credit was +// sent only once the wait ended, which it never did. +func TestH2WindowCreditSentWhileBodyWaitsForWindow(t *testing.T) { + const respBody = 10000 + srv := startRawH2With(t, rawH2Opts{settings: uploadWindow}, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.headers(sid, false, ":status", "200") + c.data(sid, false, respBody, 0) + return rawKeep // never end the response, never grant window + }) + sendUpload(t, srv) + + deadline := time.Now().Add(2 * time.Second) + for srv.windowCredit.Load() < respBody && time.Now().Before(deadline) { + time.Sleep(2 * time.Millisecond) + } + t.Logf("response bytes received by the client=%d, connection window credited=%d", srv.dataSent.Load(), srv.windowCredit.Load()) + if got := srv.windowCredit.Load(); got < respBody { + t.Errorf("the client credited %d of the %d response bytes it received while its request body waited for window: the writer must send WINDOW_UPDATE while it waits", got, respBody) + } +} + +// TestH2RedialBacksOffOnlyWhenTheServerLooksDown: h1client redials a +// connection the server ended at once, and backs off only when the redial +// fails (h1client.go DoRequest). HTTP/2 must record a redial the same way. A +// connection that answered a request and then ended is redialed without the +// backoff sleep, which would otherwise be charged to the latency of the +// request that redials; a connection that ended before it answered anything +// is paced, so a server that accepts and drops every connection is not +// redialed in a loop. The backoff is set far beyond each context, so a sleep +// shows as a failed redial. +func TestH2RedialBacksOffOnlyWhenTheServerLooksDown(t *testing.T) { + srv := startRawH2(t, respondOK) + host, port := srv.hostPort() + newClient := func(t *testing.T) *h2Client { + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 1)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(cl.Close) + return cl + } + const longBackoff = 20 * time.Second // sleep() jitters it to 10-20 s + + t.Run("answered", func(t *testing.T) { + cl := newClient(t) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + if _, err := cl.DoRequest(ctx, 0); err != nil { + t.Fatal(err) + } + slot := cl.conns[0] + slot.cur.Load().failConn(errors.New("test: the server ended the connection")) + slot.backoff.next = longBackoff + start := time.Now() + hc := cl.reconnectSlot(ctx, slot) + t.Logf("redial after a connection that answered: %v, connection=%t", time.Since(start), hc != nil) + if hc == nil { + t.Fatalf("no connection after %v: the redial slept the backoff although the connection it replaces had answered; h1client redials such a connection at once", + time.Since(start)) + } + }) + t.Run("never answered", func(t *testing.T) { + cl := newClient(t) + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + slot := cl.conns[0] + slot.cur.Load().failConn(errors.New("test: the server dropped the connection before answering")) + slot.backoff.next = longBackoff + start := time.Now() + hc := cl.reconnectSlot(ctx, slot) + t.Logf("redial after a connection that never answered: %v, connection=%t", time.Since(start), hc != nil) + if hc != nil { + t.Fatal("a connection that never answered was redialed without the backoff: a server that drops every connection would be redialed in a loop") + } + }) + // Guard: with no sleep in the way, the redial must still not outlive + // the run. Benchmarker.Run cancels the context before it calls Close, so + // a connection dialed after that would be one Close never sees. + t.Run("run over", func(t *testing.T) { + cl := newClient(t) + ctx, cancel := context.WithCancel(context.Background()) + if _, err := cl.DoRequest(ctx, 0); err != nil { + t.Fatal(err) + } + slot := cl.conns[0] + slot.cur.Load().failConn(errors.New("test: the server ended the connection")) + cancel() + if hc := cl.reconnectSlot(ctx, slot); hc != nil { + t.Fatal("a connection was dialed after the run's context ended: Close, which runs after the cancel, never closes it") + } + }) +} + +// TestH2ResponseWithoutAReadableStatusIsNotASuccess: a response counts as a +// success only when its :status says so, as an HTTP/1.1 response does only +// with a status line (h1client: "short status line" is an error). A :status +// sent as a literal with a literal name (RFC 7541 §6.2, name index 0) is a +// status like any other; a response with no :status, or a HEADERS or DATA +// frame whose pad length does not fit its payload (RFC 9113 §6.1, §6.2: a +// connection error), is an error. On 2930ded each of these was a success. +func TestH2ResponseWithoutAReadableStatusIsNotASuccess(t *testing.T) { + literalName := func(first byte, name []byte, huffman bool, value string) []byte { + n := byte(len(name)) + if huffman { + n |= 0x80 + } + b := append([]byte{first, n}, name...) + return append(append(b, byte(len(value))), value...) + } + plain, huff := []byte(":status"), hpack.AppendHuffmanString(nil, ":status") + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, path string) rawH2Action { + endAll := http2.FlagHeadersEndHeaders | http2.FlagHeadersEndStream + switch path { + case "/literal-name-404": // without indexing, literal name + _ = c.fr.WriteRawFrame(http2.FrameHeaders, endAll, sid, literalName(0x00, plain, false, "404")) + case "/literal-name-huffman-503": // never indexed, Huffman-coded literal name + _ = c.fr.WriteRawFrame(http2.FrameHeaders, endAll, sid, literalName(0x10, huff, true, "503")) + case "/literal-name-indexed-502": // incremental indexing, literal name + _ = c.fr.WriteRawFrame(http2.FrameHeaders, endAll, sid, literalName(0x40, plain, false, "502")) + case "/no-status": // a field block without :status + c.headers(sid, true, "content-type", "text/plain") + case "/headers-bad-padding": // Pad Length 200 in a 2-byte payload, then :status 200 + _ = c.fr.WriteRawFrame(http2.FrameHeaders, endAll|http2.FlagHeadersPadded, sid, []byte{200, 0x88}) + case "/data-bad-padding": // :status 200, then DATA whose Pad Length 50 exceeds its 11-byte payload + c.headers(sid, false, ":status", "200") + _ = c.fr.WriteRawFrame(http2.FrameData, http2.FlagDataEndStream|http2.FlagDataPadded, sid, append([]byte{50}, make([]byte, 10)...)) + default: + c.ok(sid) + } + return rawKeep + }) + host, port := srv.hostPort() + + for _, tc := range []struct { + path string + status int // 0: a success; -1: an error other than a status + }{ + {"/literal-name-404", 404}, + {"/literal-name-huffman-503", 503}, + {"/literal-name-indexed-502", 502}, + {"/no-status", -1}, + {"/headers-bad-padding", -1}, + {"/data-bad-padding", -1}, + {"/ok", 0}, + } { + n, err := h2Once(t, host, port, tc.path) + t.Logf("%-26s status=%d bytes=%d err=%v", tc.path, h2Status(err), n, err) + if got := h2Status(err); got != tc.status { + t.Errorf("%s: recorded status %d (err=%v), want %d (0: success, -1: an error): a response is a success only when its :status says so", + tc.path, got, err, tc.status) + } + } +} From b34dc8517683f471b3dbf6f75de6cafdd7375fa0 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:13:44 +0200 Subject: [PATCH 08/31] fix(h2client): the body flow-control wait returns window credit and stops on a failed flush (#89) A request body that waits for send window flushes and polls in writeBodyFlowControlled. That flush dropped its error, so a connection whose writes failed while its reads did not was never ended: the writer polled for window until the run ended, and every worker of the connection waited behind it. This is the case writeFailed covers for writeLoop's own flushes (#89); the wait now returns the error, and processWriteReq answers the stream once and ends the connection through writeFailed. The same wait never sent the receive window readLoop credits meanwhile (flushWindowUpdate runs only between writeLoop's requests), so a peer that waits for that credit before it grants ours deadlocked with the wait. The wait now flushes it with the body, as writeLoop's flushes do. TestH2WriteErrorWhileBodyWaitsForWindow and TestH2WindowCreditSentWhileBodyWaitsForWindow pass. --- h2client.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/h2client.go b/h2client.go index 392c9e3..a937453 100644 --- a/h2client.go +++ b/h2client.go @@ -1070,9 +1070,9 @@ func (hc *h2Conn) finish(chPtr *chan h2Response, resp h2Response) (int, error) { // hc.curStreamID; readLoop replenishes connSendWindow/curStreamWindow on // WINDOW_UPDATE. When a window is exhausted we flush the buffered frames (so the // server can consume + replenish) and poll — no extra goroutine/channel. The -// wait ends when the connection is closed (done), which Close does at the end -// of a run and failConn when the connection dies: that is the backstop -// against a peer that never grants window. +// wait ends when that flush fails, or when the connection is closed (done), +// which Close does at the end of a run and failConn when the connection dies: +// that is the backstop against a peer that never grants window. // // Without this, a 64KiB body (post-64k-h2 = 65536 B) either trips FRAME_SIZE_ERROR // (64KiB DATA frame vs a 16384-default server) or overruns the 65535 window. @@ -1095,7 +1095,15 @@ func (hc *h2Conn) writeBodyFlowControlled(streamID uint32, data []byte) error { if avail <= 0 { // Window exhausted: flush so the peer receives what we've sent and // can send WINDOW_UPDATE, then wait for readLoop to replenish. - _ = hc.bufWriter.Flush() + // The flush carries the receive window readLoop has credited + // meanwhile, as writeLoop's own flushes do: a peer that waits for + // that credit before it grants ours would otherwise deadlock with + // this wait. A failed flush ends the wait and, through the + // caller's writeFailed, the connection (#89). + hc.flushWindowUpdate() + if err := hc.bufWriter.Flush(); err != nil { + return err + } select { case <-hc.done: return fmt.Errorf("h2client: conn closed mid-body (flow-control wait)") From ec23f41ce4d39a737a557dbe75794153e9fa5e68 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:14:15 +0200 Subject: [PATCH 09/31] fix(h2client): a connection out of stream IDs is redialed, not failed request by request (#88) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the lifetime deadline gone (#88), a connection lives as long as the server keeps it, so a long run can use up its 2^30 client stream IDs: at 16 connections and about 1M requests/s, after roughly 4.8 hours. From then on processWriteReq answered every request with "stream ID exhausted" and left the connection open and not closed, so DoRequest never redialed it: every worker of the slot spun errors, the failure #89 fixes for a dead connection. RFC 9113 §5.1.1: a client that cannot open a new stream opens a new connection. The connection is now marked closed, so DoRequest redials it (and reconnectSlot closes it), and the request, which never reached the server, goes to the new connection as errH2NotSent. Streams still in flight on the old connection at that moment (at most MaxStreams - 1, once per 2^30 requests) end when it is closed, as a GOAWAY's in-flight streams do until #92. TestH2StreamIDExhaustionRedials passes: 8 requests, 3 on the first connection's last IDs, 0 errors, 2 connections, the first one closed. --- h2client.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/h2client.go b/h2client.go index a937453..e92ab63 100644 --- a/h2client.go +++ b/h2client.go @@ -776,8 +776,16 @@ func (hc *h2Conn) processWriteReq(req h2WriteReq) { case h2WriteHeaders: streamID := hc.nextStreamID.Add(2) - 2 if streamID > 0x7FFFFFFF { + // The connection's stream IDs are used up, which a connection + // that lives as long as the server keeps it (#88) reaches after + // 2^30 requests. RFC 9113 §5.1.1: open a new connection. Marked + // closed, it takes no new request, and DoRequest redials it and + // closes it; this request never reached the server, so it goes + // to the new connection (errH2NotSent), as does every request + // still queued here. + hc.closed.Store(true) if req.respCh != nil { - *req.respCh <- h2Response{err: fmt.Errorf("h2client: stream ID exhausted")} + *req.respCh <- h2Response{err: errH2NotSent} } return } @@ -927,8 +935,9 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { return st } -// errH2NotSent is roundTrip's report that the connection died before the -// request was handed to it. Nothing reached the server, so it is not a +// errH2NotSent reports a request that never reached the server: the +// connection died before the request was handed to it (roundTrip), or ran +// out of stream IDs before it was written (processWriteReq). It is not a // failed request: DoRequest takes the request to the redialed connection. // Never returned to DoRequest's caller. var errH2NotSent = errors.New("h2client: connection gone before the request was sent") From cda3d4b95dedb4522a5b215b19a600958b5d997d Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:15:29 +0200 Subject: [PATCH 10/31] fix(h2client): redial a connection that served at once, as h1client does (#89) reconnectSlot slept the slot's backoff (5-10 ms on the first call) before every redial. h1client redials a connection the server ended at once and sleeps only after a failed redial, inside a request it counts as an error (h1client.go DoRequest). With #89 an HTTP/2 connection is redialed after every death, not only after a GOAWAY, so the difference reached more requests: the sleep, like the dial and handshake, lands in the latency of the request that redials, which the Benchmarker records. The backoff now applies only while the server looks down: the last redial failed, or the connection being replaced ended before it carried a single response (readLoop marks it served), so a server that accepts and drops every connection is still not hot-redialled. A connection that served and then ended (closed, GOAWAY, stream IDs used up) is redialed at once. The dial and handshake stay in the latency of the request that redials, as a write-failure retry's reconnect does in h1client. reconnectSlot also checks ctx before it dials: sleep can report its timer when ctx is done too (CodeRabbit, PR #91), and a served connection skips the sleep altogether. TestH2RedialBacksOffOnlyWhenTheServerLooksDown passes: 293 us after a connection that answered, backoff kept (context expired, no dial) after one that never did. --- h2client.go | 55 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/h2client.go b/h2client.go index e92ab63..3c9caf6 100644 --- a/h2client.go +++ b/h2client.go @@ -100,7 +100,7 @@ type h2Client struct { // would otherwise strand the slot dead and the worker would spin // closed-conn errors with no recovery (fastapi-h2 logged ~1.1B errors / // 0 requests from exactly this). reconnectSlot calls it under the slot - // lock, paced by the slot's backoff. + // lock, paced by the slot's backoff while the server looks down. redial func() (*h2Conn, error) // dialedViaUpgrade reports whether this client's connections were @@ -204,6 +204,10 @@ type h2Conn struct { // closed marks a connection that takes no new request: DoRequest // redials the slot instead. Set before done is closed. closed atomic.Bool + // served is set by readLoop once the connection carries a response + // frame. reconnectSlot backs off before it replaces a connection that + // never did: the server is down or drops every connection. + served atomic.Bool } // h2StreamSlot holds an atomic pointer to a response channel. @@ -422,23 +426,37 @@ func newH2ClientWithDialer(host, port, path string, cfg Config, upgrade bool) (* } // reconnectSlot re-establishes a dead slot's connection, single-flighted under -// the slot lock and paced by the slot's backoff. Returns the live conn, or nil -// if the redial failed (caller surfaces a connect error) or ctx ended. A -// concurrent caller that finds the slot already healed returns immediately. +// the slot lock and, while the server looks down, paced by the slot's +// backoff. Returns the live conn, or nil if the redial failed (caller +// surfaces a connect error) or ctx ended. A concurrent caller that finds the +// slot already healed returns immediately. func (c *h2Client) reconnectSlot(ctx context.Context, slot *h2ConnSlot) *h2Conn { slot.mu.Lock() defer slot.mu.Unlock() - if hc := slot.cur.Load(); hc != nil && !hc.closed.Load() { - return hc // another worker already re-dialed this slot + old := slot.cur.Load() + if old != nil && !old.closed.Load() { + return old // another worker already re-dialed this slot } - if old := slot.cur.Load(); old != nil { + if old != nil { old.closeConn() // release fds/goroutines of the dead conn } - // Pace retries so a server that stays down can't be hot-redialled. - if !slot.backoff.sleep(ctx, nil) { - return nil // ctx cancelled mid-backoff + // Back off only when the server looks down, as h1client does: the last + // redial failed (the slot is empty), or the connection ended before it + // carried a single response, so a server that accepts and drops every + // connection cannot be hot-redialled. A connection that served and then + // ended (the server closed it or sent GOAWAY, or its stream IDs ran out) + // is redialed at once: the sleep would be charged to the latency of the + // request that redials, where h1client sleeps only after a failed redial, + // inside a request it counts as an error. + if old == nil || !old.served.Load() { + if !slot.backoff.sleep(ctx, nil) { + return nil // ctx cancelled mid-backoff + } + } + if ctx.Err() != nil { + return nil // the run is over (sleep may report its timer although ctx is done too) } hc, err := c.redial() if err != nil { @@ -923,7 +941,8 @@ func (hc *h2Conn) readLoop() { } // stream returns the slot of streamID with its response state reset if the -// slot last held another stream. readLoop only. +// slot last held another stream, and marks the connection served. readLoop +// only. func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { st := &hc.streamSlots[(streamID>>1)%uint32(len(hc.streamSlots))] if st.streamID != streamID { @@ -931,6 +950,9 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { st.status = 0 st.final = false st.bytes = 0 + if !hc.served.Load() { + hc.served.Store(true) + } } return st } @@ -969,11 +991,12 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { if err != errH2NotSent { // the sentinel itself, never wrapped return n, err } - // The connection died while this request waited for a stream. Each - // pass through here needs a connection to have died, and every - // connection is dialed through reconnectSlot's backoff, so a server - // that keeps closing connections cannot make this loop spin; ctx - // ends it. + // The connection died while this request waited for a stream, or + // ran out of stream IDs before writing it. Each pass through here + // needs a connection to have ended, and reconnectSlot redials without + // its backoff only a connection that carried a response, so a server + // that keeps closing connections cannot make this loop spin: either + // it serves on each connection, or each redial is paced. ctx ends it. } } From 260e24d4c14e5c09b0a23b57f6f9c3725314e2f7 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:16:28 +0200 Subject: [PATCH 11/31] fix(h2client): a response is a success only when its :status says so (#85) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fallbacks still turned a response into a silent success: - A :status sent as a literal with a literal name (RFC 7541 §6.2, name index 0) was not decoded: status 0. Legal HPACK, if unusual for a name the static table holds. extractStatus now reads the name, plain or Huffman-coded, and decodes the value as for a static :status name. - A response whose :status could not be read at all (status 0, or a value that is not a status) passed DoRequest's status >= 400 test as a success. It is now an error (errH2NoStatus), as h1client fails a response without a status line. - A HEADERS or DATA frame whose Pad Length did not fit its payload was read as an empty block or empty data: a success with status 0, or a success with its bytes dropped. RFC 9113 §6.1 and §6.2 make it a connection error of type PROTOCOL_ERROR; the connection now ends through failConn, as a dead one does. TestH2ResponseWithoutAReadableStatusIsNotASuccess passes; the #85 status tests still pass. --- h2client.go | 51 ++++++++++++++++++++++++++++++++++++++++++++++++--- h2framer.go | 8 ++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/h2client.go b/h2client.go index 3c9caf6..700389b 100644 --- a/h2client.go +++ b/h2client.go @@ -247,7 +247,8 @@ var h2StaticStatus = [7]int{200, 204, 206, 304, 400, 404, 500} // extractStatus extracts the HTTP status code from an HPACK-encoded header block // without allocating. Handles indexed representations from the HPACK static table // (covers >99% of benchmark responses) and falls back to literal parsing. -// Returns 0 for unparseable blocks (treated as success in benchmarks). +// Returns 0 for a block without a :status it can read, which DoRequest counts +// as an error (errH2NoStatus), never as a success. func extractStatus(headerBlock []byte) int { if len(headerBlock) == 0 { return 0 @@ -304,7 +305,30 @@ func extractStatus(headerBlock []byte) int { return 0 } - if nameIdx < 8 || nameIdx > 14 || pos >= len(headerBlock) { + if nameIdx < 8 || nameIdx > 14 { + // Name index 0: the name is a string literal (RFC 7541 §6.2), legal + // if unusual for a name the static table holds. Accept ":status", + // plain or Huffman-coded, and go on as for a static :status name. + if nameIdx != 0 || pos >= len(headerBlock) { + return 0 + } + nameByte := headerBlock[pos] + pos++ + nameLen := int(nameByte & 0x7F) + if pos+nameLen > len(headerBlock) { + return 0 + } + name := headerBlock[pos : pos+nameLen] + pos += nameLen + if nameByte&0x80 != 0 { + if string(name) != h2HuffmanStatusName { + return 0 + } + } else if string(name) != ":status" { + return 0 + } + } + if pos >= len(headerBlock) { return 0 } @@ -331,6 +355,9 @@ func extractStatus(headerBlock []byte) int { return parseStatusCode(value) } +// h2HuffmanStatusName is the HPACK Huffman encoding of the name ":status". +var h2HuffmanStatusName = string(hpack.AppendHuffmanString(nil, ":status")) + // h2HuffmanStatus maps the HPACK Huffman encoding of every status code // 100-599 to the code, so extractStatus decodes a Huffman-coded :status with // one map lookup (string(b) in a map index does not allocate). @@ -855,6 +882,15 @@ func (hc *h2Conn) readLoop() { return } + // A HEADERS or DATA frame whose padding does not fit it is a + // connection error (RFC 9113 §6.1, §6.2): nothing in it can be read, + // so the connection ends as a dead one does. + if frame.badPadding() { + hc.failConn(fmt.Errorf("h2client: PROTOCOL_ERROR: frame type %d on stream %d: its Pad Length does not fit its %d-byte payload", + frame.Type, frame.StreamID, frame.Length)) + return + } + switch frame.Type { case frameHeaders: // The status is the final response's :status, whatever ends the @@ -957,6 +993,11 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { return st } +// errH2NoStatus is DoRequest's error for a response without a :status the +// client can read: no :status at all, or none that extractStatus decodes. It +// is not a success, as h1client fails a response without a status line. +var errH2NoStatus = errors.New("h2client: response without a readable :status") + // errH2NotSent reports a request that never reached the server: the // connection died before the request was handed to it (roundTrip), or ran // out of stream IDs before it was written (processWriteReq). It is not a @@ -1082,13 +1123,17 @@ func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (i } // finish returns the stream's channel and token and turns its answer into -// DoRequest's result: a status >= 400 is an error, as h1client counts it. +// DoRequest's result: a status >= 400 is an error, as h1client counts it, and +// so is a response whose :status could not be read. func (hc *h2Conn) finish(chPtr *chan h2Response, resp h2Response) (int, error) { hc.chanPool.Put(chPtr) hc.streamSem <- struct{}{} if resp.err != nil { return 0, resp.err } + if resp.status < 100 { + return resp.bytesRead, errH2NoStatus + } if resp.status >= 400 { return resp.bytesRead, statusError(resp.status) } diff --git a/h2framer.go b/h2framer.go index 091085e..d463637 100644 --- a/h2framer.go +++ b/h2framer.go @@ -80,6 +80,14 @@ func (f *h2Frame) unpadded() []byte { return p[1 : len(p)-int(p[0])] } +// badPadding reports a DATA or HEADERS frame whose Pad Length does not fit +// its payload: a connection error of type PROTOCOL_ERROR (RFC 9113 §6.1, +// §6.2). Nothing in such a frame can be read (unpadded returns nil). +func (f *h2Frame) badPadding() bool { + return (f.Type == frameData || f.Type == frameHeaders) && f.Flags&flagPadded != 0 && + (len(f.payload) == 0 || int(f.payload[0]) >= len(f.payload)) +} + // Data returns the data of a DATA frame, without its padding. Flow control // counts the whole payload, padding included (RFC 9113 §6.9.1): use Length. func (f *h2Frame) Data() []byte { return f.unpadded() } From 5893503606239245238ab38d0b6bdc781077d828 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:44:52 +0200 Subject: [PATCH 12/31] test(h2client): failing test for a stream whose slot a later stream takes A connection keeps each stream's response channel in slot (streamID/2) mod 2*MaxStreams. A stream still waiting when 2*MaxStreams-1 later streams have been opened shares its slot with the next one, which overwrites its channel: the held stream's response goes to the other request, and the held stream's worker waits for the rest of the run. TestH2StreamOutlivedByLaterStreamsGetsItsResponse holds the first of 2 streams while a second worker sends 4 more; the server then answers the 5th stream and the held one. On this commit (and on main a89f02d) the held stream's worker times out after 3 s with the server having answered it. Pre-existing, found in round 1 of PR #91's review: BenchmarkH2CUpgradeRequest, which has no deadline, hung for 11 minutes on a loaded machine with one worker in await on a live connection and no request pending on the server. --- h2client_accounting_test.go | 63 +++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index d89901a..c224600 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -1348,3 +1348,66 @@ func TestH2ResponseWithoutAReadableStatusIsNotASuccess(t *testing.T) { } } } + +// TestH2StreamOutlivedByLaterStreamsGetsItsResponse: a connection keeps each +// stream's response channel in slot (streamID/2) mod 2*MaxStreams, so a +// stream still waiting when 2*MaxStreams-1 later streams have been opened +// shares its slot with the next one. With 2 streams (4 slots), the server +// holds the first stream while a second worker sends 4 more; the 5th stream +// on the connection would take the held stream's slot. The server then +// answers both. Each worker must get its own response. On main (a89f02d) +// and on 2930ded the 5th stream overwrote the held stream's channel: its +// response went to the other worker, and the held stream's worker waited +// for the rest of the run. (Found in round 1, when an h2c benchmark with no +// deadline hung for 11 minutes on a loaded machine.) +func TestH2StreamOutlivedByLaterStreamsGetsItsResponse(t *testing.T) { + firstSeen := make(chan struct{}) + var held uint32 + srv := startRawH2With(t, rawH2Opts{settings: []http2.Setting{{ID: http2.SettingMaxConcurrentStreams, Val: 2}}}, + func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + switch c.requests { + case 1: // hold the first stream + held = sid + close(firstSeen) + case 5: // the stream after 3 more: it would reuse the held stream's slot + c.ok(sid) + c.ok(held) + default: + c.ok(sid) + } + return rawKeep + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 2)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + type result struct { + n int + err error + } + first := make(chan result, 1) + go func() { + n, err := cl.DoRequest(ctx, 0) + first <- result{n, err} + }() + select { + case <-firstSeen: + case <-time.After(3 * time.Second): + t.Fatal("the first request never reached the server") + } + for i := range 4 { + if n, err := cl.DoRequest(ctx, 0); err != nil || n != len(rawOKBody) { + t.Fatalf("second worker, request %d: bytes=%d err=%v", i+1, n, err) + } + } + r := <-first + t.Logf("held stream: bytes=%d err=%v (server streams: held=%d)", r.n, r.err, held) + if r.err != nil || r.n != len(rawOKBody) { + t.Errorf("the server answered the held stream, but its worker got bytes=%d err=%v: a later stream took its slot and its response", r.n, r.err) + } +} From b659f70da29bb61129939e8bd1e2c31a296e2ed7 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:45:27 +0200 Subject: [PATCH 13/31] fix(h2client): never give a new stream the slot of one still waiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A connection keeps each stream's response channel in slot (streamID/2) mod 2*MaxStreams, and writeLoop took the next stream ID whatever its slot held. The token semaphore bounds how many streams wait at once, not how long one waits: a stream still waiting when 2*MaxStreams-1 later streams have been opened (a slow response under load, or a stream a worker abandoned) had its channel overwritten by the next stream in its slot. Its response then went to that other request, whose own response was dropped, and its worker waited on a live connection for the rest of the run: stranded, and never counted. BenchmarkH2CUpgradeRequest, which has no deadline, hung this way for 11 minutes on a loaded machine. writeLoop now skips a stream ID whose slot is still held. IDs may have gaps: a new stream's ID need only exceed every earlier one (RFC 9113 §5.1.1). At most MaxStreams slots are held by streams with a token, so a free slot comes within MaxStreams+1 IDs; the search is bounded by the slot count so a slot array held throughout by abandoned streams cannot loop. Pre-existing on main (a89f02d). TestH2StreamOutlivedByLaterStreamsGetsItsResponse passes: the held stream gets its own 2-byte response. --- h2client.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/h2client.go b/h2client.go index 700389b..7044d5d 100644 --- a/h2client.go +++ b/h2client.go @@ -157,10 +157,11 @@ type h2Conn struct { writeCh chan h2WriteReq // Stream management — lock-free fixed-size slot array. - // Index = (streamID >> 1) % len(streamSlots). The semaphore limits - // concurrent streams, and stream IDs are assigned sequentially by - // writeLoop, so slots cycle predictably with no collisions. - // Sized at 2x effectiveStreams for headroom against wrap-around. + // Index = (streamID >> 1) % len(streamSlots), sized at 2x + // effectiveStreams. The semaphore limits concurrent streams, but a slow + // or abandoned stream can still hold its slot when the IDs come round + // to it again, so writeLoop skips an ID whose slot is held: no two + // pending streams share a slot. nextStreamID atomic.Uint32 streamSlots []h2StreamSlot @@ -819,7 +820,23 @@ func (hc *h2Conn) flushWindowUpdate() { func (hc *h2Conn) processWriteReq(req h2WriteReq) { switch req.kind { case h2WriteHeaders: + // Take the next stream ID whose slot is free. A slot still holds the + // channel of a stream that 2*MaxStreams-1 later streams have outlived + // (a slow response, or a request its worker abandoned); reusing it + // would hand that stream's response to this request and leave its + // worker waiting for the rest of the run. IDs may skip: a new + // stream's ID need only exceed every earlier one (RFC 9113 §5.1.1). + // At most MaxStreams slots are held by streams with a token, so a + // free slot comes within MaxStreams+1 IDs; the bound keeps a slot + // array held throughout (abandoned streams) from looping. + numSlots := uint32(len(hc.streamSlots)) streamID := hc.nextStreamID.Add(2) - 2 + for range numSlots { + if hc.streamSlots[(streamID>>1)%numSlots].ch.Load() == nil { + break + } + streamID = hc.nextStreamID.Add(2) - 2 + } if streamID > 0x7FFFFFFF { // The connection's stream IDs are used up, which a connection // that lives as long as the server keeps it (#88) reaches after @@ -835,7 +852,7 @@ func (hc *h2Conn) processWriteReq(req h2WriteReq) { return } - slotIdx := (streamID >> 1) % uint32(len(hc.streamSlots)) + slotIdx := (streamID >> 1) % numSlots hc.streamSlots[slotIdx].ch.Store(req.respCh) err := hc.framer.WriteHeaders(streamID, req.block, !req.hasBody) From 5f519b50eeb35c81e5258ca6ef80deb308cfebc7 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 03:12:46 +0200 Subject: [PATCH 14/31] test(h2client): failing test for requests the connection never wrote TestH2RequestsTheConnectionNeverWroteAreRetried: 4 workers share one connection with 4 streams and POST 100 KiB bodies; the server reads the 65,535 bytes the window allows and closes the connection. One request's body was being written (in flight, one error); the other three were still queued for the writer and never reached the server, which PR #91's own rule says is not an error. On this commit: 166 errors over 42 connections, 3.95 per connection. Review nit N2 of PR #91 (round 1); N1 is the same wait seen from the response side. --- h2client_accounting_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index c224600..1ecb433 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -1411,3 +1411,31 @@ func TestH2StreamOutlivedByLaterStreamsGetsItsResponse(t *testing.T) { t.Errorf("the server answered the held stream, but its worker got bytes=%d err=%v: a later stream took its slot and its response", r.n, r.err) } } + +// TestH2RequestsTheConnectionNeverWroteAreRetried: 4 workers share one +// connection with 4 streams and POST 100 KiB bodies; the server reads the +// 65,535 bytes the connection window allows and closes the connection. The +// first request's body was being written, so it is in flight and is one +// error. The other three were still queued for the writer: they never +// reached the server, so, like a request still waiting for a stream, they go +// to the redialed connection and are not errors. Before the fix their workers +// woke on the closed connection and gave up before the writer answered, and +// the writer failed the ones it reached: 4 errors per connection. +func TestH2RequestsTheConnectionNeverWroteAreRetried(t *testing.T) { + srv := startRawH2With(t, rawH2Opts{closeAfterData: 65535}, func(*rawH2Conn, uint32, string) rawH2Action { + return rawKeep // take the request and its body, never answer + }) + const streams = 4 + res := runBench(t, Config{URL: srv.url("/upload"), Method: "POST", Body: make([]byte, 100<<10), Duration: 400 * time.Millisecond, + Workers: streams, HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: streams}}) + killed := srv.killed.Load() + t.Logf("connections=%d closed by the server=%d requests=%d errors=%d errors per closed connection=%.2f", + srv.accepted.Load(), killed, res.Requests, res.Errors, float64(res.Errors)/float64(max(killed, 1))) + if killed < 3 { + t.Fatalf("the server closed %d connection(s): the client did not keep redialing", killed) + } + if res.Errors > killed { + t.Errorf("errors=%d over %d connections the server closed with one body in flight each: requests the connection never wrote were charged as errors", + res.Errors, killed) + } +} From dbf32fd275303449801482dffab1ad4b0f478e0a Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 03:15:14 +0200 Subject: [PATCH 15/31] fix(h2client): a request the connection never wrote is retried, and a late answer is taken (#89) PR #91's rule: a request that never reached the server is not an error and goes to the redialed connection. It held for a request still waiting for a stream, but not for one already queued for the connection's writer when the connection died. Its worker woke on the closed connection and returned "connection closing" at once, before the writer reached the request; and the writer, reaching it, wrote it into a dead connection or failed it. With 4 streams and one body in flight, 3.95 errors per dead connection where 1 is right. - processWriteReq writes no request once the connection takes none (died, closed, stream IDs used up): it answers errH2NotSent. The writer's final drain (answerQueued) does the same for the requests still queued when it stops. - await, on a closed connection, no longer decides at once. It waits for its answer, or until readLoop and writeLoop have both returned (loopsDone), which they do soon after done closes, having sent every answer they will send. Only a request neither loop answered is "connection closing". The same wait closes the window where readLoop had taken a response before writeLoop failed the connection and the worker gave up before the response arrived (review nit N1): the response is now the request's outcome. - The wait also ends on ctx, so shutdown is not held by it. TestH2RequestsTheConnectionNeverWroteAreRetried passes: 1.00 error per dead connection (44 of 44). TestH2ClosedConnectionWritesNoNewRequest and TestH2AwaitTakesAnAnswerThatArrivesAfterTheClose pin each half, and TestH2ConnectionLoopsEndAfterClose that a closed connection's loops do return. TestH2ResponseWinsOverConnectionClose now builds its closed connection with loopsDone closed too, the state await meets once both loops have returned. --- h2client.go | 117 +++++++++++++++++++++++++--------- h2client_accounting_test.go | 121 ++++++++++++++++++++++++++++++++++-- 2 files changed, 203 insertions(+), 35 deletions(-) diff --git a/h2client.go b/h2client.go index 7044d5d..e97fabf 100644 --- a/h2client.go +++ b/h2client.go @@ -201,6 +201,14 @@ type h2Conn struct { done chan struct{} closeOnce sync.Once + // loopsDone is closed once readLoop and writeLoop have both returned + // (loopsLeft counts them down), which they do soon after done closes: + // closeConn closes the socket too. Each has sent by then every answer it + // will ever send, so await waits for it before it decides that a request + // on a closed connection got no answer. + loopsDone chan struct{} + loopsLeft atomic.Int32 + addr string // closed marks a connection that takes no new request: DoRequest // redials the slot instead. Set before done is closed. @@ -724,6 +732,7 @@ func completeH2Handshake(conn net.Conn, br *bufio.Reader, addr string, maxStream serverInitWindow: serverInitWin, streamSem: make(chan struct{}, effectiveStreams), done: make(chan struct{}), + loopsDone: make(chan struct{}), addr: addr, chanPool: sync.Pool{ New: func() any { @@ -741,6 +750,7 @@ func completeH2Handshake(conn net.Conn, br *bufio.Reader, addr string, maxStream hc.streamSem <- struct{}{} } + hc.loopsLeft.Store(2) go hc.writeLoop() go hc.readLoop() @@ -753,6 +763,7 @@ func completeH2Handshake(conn net.Conn, br *bufio.Reader, addr string, maxStream // It flushes pending connection-level WINDOW_UPDATE (accumulated by readLoop // via atomic counter) both when processing requests AND periodically when idle. func (hc *h2Conn) writeLoop() { + defer hc.loopExited() ticker := time.NewTicker(1 * time.Millisecond) defer ticker.Stop() @@ -785,21 +796,37 @@ func (hc *h2Conn) writeLoop() { hc.writeFailed(hc.bufWriter.Flush()) case <-hc.done: - // Drain pending requests — send errors to respCh so workers unblock. - for { - select { - case req := <-hc.writeCh: - if req.respCh != nil { - *req.respCh <- h2Response{err: fmt.Errorf("connection closing")} - } - default: - return - } + hc.answerQueued() + return + } + } +} + +// answerQueued answers every request still queued for a closed connection's +// writer. None of them was written, so none reached the server: each goes +// to the redialed connection (errH2NotSent), as a request still waiting for +// a stream does, instead of counting as an error. +func (hc *h2Conn) answerQueued() { + for { + select { + case req := <-hc.writeCh: + if req.respCh != nil { + *req.respCh <- h2Response{err: errH2NotSent} } + default: + return } } } +// loopExited is deferred by readLoop and writeLoop: the second to return +// closes loopsDone. +func (hc *h2Conn) loopExited() { + if hc.loopsLeft.Add(-1) == 0 { + close(hc.loopsDone) + } +} + // writeFailed ends the connection after a write to it failed (nil: no-op). // The peer is gone, and readLoop does not always see it: the server may keep // its side open, or its reset may not have reached the read yet. Without @@ -820,6 +847,18 @@ func (hc *h2Conn) flushWindowUpdate() { func (hc *h2Conn) processWriteReq(req h2WriteReq) { switch req.kind { case h2WriteHeaders: + // A connection that takes no new request (it died, the client closed + // it, or its stream IDs ran out) never writes this one: it goes to + // the redialed connection, as a request still waiting for a stream + // does. Written, it would fail on bufio's sticky error, or wait for a + // response nobody reads, and count as an error. + if hc.closed.Load() { + if req.respCh != nil { + *req.respCh <- h2Response{err: errH2NotSent} + } + return + } + // Take the next stream ID whose slot is free. A slot still holds the // channel of a stream that 2*MaxStreams-1 later streams have outlived // (a slow response, or a request its worker abandoned); reusing it @@ -884,6 +923,7 @@ func (hc *h2Conn) processWriteReq(req h2WriteReq) { // is full of worker requests — readLoop blocks, can't read responses, server's // flow control windows exhaust, everything stalls. func (hc *h2Conn) readLoop() { + defer hc.loopExited() numSlots := uint32(len(hc.streamSlots)) for { @@ -1016,10 +1056,11 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { var errH2NoStatus = errors.New("h2client: response without a readable :status") // errH2NotSent reports a request that never reached the server: the -// connection died before the request was handed to it (roundTrip), or ran -// out of stream IDs before it was written (processWriteReq). It is not a -// failed request: DoRequest takes the request to the redialed connection. -// Never returned to DoRequest's caller. +// connection died before the request was handed to it (roundTrip), or took +// no new request (it died, was closed, or ran out of stream IDs) before its +// writer wrote it (processWriteReq, answerQueued). It is not a failed +// request: DoRequest takes the request to the redialed connection. Never +// returned to DoRequest's caller. var errH2NotSent = errors.New("h2client: connection gone before the request was sent") // DoRequest sends an HTTP/2 request and waits for the response. @@ -1049,17 +1090,20 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { if err != errH2NotSent { // the sentinel itself, never wrapped return n, err } - // The connection died while this request waited for a stream, or - // ran out of stream IDs before writing it. Each pass through here - // needs a connection to have ended, and reconnectSlot redials without - // its backoff only a connection that carried a response, so a server - // that keeps closing connections cannot make this loop spin: either - // it serves on each connection, or each redial is paced. ctx ends it. + // The connection ended before this request was written: while it + // waited for a stream, or before the connection's writer reached it + // (the connection died, or ran out of stream IDs). Each pass through + // here needs a connection to have ended, and reconnectSlot redials + // without its backoff only a connection that carried a response, so + // a server that keeps closing connections cannot make this loop + // spin: either it serves on each connection, or each redial is + // paced. ctx ends it. } } // roundTrip sends one request on hc and waits for its response. It returns -// errH2NotSent if hc died before the request was handed to its writer. +// errH2NotSent if hc ended before the request was written: before it was +// handed to the writer, or before the writer reached it. func (c *h2Client) roundTrip(ctx context.Context, hc *h2Conn, idx int) (int, error) { // Acquire a stream. When the connection dies, the workers queued here // wake through done, or through the tokens its failed streams return. @@ -1107,11 +1151,11 @@ func (c *h2Client) roundTrip(ctx context.Context, hc *h2Conn, idx int) (int, err return hc.await(ctx, chPtr, idx) } -// await waits for the answer to a request in flight on hc: a response (from -// readLoop) or an error (from writeLoop, or failStreams). If the connection -// dies, exactly one of failStreams, writeLoop (a write error, or its drain of -// writeCh) and done below answers the request, so it is one error, never -// zero or two. +// await waits for the answer to a request handed to hc: a response (from +// readLoop), an error (from writeLoop, or failStreams), or errH2NotSent (from +// writeLoop, for a request it never wrote). Exactly one of them answers each +// request, and done below only when none did, so a request is one outcome, +// never zero or two. func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (int, error) { select { case resp := <-*chPtr: @@ -1124,11 +1168,22 @@ func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (i hc.streamSem <- struct{}{} return 0, ctx.Err() case <-hc.done: - // readLoop answers a stream before it fails the connection, so a - // response may be waiting here too. A worker parked in this select - // gets it by direct handoff before done closes, but one that reaches - // the select after both finds both ready, and select picks at random: - // a request the server answered must not become an error. + // The connection is closing, and this request's answer may still + // be on its way: readLoop may be delivering a response it took + // before the close, and writeLoop answers every request it never + // wrote with errH2NotSent. Both loops return soon after done closes, + // having sent every answer they will send, so wait for them (or the + // answer), then take the answer if there is one. Deciding at once + // turned both into errors: a response the server sent, and a request + // that never reached it. + select { + case resp := <-*chPtr: + return hc.finish(chPtr, resp) + case <-hc.loopsDone: + case <-ctx.Done(): + hc.streamSem <- struct{}{} + return 0, ctx.Err() + } select { case resp := <-*chPtr: return hc.finish(chPtr, resp) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 1ecb433..58a4077 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -915,12 +915,14 @@ func TestH2ReadErrorMidStreamFailsEachInFlightStreamOnce(t *testing.T) { // TestH2ResponseWinsOverConnectionClose: a worker that reaches its wait // after readLoop has both answered its stream and failed the connection finds -// the response and the closed connection ready at once. The response is the -// request's outcome; select alone would pick at random. (Found in review of -// the #89 fix, which makes a close right after a response common.) +// the response and the closed connection ready at once, and, since round 1, +// both loops already returned (loopsDone). The response is the request's +// outcome; select alone would pick at random. (Found in review of the #89 +// fix, which makes a close right after a response common.) func TestH2ResponseWinsOverConnectionClose(t *testing.T) { - hc := &h2Conn{done: make(chan struct{}), streamSem: make(chan struct{}, 1)} + hc := &h2Conn{done: make(chan struct{}), loopsDone: make(chan struct{}), streamSem: make(chan struct{}, 1)} close(hc.done) + close(hc.loopsDone) for i := range 200 { ch := make(chan h2Response, 1) ch <- h2Response{status: 200, bytesRead: 2} @@ -1439,3 +1441,114 @@ func TestH2RequestsTheConnectionNeverWroteAreRetried(t *testing.T) { res.Errors, killed) } } + +// TestH2ClosedConnectionWritesNoNewRequest: once a connection takes no new +// request, its writer writes none of those it still holds: the one it takes +// next, and the ones still queued when it stops. Each is answered +// errH2NotSent, so DoRequest takes it to the redialed connection. +func TestH2ClosedConnectionWritesNoNewRequest(t *testing.T) { + var wire bytes.Buffer + bw := bufio.NewWriter(&wire) + hc := &h2Conn{bufWriter: bw, framer: newH2Framer(bw, nil), writeCh: make(chan h2WriteReq, 4), streamSlots: make([]h2StreamSlot, 4)} + hc.nextStreamID.Store(1) + hc.closed.Store(true) + + chans := make([]chan h2Response, 4) + for i := range chans { + chans[i] = make(chan h2Response, 1) + } + hc.processWriteReq(h2WriteReq{kind: h2WriteHeaders, block: []byte{0x82}, respCh: &chans[0]}) // the one it takes next + for i := 1; i < len(chans); i++ { + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: []byte{0x82}, respCh: &chans[i]} // still queued + } + hc.answerQueued() + if err := bw.Flush(); err != nil { + t.Fatal(err) + } + for i, ch := range chans { + select { + case r := <-ch: + if r.err != errH2NotSent { + t.Errorf("request %d on a closed connection was answered %+v, want errH2NotSent", i, r) + } + default: + t.Errorf("request %d on a closed connection was never answered", i) + } + } + if wire.Len() != 0 { + t.Errorf("the writer wrote %d bytes for requests on a closed connection, want 0", wire.Len()) + } +} + +// TestH2AwaitTakesAnAnswerThatArrivesAfterTheClose: a worker that sees its +// connection close may still be owed an answer: a response readLoop took +// before the close and is delivering, or writeLoop's errH2NotSent for a +// request it never wrote. await waits until both loops have returned +// (loopsDone), and only a request no loop answered is "connection closing". +// Deciding at once made the response an error (N1) and the unsent request +// an error instead of a retry (N2). +func TestH2AwaitTakesAnAnswerThatArrivesAfterTheClose(t *testing.T) { + for _, tc := range []struct { + name string + answer *h2Response // nil: no loop answers + }{ + {"response in delivery", &h2Response{status: 200, bytesRead: 2}}, + {"request never written", &h2Response{err: errH2NotSent}}, + {"no answer", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + hc := &h2Conn{done: make(chan struct{}), loopsDone: make(chan struct{}), streamSem: make(chan struct{}, 1)} + close(hc.done) + ch := make(chan h2Response, 1) + go func() { + time.Sleep(20 * time.Millisecond) // the loops are still at work when the worker sees done + if tc.answer != nil { + ch <- *tc.answer + } + close(hc.loopsDone) + }() + n, err := hc.await(context.Background(), &ch, 0) + <-hc.streamSem // the token await returned + t.Logf("await returned (%d, %v)", n, err) + switch { + case tc.answer == nil: + if err == nil || errors.Is(err, errH2NotSent) { + t.Errorf("no loop answered, await returned (%d, %v), want a connection-closing error", n, err) + } + case tc.answer.err != nil: + if err != errH2NotSent { + t.Errorf("writeLoop answered errH2NotSent after the close, await returned (%d, %v)", n, err) + } + default: + if err != nil || n != 2 { + t.Errorf("readLoop delivered a 200 after the close, await returned (%d, %v), want (2, nil)", n, err) + } + } + }) + } +} + +// TestH2ConnectionLoopsEndAfterClose: await relies on readLoop and writeLoop +// returning soon after a connection closes, and on loopsDone saying so. Close +// a live connection and require loopsDone within 2 s. +func TestH2ConnectionLoopsEndAfterClose(t *testing.T) { + srv := startRawH2(t, respondOK) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 4)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + if _, err := cl.DoRequest(ctx, 0); err != nil { + t.Fatal(err) + } + hc := cl.conns[0].cur.Load() + hc.closeConn() + select { + case <-hc.loopsDone: + case <-time.After(2 * time.Second): + t.Fatal("2 s after the connection closed, its readLoop and writeLoop had not both returned: a worker waiting in await for them would wait until its context ends") + } +} From d766a187d4ecbaf5f354d8ad832652cdd979b759 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 02:16:45 +0200 Subject: [PATCH 16/31] ci(test): print each test's verdict and allow 300 s for the root package The Test job ran the root package under -race with -timeout 120s. In the last 22 Test jobs of main and of other PRs (2026-09-01 to 2026-09-27) it took 106.5-109.4 s; in PR #91's two runs, 112.9 s and 112.3 s. A runner about 6 % slower would time the whole package out and fail every test in it, on main as well. The timeout is now 300 s, as the Coverage job already has, and the release gate's copy of the same command gets the same timeout. The job also runs with -v, so its log names every test and its verdict instead of one line per package: a reviewer can see that a new test ran in CI instead of inferring it from the package's run time. --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4ddc8e5..2925728 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -77,7 +77,7 @@ jobs: go mod verify go build ./... go vet ./... - go test -race -count=1 -timeout 120s ./... + go test -race -count=1 -timeout 300s ./... publish: name: Create tag and release From 0cf7afe51640f09582def8d9717e7c61636f3984 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 04:45:18 +0200 Subject: [PATCH 17/31] test(h2client): failing tests for the round-2 review of #91 On e37777e each of these fails for the reason it names: - the h2c upgrade's own stream-1 response marks a connection served, so a server that answers only the upgrade is redialed without backoff (675 and 1321 connections in 300 ms in two runs); - a connection that ends before it answered anything is redialed after a sleep inside a request that can then succeed, and the pace restarts at every dial that succeeds (44 and 46 connections in 400 ms against a server that GOAWAYs each handshake, with 43 and 24 errors); - the backoff sleep comes before the dial, inside the request that dials, and holds the slot lock, so it lands in recorded latencies; - a request body waiting for window never stops when the server answers or resets its stream early, and the connection's later requests hang; - a request queued after the writer's final drain is "connection closing"; - a stream that ends with only a 1xx status is a success; - a HEADERS frame too short for its PRIORITY fields is not a connection error. TestH2RedialBacksOffOnlyWhenTheServerLooksDown now drives DoRequest, and TestH2RequestsTheConnectionNeverWroteAreRetried's server starts each response, so that its connections have served. --- h2client_accounting_test.go | 548 +++++++++++++++++++++++++++++++++--- 1 file changed, 506 insertions(+), 42 deletions(-) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 58a4077..70b3ed5 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -61,6 +61,8 @@ type rawH2Server struct { killAfter time.Duration // > 0: the server closes each connection (no GOAWAY) this long after accepting it closeAfterData int // > 0: the server closes a connection (no GOAWAY) once it has read this many DATA bytes on it settings []http2.Setting // the server's SETTINGS (none: every default) + answerUpgrade bool // answer the h2c upgrade request on stream 1, as celeris does (see rawH2Handshake) + goAwayAtOnce bool // send GOAWAY as soon as the client's SETTINGS are acked, before any request accepted atomic.Int64 // connections accepted killed atomic.Int64 // connections the server ended (rawClose or killAfter) @@ -68,6 +70,8 @@ type rawH2Server struct { dataSent atomic.Int64 // DATA frame payload bytes written, padding included windowCredit atomic.Int64 // connection WINDOW_UPDATE increments received, minus each handshake's own answeredLate atomic.Int64 // requests answered on a connection other than the first one + clientResets atomic.Int64 // RST_STREAM frames received from the client + clientCancel atomic.Int64 // of which with error code CANCEL mu sync.Mutex conns []net.Conn @@ -92,6 +96,8 @@ type rawH2Opts struct { killAfter time.Duration closeAfterData int settings []http2.Setting + answerUpgrade bool + goAwayAtOnce bool } func startRawH2(t *testing.T, h rawH2Handler) *rawH2Server { @@ -104,7 +110,8 @@ func startRawH2With(t *testing.T, opts rawH2Opts, h rawH2Handler) *rawH2Server { if err != nil { t.Fatal(err) } - s := &rawH2Server{ln: ln, handler: h, killAfter: opts.killAfter, closeAfterData: opts.closeAfterData, settings: opts.settings} + s := &rawH2Server{ln: ln, handler: h, killAfter: opts.killAfter, closeAfterData: opts.closeAfterData, settings: opts.settings, + answerUpgrade: opts.answerUpgrade, goAwayAtOnce: opts.goAwayAtOnce} s.wg.Add(1) go func() { defer s.wg.Done() @@ -159,7 +166,7 @@ func (s *rawH2Server) serve(nc net.Conn) { br := bufio.NewReader(nc) fr := http2.NewFramer(nc, br) - if !rawH2Handshake(nc, br, fr, s.settings...) { + if !rawH2Handshake(nc, br, fr, s.answerUpgrade, s.settings...) { _ = nc.Close() return } @@ -182,6 +189,14 @@ func (s *rawH2Server) serve(nc net.Conn) { case *http2.SettingsFrame: if !f.IsAck() { _ = fr.WriteSettingsAck() + if s.goAwayAtOnce { + _ = fr.WriteGoAway(0, http2.ErrCodeNo, nil) // the handshake is done, and no request will be served + } + } + case *http2.RSTStreamFrame: + s.clientResets.Add(1) + if f.ErrCode == http2.ErrCodeCancel { + s.clientCancel.Add(1) } case *http2.PingFrame: if !f.IsAck() { @@ -224,8 +239,12 @@ func (s *rawH2Server) serve(nc net.Conn) { // rawH2Handshake reads the client connection preface, performing the h2c // upgrade first when the connection starts with an HTTP/1.1 request, and -// sends the server's SETTINGS. -func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer, settings ...http2.Setting) bool { +// sends the server's SETTINGS. With answerUpgrade, the upgrade request is +// answered on stream 1, as RFC 7540 §3.2 requires, in the order celeris +// writes it (internal/conn/h2.go NewH2StateFromUpgrade): SETTINGS, a SETTINGS +// ACK for the client's HTTP2-Settings, then the response, before the client +// preface is read. +func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer, answerUpgrade bool, settings ...http2.Setting) bool { head, err := br.Peek(len(http2.ClientPreface)) if err != nil { return false @@ -244,6 +263,11 @@ func rawH2Handshake(nc net.Conn, br *bufio.Reader, fr *http2.Framer, settings .. if err := fr.WriteSettings(settings...); err != nil { return false } + if answerUpgrade { + _ = fr.WriteSettingsAck() + _ = fr.WriteHeaders(http2.HeadersFrameParam{StreamID: 1, BlockFragment: []byte{0x88}, EndHeaders: true}) // :status 200 + _ = fr.WriteData(1, true, []byte(rawOKBody)) + } if head, err = br.Peek(len(http2.ClientPreface)); err != nil || string(head) != http2.ClientPreface { return false } @@ -291,6 +315,11 @@ func (c *rawH2Conn) tornData(streamID uint32, declared, sent int) { _, _ = c.nc.Write(append(hdr, make([]byte, sent)...)) } +// reset ends a stream with RST_STREAM(code). +func (c *rawH2Conn) reset(streamID uint32, code http2.ErrCode) { + _ = c.fr.WriteRSTStream(streamID, code) +} + // ok answers a stream with 200 and the 2-byte body "OK", in one DATA frame. func (c *rawH2Conn) ok(streamID uint32) { c.headers(streamID, false, ":status", "200") @@ -1218,15 +1247,19 @@ func TestH2WindowCreditSentWhileBodyWaitsForWindow(t *testing.T) { } } -// TestH2RedialBacksOffOnlyWhenTheServerLooksDown: h1client redials a -// connection the server ended at once, and backs off only when the redial -// fails (h1client.go DoRequest). HTTP/2 must record a redial the same way. A -// connection that answered a request and then ended is redialed without the -// backoff sleep, which would otherwise be charged to the latency of the -// request that redials; a connection that ended before it answered anything -// is paced, so a server that accepts and drops every connection is not -// redialed in a loop. The backoff is set far beyond each context, so a sleep -// shows as a failed redial. +// TestH2RedialBacksOffOnlyWhenTheServerLooksDown: how a slot redials, matched +// to h1client (h1client.go DoRequest). h1client redials a connection the +// server ended at once, and when the redial fails it counts a connect error +// and sleeps its backoff inside the request it returns as an error, so the +// sleep reaches no recorded latency; the next request dials at once. +// +// HTTP/2 does the same, and treats a connection that ended before it carried +// a response to any request as the server looking down too: the request that +// finds it fails and sleeps, and nothing is dialed for it, so a server that +// completes each handshake and then drops the connection is paced and shows +// as errors. The pace doubles while the server keeps looking down, and is +// reset once a connection has served. Every subtest drives DoRequest; the +// backoff is set far beyond each context where a sleep must not happen. func TestH2RedialBacksOffOnlyWhenTheServerLooksDown(t *testing.T) { srv := startRawH2(t, respondOK) host, port := srv.hostPort() @@ -1239,37 +1272,239 @@ func TestH2RedialBacksOffOnlyWhenTheServerLooksDown(t *testing.T) { return cl } const longBackoff = 20 * time.Second // sleep() jitters it to 10-20 s + // endConn ends the slot's connection as a server that closes it does. + endConn := func(slot *h2ConnSlot, why string) { + slot.cur.Load().failConn(errors.New("test: " + why)) + } + // emptySlot leaves the slot as a failed redial leaves it. + emptySlot := func(slot *h2ConnSlot) { + slot.cur.Load().closeConn() + slot.cur.Store(nil) + } + ctxFor := func(t *testing.T, d time.Duration) context.Context { + ctx, cancel := context.WithTimeout(context.Background(), d) + t.Cleanup(cancel) + return ctx + } t.Run("answered", func(t *testing.T) { cl := newClient(t) - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) - defer cancel() + dials := countDials(cl, 0) + ctx := ctxFor(t, 3*time.Second) if _, err := cl.DoRequest(ctx, 0); err != nil { t.Fatal(err) } slot := cl.conns[0] - slot.cur.Load().failConn(errors.New("test: the server ended the connection")) + endConn(slot, "the server ended the connection") slot.backoff.next = longBackoff start := time.Now() - hc := cl.reconnectSlot(ctx, slot) - t.Logf("redial after a connection that answered: %v, connection=%t", time.Since(start), hc != nil) - if hc == nil { - t.Fatalf("no connection after %v: the redial slept the backoff although the connection it replaces had answered; h1client redials such a connection at once", - time.Since(start)) + _, err := cl.DoRequest(ctx, 0) + t.Logf("request after a connection that answered: %v, err=%v, dials=%d", time.Since(start), err, dials.Load()) + if err != nil { + t.Fatalf("the request that redials a connection which had answered failed after %v (%v): h1client redials such a connection at once, without its backoff", + time.Since(start), err) } }) t.Run("never answered", func(t *testing.T) { cl := newClient(t) - ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) - defer cancel() + dials := countDials(cl, 0) + slot := cl.conns[0] + endConn(slot, "the server dropped the connection before it answered anything") + slot.backoff.next = 200 * time.Millisecond // sleep() jitters it to 100-200 ms + start := time.Now() + _, err := cl.DoRequest(ctxFor(t, 3*time.Second), 0) + elapsed := time.Since(start) + t.Logf("request that found a connection which never answered: %v, err=%v, dials=%d", elapsed, err, dials.Load()) + if err == nil { + t.Fatalf("the request that found a connection which ended before it answered anything succeeded, after %v and %d dial(s): the server looks down, so it must fail, sleep the backoff inside that failure and dial nothing, or the sleep lands in a recorded latency and the outage shows no error", + elapsed, dials.Load()) + } + if n := dials.Load(); n != 0 { + t.Errorf("%d dial(s) for the request that found a connection which never answered: it fails, and the next request dials", n) + } + if elapsed < 100*time.Millisecond { + t.Errorf("the request returned after %v: it did not sleep the backoff (100-200 ms), so a server that drops every connection is redialed in a loop", elapsed) + } + }) + t.Run("after a failed redial", func(t *testing.T) { + cl := newClient(t) + slot := cl.conns[0] + emptySlot(slot) + slot.backoff.next = longBackoff + start := time.Now() + _, err := cl.DoRequest(ctxFor(t, 3*time.Second), 0) + t.Logf("request after a failed redial: %v, err=%v", time.Since(start), err) + if err != nil { + t.Fatalf("the request after a failed redial failed after %v (%v): it slept the backoff before it dialed, so the sleep lands in its latency when the dial succeeds; h1client sleeps only inside the request whose redial failed", + time.Since(start), err) + } + }) + t.Run("a failed redial", func(t *testing.T) { + cl := newClient(t) + ctx := ctxFor(t, 3*time.Second) + if _, err := cl.DoRequest(ctx, 0); err != nil { + t.Fatal(err) + } + dials := countDials(cl, 1) + slot := cl.conns[0] + endConn(slot, "the server ended the connection") + start := time.Now() + _, err := cl.DoRequest(ctx, 0) + elapsed := time.Since(start) + t.Logf("request whose redial failed: %v, err=%v, dials=%d", elapsed, err, dials.Load()) + if err == nil || dials.Load() != 1 { + t.Fatalf("err=%v dials=%d, want an error after 1 failed dial", err, dials.Load()) + } + // The connection it replaces had served, so the pace starts over: + // the first step sleeps 5-10 ms. + if elapsed < reconnectBackoffMin/2 { + t.Errorf("the request whose redial failed returned after %v without sleeping the backoff (at least %v): as in h1client it paces the server that looks down inside the request it fails", + elapsed, reconnectBackoffMin/2) + } + }) + t.Run("the sleep holds no lock", func(t *testing.T) { + cl := newClient(t) + slot := cl.conns[0] + emptySlot(slot) + slot.backoff.next = longBackoff + failed := make(chan struct{}) + var n atomic.Int64 + redial := cl.redial + cl.redial = func() (*h2Conn, error) { + if n.Add(1) == 1 { + close(failed) + return nil, errors.New("test: connection refused") + } + return redial() + } + ctxA, cancelA := context.WithTimeout(context.Background(), 10*time.Second) + defer cancelA() + aDone := make(chan error, 1) + go func() { + _, err := cl.DoRequest(ctxA, 0) + aDone <- err + }() + select { + case <-failed: + case <-time.After(3 * time.Second): + t.Fatal("the request on an empty slot did not dial within 3 s: it slept the backoff before it dialed") + } + // A's redial failed and A sleeps 10-20 s. B, on the same slot, must + // not wait for that sleep: it dials at once. + ctxB, cancelB := context.WithTimeout(context.Background(), 3*time.Second) + defer cancelB() + bDone := make(chan error, 1) + start := time.Now() + go func() { + _, err := cl.DoRequest(ctxB, 1) + bDone <- err + }() + select { + case err := <-bDone: + t.Logf("second request while the first sleeps its backoff: %v, err=%v", time.Since(start), err) + if err != nil { + t.Fatalf("the second request failed (%v): it waited for the backoff sleep of the request whose redial failed", err) + } + case <-time.After(5 * time.Second): + t.Fatal("the second request did not return within 5 s: the request whose redial failed sleeps its backoff holding the slot lock, and every worker of the slot waits for it") + } + aReturned := false + select { + case err := <-aDone: + aReturned = true + t.Errorf("the request whose redial failed returned (%v) before its backoff ended", err) + default: + } + cancelA() + if !aReturned { + <-aDone + } + }) + t.Run("grows until a connection serves", func(t *testing.T) { + cl := newClient(t) slot := cl.conns[0] - slot.cur.Load().failConn(errors.New("test: the server dropped the connection before answering")) + ctx := ctxFor(t, 5*time.Second) + for round := 1; round <= 3; round++ { + if round > 1 { + hc, err := cl.redial() + if err != nil { + t.Fatal(err) + } + slot.cur.Store(hc) + } + endConn(slot, "the server dropped the connection before it answered anything") + if _, err := cl.DoRequest(ctx, 0); err == nil { + t.Fatalf("round %d: the request that found a connection which never answered succeeded (backoff.next=%v)", round, slot.backoff.next) + } + want := min(reconnectBackoffMin< time.Second { + t.Errorf("the first backoff after a connection served took %v: the pace was not reset (want 5-10 ms)", d) + } + }) + t.Run("h2c: only the upgrade's own stream answered", func(t *testing.T) { + // The server answers the upgrade request on stream 1 (RFC 7540 §3.2; + // celeris writes it before it reads the preface) and refuses every + // request of the run. The upgrade's response is a handshake artefact, + // not an answer to a request: the connection has served nothing. + up := startRawH2With(t, rawH2Opts{answerUpgrade: true}, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.reset(sid, http2.ErrCodeRefusedStream) + return rawKeep + }) + uh, uport := up.hostPort() + cl, err := newH2CUpgradeClient(uh, uport, "/", testH2Cfg("GET", nil, nil, 1, 1)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(cl.Close) + dials := countDials(cl, 0) + slot := cl.conns[0] + // The refusal arrives after the stream-1 response, which readLoop has + // therefore read. + var re *HTTP2ResetError + if _, err := cl.DoRequest(ctxFor(t, 3*time.Second), 0); !errors.As(err, &re) || re.Code != uint32(http2.ErrCodeRefusedStream) { + t.Fatalf("got %v, want the REFUSED_STREAM reset", err) + } + if slot.cur.Load().served.Load() { + t.Error("the h2c upgrade's own stream-1 response marked the connection served, though no request of the run was answered on it") + } + endConn(slot, "the server dropped the connection") + slot.backoff.next = longBackoff + _, err = cl.DoRequest(ctxFor(t, 300*time.Millisecond), 0) + t.Logf("request after an upgraded connection that answered only stream 1: err=%v, dials=%d", err, dials.Load()) + if n := dials.Load(); n != 0 { + t.Errorf("the connection that answered only the upgrade's stream 1 was redialed at once (%d dial(s)): a server that answers the upgrade and then drops every connection is redialed in a loop", n) } }) // Guard: with no sleep in the way, the redial must still not outlive @@ -1281,15 +1516,29 @@ func TestH2RedialBacksOffOnlyWhenTheServerLooksDown(t *testing.T) { if _, err := cl.DoRequest(ctx, 0); err != nil { t.Fatal(err) } - slot := cl.conns[0] - slot.cur.Load().failConn(errors.New("test: the server ended the connection")) + dials := countDials(cl, 0) + endConn(cl.conns[0], "the server ended the connection") cancel() - if hc := cl.reconnectSlot(ctx, slot); hc != nil { - t.Fatal("a connection was dialed after the run's context ended: Close, which runs after the cancel, never closes it") + if _, err := cl.DoRequest(ctx, 0); err == nil || dials.Load() != 0 { + t.Fatalf("err=%v dials=%d after the run's context ended: a connection dialed now is one Close, which runs after the cancel, never closes", err, dials.Load()) } }) } +// countDials routes the client's redials through a counter and fails the +// first failFirst of them, as a refused connection would. +func countDials(cl *h2Client, failFirst int64) *atomic.Int64 { + var n atomic.Int64 + redial := cl.redial + cl.redial = func() (*h2Conn, error) { + if n.Add(1) <= failFirst { + return nil, errors.New("test: connection refused") + } + return redial() + } + return &n +} + // TestH2ResponseWithoutAReadableStatusIsNotASuccess: a response counts as a // success only when its :status says so, as an HTTP/1.1 response does only // with a status line (h1client: "short status line" is an error). A :status @@ -1415,17 +1664,21 @@ func TestH2StreamOutlivedByLaterStreamsGetsItsResponse(t *testing.T) { } // TestH2RequestsTheConnectionNeverWroteAreRetried: 4 workers share one -// connection with 4 streams and POST 100 KiB bodies; the server reads the -// 65,535 bytes the connection window allows and closes the connection. The -// first request's body was being written, so it is in flight and is one -// error. The other three were still queued for the writer: they never -// reached the server, so, like a request still waiting for a stream, they go -// to the redialed connection and are not errors. Before the fix their workers -// woke on the closed connection and gave up before the writer answered, and -// the writer failed the ones it reached: 4 errors per connection. +// connection with 4 streams and POST 100 KiB bodies; the server starts each +// response, reads the 65,535 bytes the connection window allows and closes +// the connection. The first request's body was being written, so it is in +// flight and is one error. The other three were still queued for the writer: +// they never reached the server, so, like a request still waiting for a +// stream, they go to the redialed connection and are not errors. Before the +// fix their workers woke on the closed connection and gave up before the +// writer answered, and the writer failed the ones it reached: 4 errors per +// connection. (The server starts each response so that its connections have +// served: since round 2 a connection that ends before it answered anything +// costs the request that finds it, which is TestH2RedialBacksOff...'s case.) func TestH2RequestsTheConnectionNeverWroteAreRetried(t *testing.T) { - srv := startRawH2With(t, rawH2Opts{closeAfterData: 65535}, func(*rawH2Conn, uint32, string) rawH2Action { - return rawKeep // take the request and its body, never answer + srv := startRawH2With(t, rawH2Opts{closeAfterData: 65535}, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + c.headers(sid, false, ":status", "200") // start the response, never end it + return rawKeep }) const streams = 4 res := runBench(t, Config{URL: srv.url("/upload"), Method: "POST", Body: make([]byte, 100<<10), Duration: 400 * time.Millisecond, @@ -1552,3 +1805,214 @@ func TestH2ConnectionLoopsEndAfterClose(t *testing.T) { t.Fatal("2 s after the connection closed, its readLoop and writeLoop had not both returned: a worker waiting in await for them would wait until its context ends") } } + +// --------------------------------------------------------------------------- +// Review of the fixes, round 2 + +// TestH2CUpgradeServerThatAnswersOnlyTheUpgradeIsPaced: an h2c server that +// answers the upgrade request on stream 1, as RFC 7540 §3.2 requires, and +// then closes each connection when the first request of the run arrives, +// without answering it. The stream-1 response must not count as the +// connection having served: every connection ends before it answered a +// request, so the server looks down, and the redials are paced. On e37777e +// the stream-1 response marked each connection served, so each was redialed +// at once: a dial storm, as on a dead port before the backoff existed. +func TestH2CUpgradeServerThatAnswersOnlyTheUpgradeIsPaced(t *testing.T) { + srv := startRawH2With(t, rawH2Opts{answerUpgrade: true}, func(*rawH2Conn, uint32, string) rawH2Action { + return rawClose + }) + res := runBench(t, Config{URL: srv.url("/"), Duration: 300 * time.Millisecond, Workers: 1, + H2CUpgrade: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 4}}) + accepted := srv.accepted.Load() + t.Logf("connections=%d requests=%d errors=%d connect_errors=%d", accepted, res.Requests, res.Errors, res.ConnectErrors) + if accepted > 60 { + t.Errorf("%d connections in %v to a server that answered no request: they were redialed without backoff", accepted, 300*time.Millisecond) + } + if res.Requests != 0 || res.Errors == 0 { + t.Errorf("requests=%d errors=%d, want 0 and at least 1: the server answered nothing", res.Requests, res.Errors) + } +} + +// TestH2ServerThatDropsEveryConnectionShowsErrors: the server completes each +// handshake, then sends GOAWAY before any request. No request reaches it, so +// none is in flight when a connection ends; each still costs the request that +// finds the connection gone, as h1client charges each request to a server +// that drops its connections, and the pace grows, so the run shows the outage +// as errors and a handful of connections. On e37777e the pace restarted at +// every dial that succeeded (44 and 46 connections in two runs), and whether +// a request was an error or retried silently depended on whether it was +// written before the GOAWAY was read (43 and 24 errors). +func TestH2ServerThatDropsEveryConnectionShowsErrors(t *testing.T) { + srv := startRawH2With(t, rawH2Opts{goAwayAtOnce: true}, func(*rawH2Conn, uint32, string) rawH2Action { + return rawKeep + }) + res := runBench(t, Config{URL: srv.url("/"), Duration: 400 * time.Millisecond, Workers: 1, + HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 4}}) + accepted := srv.accepted.Load() + t.Logf("connections=%d requests=%d errors=%d connect_errors=%d", accepted, res.Requests, res.Errors, res.ConnectErrors) + if res.Errors < accepted-2 { + t.Errorf("errors=%d over %d connections that each ended before answering anything: the outage is not counted", res.Errors, accepted) + } + if accepted > 25 { + t.Errorf("%d connections in %v to a server that drops every one: the backoff does not grow while the server looks down", accepted, 400*time.Millisecond) + } +} + +// TestH2BodyWaitEndsWhenTheServerEndsTheStream: a POST body larger than the +// server's stream window waits for a WINDOW_UPDATE; the server ends the +// stream first and never grants more. RFC 9113 §8.1 lets a server answer +// before the body is complete and then RST_STREAM(NO_ERROR) (x/net's server +// does so when its handler returns without reading the body); a server may +// also refuse the stream. It sends no window for a stream it has ended, so +// the wait must end there, without failing the connection: the later requests +// of the connection are queued behind the writer. A response that ends +// without a reset leaves the client's half of the stream open, which the +// client closes with RST_STREAM(CANCEL); after a reset it sends none (§5.4.2). +// On e37777e the writer polled for window until the run ended, and every +// later request of the connection hung, uncounted. +func TestH2BodyWaitEndsWhenTheServerEndsTheStream(t *testing.T) { + const requests = 3 + for _, tc := range []struct { + name string + answer func(c *rawH2Conn, sid uint32) + success bool // the request is a success (else the reset) + minCancels int64 // RST_STREAM(CANCEL) the client must send, at least + maxResets int64 // RST_STREAM the client may send, at most + }{ + {"response then RST_STREAM(NO_ERROR)", func(c *rawH2Conn, sid uint32) { c.ok(sid); c.reset(sid, http2.ErrCodeNo) }, true, 0, requests}, + {"response only", func(c *rawH2Conn, sid uint32) { c.ok(sid) }, true, requests, requests}, + {"RST_STREAM(REFUSED_STREAM)", func(c *rawH2Conn, sid uint32) { c.reset(sid, http2.ErrCodeRefusedStream) }, false, 0, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := startRawH2With(t, rawH2Opts{settings: uploadWindow}, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + tc.answer(c, sid) + return rawKeep // never grant window + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/upload", testH2Cfg("POST", nil, make([]byte, 2000), 1, 4)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + for i := range requests { + _, err := cl.DoRequest(ctx, 0) + var re *HTTP2ResetError + if tc.success && err != nil || !tc.success && !errors.As(err, &re) { + t.Fatalf("request %d: %v (connections=%d): the server ended each stream at once, but the connection's writer still waits for window for a body the server will never take", + i+1, err, srv.accepted.Load()) + } + } + deadline := time.Now().Add(2 * time.Second) + for srv.clientCancel.Load() < tc.minCancels && time.Now().Before(deadline) { + time.Sleep(2 * time.Millisecond) + } + time.Sleep(50 * time.Millisecond) // a reset the client should not send would be flushed by now + t.Logf("connections=%d client RST_STREAM=%d (CANCEL %d)", srv.accepted.Load(), srv.clientResets.Load(), srv.clientCancel.Load()) + if n := srv.accepted.Load(); n != 1 { + t.Errorf("connections=%d, want 1: a stream the server ended is not a connection error", n) + } + if got := srv.clientCancel.Load(); got < tc.minCancels { + t.Errorf("the client sent %d RST_STREAM(CANCEL), want %d: a body it stops sending leaves its half of the stream open on the server", got, tc.minCancels) + } + if got := srv.clientResets.Load(); got > tc.maxResets { + t.Errorf("the client sent %d RST_STREAM, want at most %d: never one for a stream the server reset", got, tc.maxResets) + } + }) + } +} + +// TestH2AwaitAnswersARequestQueuedAfterTheWriterReturned: a worker's hand-off +// to writeCh and the connection's done can be ready together, and select +// picks either, so a request can be queued after writeLoop's final drain +// (answerQueued). Nobody wrote it, so it never reached the server: await, +// once both loops have returned, answers it errH2NotSent (DoRequest takes it +// to the redialed connection) instead of "connection closing", an error. On +// e37777e it was an error. +func TestH2AwaitAnswersARequestQueuedAfterTheWriterReturned(t *testing.T) { + hc := &h2Conn{done: make(chan struct{}), loopsDone: make(chan struct{}), streamSem: make(chan struct{}, 1), writeCh: make(chan h2WriteReq, 4)} + close(hc.done) + close(hc.loopsDone) + ch := make(chan h2Response, 1) + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: []byte{0x82}, respCh: &ch} // queued after the writer returned + n, err := hc.await(context.Background(), &ch, 0) + <-hc.streamSem // the token await returned + t.Logf("await returned (%d, %v)", n, err) + if err != errH2NotSent { + t.Errorf("a request still queued for a writer that has returned was answered (%d, %v), want errH2NotSent: it was never written", n, err) + } + if len(hc.writeCh) != 0 { + t.Errorf("%d request(s) left in writeCh after await", len(hc.writeCh)) + } +} + +// TestH2ResponseWithOnlyAnInterimStatusIsNotASuccess: a stream that ends +// while its only :status is an interim 1xx has no final response, which RFC +// 9113 §8.1 makes malformed: an error (errH2NoStatus), not a success. On +// e37777e both shapes were a success with a 1xx status. +func TestH2ResponseWithOnlyAnInterimStatusIsNotASuccess(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, path string) rawH2Action { + switch path { + case "/interim-ends-stream": // HEADERS 103 with END_STREAM + c.headers(sid, true, ":status", "103") + case "/interim-then-data": // HEADERS 100, then the stream ends on DATA + c.headers(sid, false, ":status", "100") + c.data(sid, true, len(rawOKBody), 0) + case "/interim-then-final": // HEADERS 103, then the final 200: a success + c.headers(sid, false, ":status", "103") + c.ok(sid) + default: + c.ok(sid) + } + return rawKeep + }) + host, port := srv.hostPort() + for _, tc := range []struct { + path string + status int // 0: a success; -1: an error other than a status + }{ + {"/interim-ends-stream", -1}, + {"/interim-then-data", -1}, + {"/interim-then-final", 0}, + {"/ok", 0}, + } { + n, err := h2Once(t, host, port, tc.path) + t.Logf("%-22s status=%d bytes=%d err=%v", tc.path, h2Status(err), n, err) + if got := h2Status(err); got != tc.status { + t.Errorf("%s: recorded status %d (err=%v), want %d (0: success, -1: an error): a response with no final status is not a success", + tc.path, got, err, tc.status) + } + } +} + +// TestH2HeadersTooShortForTheirPriorityFieldsEndTheConnection: a HEADERS +// frame flagged PRIORITY whose payload has no room for the 5 bytes of +// priority fields is a frame size error on a frame that carries a field +// block, so a connection error (RFC 9113 §4.2, §6.2), as a Pad Length that +// does not fit is since round 1. On e37777e it was read as an empty block: +// the stream failed with errH2NoStatus and the connection went on. +func TestH2HeadersTooShortForTheirPriorityFieldsEndTheConnection(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + // PRIORITY flagged, 1 byte of payload: :status 200 where the 5 priority bytes should be. + _ = c.fr.WriteRawFrame(http2.FrameHeaders, http2.FlagHeadersEndHeaders|http2.FlagHeadersEndStream|http2.FlagHeadersPriority, sid, []byte{0x88}) + return rawKeep + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 1)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + hc := cl.conns[0].cur.Load() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + _, err = cl.DoRequest(ctx, 0) + t.Logf("err=%v connection closed=%t", err, hc.closed.Load()) + if err == nil { + t.Fatal("a HEADERS frame too short for its priority fields was a success") + } + if !hc.closed.Load() { + t.Errorf("the connection goes on after a HEADERS frame too short for its priority fields (err=%v): it is a connection error", err) + } +} From a7a4601257f98799183f901e1ce9b90101ae46ed Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 04:46:13 +0200 Subject: [PATCH 18/31] fix(h2client): the h2c upgrade's own stream 1 does not mark a connection served (#89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readLoop marked a connection served on the first HEADERS or DATA frame of any stream. On an h2c-upgraded connection that is the upgrade request's own response on stream 1, which RFC 7540 §3.2 requires the server to send and celeris writes before it reads the client preface. Every upgraded connection was therefore served before it answered a request, so reconnectSlot redialed a server that answers the upgrade and then drops each connection without backoff: 675 and 1321 connections in 300 ms in two runs of the new test, where main slept 5-10 ms before every redial. The connection keeps its first request stream (1, or 3 after an upgrade), and only a frame of a stream from there on marks it served. --- h2client.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/h2client.go b/h2client.go index e97fabf..99b9804 100644 --- a/h2client.go +++ b/h2client.go @@ -164,6 +164,9 @@ type h2Conn struct { // pending streams share a slot. nextStreamID atomic.Uint32 streamSlots []h2StreamSlot + // firstStreamID is the connection's first request stream: 1, or 3 on an + // h2c-upgraded connection, whose stream 1 is the upgrade request itself. + firstStreamID uint32 // Channel pool — pools *chan h2Response (heap-allocated pointers). // The pointer in the slot remains valid even after the goroutine exits. @@ -214,8 +217,9 @@ type h2Conn struct { // redials the slot instead. Set before done is closed. closed atomic.Bool // served is set by readLoop once the connection carries a response - // frame. reconnectSlot backs off before it replaces a connection that - // never did: the server is down or drops every connection. + // frame for one of the run's requests (not the h2c upgrade's own stream + // 1). reconnectSlot backs off before it replaces a connection that never + // did: the server is down or drops every connection. served atomic.Bool } @@ -722,11 +726,12 @@ func completeH2Handshake(conn net.Conn, br *bufio.Reader, addr string, maxStream numSlots := 2 * effectiveStreams hc := &h2Conn{ - conn: conn, - framer: framer, - bufWriter: bw, - writeCh: make(chan h2WriteReq, 4096), - streamSlots: make([]h2StreamSlot, numSlots), + conn: conn, + framer: framer, + bufWriter: bw, + writeCh: make(chan h2WriteReq, 4096), + streamSlots: make([]h2StreamSlot, numSlots), + firstStreamID: initialStreamID, // DATA-send split = the SERVER's max frame size (not our 64KiB receive cap). maxFrameSize: serverMaxFrame, serverInitWindow: serverInitWin, @@ -1034,8 +1039,11 @@ func (hc *h2Conn) readLoop() { } // stream returns the slot of streamID with its response state reset if the -// slot last held another stream, and marks the connection served. readLoop -// only. +// slot last held another stream, and marks the connection served when the +// stream is one of the run's requests. The h2c upgrade's own stream 1 is not: +// the server answers it on every upgraded connection (celeris before it even +// reads the client preface), so it shows nothing about whether the server +// serves the run. readLoop only. func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { st := &hc.streamSlots[(streamID>>1)%uint32(len(hc.streamSlots))] if st.streamID != streamID { @@ -1043,7 +1051,7 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { st.status = 0 st.final = false st.bytes = 0 - if !hc.served.Load() { + if streamID >= hc.firstStreamID && !hc.served.Load() { hc.served.Store(true) } } From 2f361adeff848e2ba2e8fd813b32ba11f21c47eb Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 04:50:31 +0200 Subject: [PATCH 19/31] fix(h2client): pace a server that looks down inside the request it fails, as h1client does (#89) Three review findings on reconnectSlot, one change: - A connection that ended before it answered any request was redialed after a backoff sleep inside the request that then used the new connection. The request was not counted, and requests the dead connection never wrote looped in DoRequest, so an outage of this shape showed few or no errors where h1client fails every request. The request that finds such a connection now fails (errH2NeverServed), and nothing is dialed for it: the server looks down, as it does when a dial fails. - The backoff was reset after every dial that succeeded, so against a server that completes each handshake and drops it the pace never grew past its first 5-10 ms step. It now doubles while connections keep ending unserved, and starts over once a connection has served. - The sleep came before the dial, in the request that dials, and under the slot lock: a request that then succeeded recorded it in its latency, and so did the slot's other workers waiting on the lock. h1client sleeps only inside the request whose redial failed, which records no latency. So does this now: the request that fails takes the interval under the lock and sleeps it after releasing it, and the next request dials at once. connectBackoff gains step(), the interval without the sleep; sleep() is step() plus the same ctx-abortable wait. The failed-dial error now wraps its cause, as h1client's does. --- backoff.go | 14 ++++++- h2client.go | 109 ++++++++++++++++++++++++++++++---------------------- 2 files changed, 77 insertions(+), 46 deletions(-) diff --git a/backoff.go b/backoff.go index f5c4601..da34578 100644 --- a/backoff.go +++ b/backoff.go @@ -46,13 +46,25 @@ type connectBackoff struct { // cancelled or abort is closed, so Close() and run shutdown are never stuck // behind a sleep. A nil abort channel is valid and never fires. func (b *connectBackoff) sleep(ctx context.Context, abort <-chan struct{}) bool { + return sleepFor(ctx, abort, b.step()) +} + +// step returns the current backoff interval, jittered uniformly down to half, +// and doubles the interval for the following call, capped at +// reconnectBackoffMax, without sleeping: a caller that owns the backoff under +// a lock takes the interval there and sleeps it after releasing the lock. +func (b *connectBackoff) step() time.Duration { d := b.next if d <= 0 { d = reconnectBackoffMin } b.next = min(d*2, reconnectBackoffMax) + return d/2 + rand.N(d/2+1) // uniform in [d/2, d] +} - d = d/2 + rand.N(d/2+1) // uniform in [d/2, d] +// sleepFor blocks for d. It returns early — reporting false — when ctx is +// cancelled or abort is closed. A nil abort channel is valid and never fires. +func sleepFor(ctx context.Context, abort <-chan struct{}, d time.Duration) bool { t := time.NewTimer(d) defer t.Stop() select { diff --git a/h2client.go b/h2client.go index 99b9804..aaf4824 100644 --- a/h2client.go +++ b/h2client.go @@ -218,8 +218,9 @@ type h2Conn struct { closed atomic.Bool // served is set by readLoop once the connection carries a response // frame for one of the run's requests (not the h2c upgrade's own stream - // 1). reconnectSlot backs off before it replaces a connection that never - // did: the server is down or drops every connection. + // 1). A connection that ended without one shows the server down or + // dropping every connection: reconnectSlot fails the request that finds + // it, after the slot's backoff. served atomic.Bool } @@ -465,48 +466,57 @@ func newH2ClientWithDialer(host, port, path string, cfg Config, upgrade bool) (* }, nil } -// reconnectSlot re-establishes a dead slot's connection, single-flighted under -// the slot lock and, while the server looks down, paced by the slot's -// backoff. Returns the live conn, or nil if the redial failed (caller -// surfaces a connect error) or ctx ended. A concurrent caller that finds the -// slot already healed returns immediately. -func (c *h2Client) reconnectSlot(ctx context.Context, slot *h2ConnSlot) *h2Conn { +// reconnectSlot replaces a slot's dead connection, single-flighted under the +// slot lock: a caller that finds the slot already healed takes the new +// connection. When the server looks down, it fails the request instead, as +// h1client fails a request whose redial fails: the dial failed (a connect +// error), or the connection being replaced ended before it answered a single +// request (errH2NeverServed: the server completes handshakes and drops them, +// and nothing is dialed for this request). That request then sleeps the +// slot's backoff after it releases the lock, so the sleep paces the server, +// lands in no recorded latency (a failed request records none) and holds up +// none of the slot's other workers; the next request dials at once. The +// backoff doubles while the server keeps looking down and starts over once a +// connection has served. After the run's context ends it dials nothing. +func (c *h2Client) reconnectSlot(ctx context.Context, slot *h2ConnSlot) (*h2Conn, error) { slot.mu.Lock() - defer slot.mu.Unlock() - old := slot.cur.Load() if old != nil && !old.closed.Load() { - return old // another worker already re-dialed this slot + slot.mu.Unlock() + return old, nil // another worker already re-dialed this slot } - if old != nil { - old.closeConn() // release fds/goroutines of the dead conn + if ctx.Err() != nil { + slot.mu.Unlock() + return nil, ctx.Err() // Close, which runs after the cancel, would never close a connection dialed now } - // Back off only when the server looks down, as h1client does: the last - // redial failed (the slot is empty), or the connection ended before it - // carried a single response, so a server that accepts and drops every - // connection cannot be hot-redialled. A connection that served and then - // ended (the server closed it or sent GOAWAY, or its stream IDs ran out) - // is redialed at once: the sleep would be charged to the latency of the - // request that redials, where h1client sleeps only after a failed redial, - // inside a request it counts as an error. - if old == nil || !old.served.Load() { - if !slot.backoff.sleep(ctx, nil) { - return nil // ctx cancelled mid-backoff + var err error + if old != nil { + old.closeConn() // release fds/goroutines of the dead conn + if old.served.Load() { + slot.backoff.reset() // the server was up: redial at once, and the pace starts over + } else { + err = errH2NeverServed } } - if ctx.Err() != nil { - return nil // the run is over (sleep may report its timer although ctx is done too) - } - hc, err := c.redial() - if err != nil { + if err == nil { + hc, dialErr := c.redial() + if dialErr == nil { + slot.cur.Store(hc) + slot.mu.Unlock() + return hc, nil + } recordConnectError() - slot.cur.Store(nil) - return nil + err = dialErr } - slot.backoff.reset() - slot.cur.Store(hc) - return hc + + // The server looks down. The next request dials at once; this one fails + // after the backoff, which it sleeps outside the lock. + slot.cur.Store(nil) + pause := slot.backoff.step() + slot.mu.Unlock() + sleepFor(ctx, nil, pause) + return nil, err } // h2HopByHopHeaders lists HTTP/1.1 connection-specific headers that are @@ -1058,6 +1068,12 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { return st } +// errH2NeverServed is DoRequest's error for a request that found its +// connection ended before the connection answered any request: the server +// completes handshakes and drops the connections, or is shutting down. The +// server looks down, so the request fails (see reconnectSlot). +var errH2NeverServed = errors.New("h2client: the connection ended before it answered a request") + // errH2NoStatus is DoRequest's error for a response without a :status the // client can read: no :status at all, or none that extractStatus decodes. It // is not a success, as h1client fails a response without a status line. @@ -1081,16 +1097,20 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { hc := slot.cur.Load() // A server that closed, reset or GOAWAYed this connection mid-cell - // leaves the slot dead (failConn); re-dial (paced) so the - // request gets a fresh conn instead of spinning closed-conn errors. - // backoff lives in reconnectSlot. + // leaves the slot dead (failConn); re-dial so the request gets a + // fresh conn instead of spinning closed-conn errors. While the + // server looks down, reconnectSlot fails the request instead, paced + // by the slot's backoff. if hc == nil || hc.closed.Load() { - hc = c.reconnectSlot(ctx, slot) - if hc == nil { - if ctx.Err() != nil { + var err error + if hc, err = c.reconnectSlot(ctx, slot); err != nil { + switch { + case ctx.Err() != nil: return 0, ctx.Err() + case err == errH2NeverServed: + return 0, fmt.Errorf("h2client: conn[%d]: %w", idx, err) } - return 0, fmt.Errorf("h2client: conn[%d] reconnect failed", idx) + return 0, fmt.Errorf("h2client: conn[%d] reconnect failed: %w", idx, err) } } @@ -1102,10 +1122,9 @@ func (c *h2Client) DoRequest(ctx context.Context, workerID int) (int, error) { // waited for a stream, or before the connection's writer reached it // (the connection died, or ran out of stream IDs). Each pass through // here needs a connection to have ended, and reconnectSlot redials - // without its backoff only a connection that carried a response, so - // a server that keeps closing connections cannot make this loop - // spin: either it serves on each connection, or each redial is - // paced. ctx ends it. + // only a connection that answered a request: one that never did + // fails this request, so a server that keeps dropping connections + // cannot make this loop spin uncounted. ctx ends it too. } } From 0b1adcd59cd2e9401769084a51ed8a8b2c90f872 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 04:50:47 +0200 Subject: [PATCH 20/31] fix(h2client): a request body stops waiting for window once the server ends its stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing on main. writeBodyFlowControlled waited for send window until a flush failed or the connection closed. RFC 9113 §8.1 lets a server answer before the request body is complete and then reset the stream with RST_STREAM(NO_ERROR), which x/net's server does when a handler returns without reading the body; a server can also refuse the stream. It grants no window for a stream it has ended, and the connection stays healthy, so the writer polled until the run ended and every later request of the connection waited behind it, never counted. A body larger than the server's stream window was enough (the removed post-64k-h2 row: 65,536 bytes against the default 65,535). The wait now also ends when readLoop has taken the stream's channel out of its slot, which it does when the response ends or the stream is reset. That is not a connection error: the worker already has its answer, and the writer goes on with the next request. A response that ended without a reset leaves the client's half of the stream open, which it closes with RST_STREAM(CANCEL); after the server's reset it sends none (§5.4.2). processWriteReq keys the send window to the stream before its HEADERS are written, so a WINDOW_UPDATE or RST_STREAM that comes back at once is not lost. --- h2client.go | 67 +++++++++++++++++++++++++++++++++++++++-------------- h2framer.go | 18 ++++++++++++++ 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/h2client.go b/h2client.go index aaf4824..980ae60 100644 --- a/h2client.go +++ b/h2client.go @@ -189,10 +189,13 @@ type h2Conn struct { // currently-writing stream's window lives in curStreamWindow keyed by // curStreamID; readLoop replenishes both on WINDOW_UPDATE. Without this a // 64KiB body (post-64k-h2 = 65536 B) exceeds the 65535 window by one byte - // and the request hangs until the run ends. + // and the request hangs until the run ends. curStreamReset is the ID of + // that stream once the server has reset it (RST_STREAM): the body writer + // then stops, and sends no RST_STREAM of its own (RFC 9113 §5.4.2). connSendWindow atomic.Int64 curStreamID atomic.Uint32 curStreamWindow atomic.Int64 + curStreamReset atomic.Uint32 serverInitWindow uint32 // Concurrency limit @@ -908,10 +911,20 @@ func (hc *h2Conn) processWriteReq(req h2WriteReq) { slotIdx := (streamID >> 1) % numSlots hc.streamSlots[slotIdx].ch.Store(req.respCh) + if req.hasBody { + // Key the send window to this stream before its HEADERS can reach + // the server, which may grant window for it or reset it as soon + // as they do. + hc.curStreamWindow.Store(int64(hc.serverInitWindow)) + hc.curStreamID.Store(streamID) + } err := hc.framer.WriteHeaders(streamID, req.block, !req.hasBody) if err == nil && req.hasBody { - err = hc.writeBodyFlowControlled(streamID, req.data) + err = hc.writeBodyFlowControlled(streamID, &hc.streamSlots[slotIdx], req.respCh, req.data) + if err == errH2BodyAbandoned { + return // the server ended the stream, and its worker has the answer; the connection is fine + } } // Whoever takes respCh out of the slot answers it, exactly once: // readLoop may have taken it first (a response, a reset, or @@ -1005,6 +1018,9 @@ func (hc *h2Conn) readLoop() { } case frameRSTStream: + if frame.StreamID == hc.curStreamID.Load() { + hc.curStreamReset.Store(frame.StreamID) // before the answer: the body writer reads them in the other order + } idx := (frame.StreamID >> 1) % numSlots chPtr := hc.streamSlots[idx].ch.Swap(nil) if chPtr != nil { @@ -1239,23 +1255,27 @@ func (hc *h2Conn) finish(chPtr *chan h2Response, resp h2Response) (int, error) { return resp.bytesRead, nil } +// errH2BodyAbandoned is writeBodyFlowControlled's report of a body it stopped +// because the server ended the stream first. Not an error: the stream's worker +// already has its answer, and the connection is fine. +var errH2BodyAbandoned = errors.New("h2client: request body abandoned: the server ended the stream") + // writeBodyFlowControlled sends a request body as DATA frames, respecting BOTH // the server's SETTINGS_MAX_FRAME_SIZE and its connection + per-stream send // windows (RFC 7540 §6.9). The writer goroutine is sequential (one body at a // time), so the active stream's window lives in hc.curStreamWindow keyed by -// hc.curStreamID; readLoop replenishes connSendWindow/curStreamWindow on -// WINDOW_UPDATE. When a window is exhausted we flush the buffered frames (so the -// server can consume + replenish) and poll — no extra goroutine/channel. The -// wait ends when that flush fails, or when the connection is closed (done), -// which Close does at the end of a run and failConn when the connection dies: -// that is the backstop against a peer that never grants window. +// hc.curStreamID (processWriteReq keys them); readLoop replenishes +// connSendWindow/curStreamWindow on WINDOW_UPDATE. When a window is exhausted +// we flush the buffered frames (so the server can consume + replenish) and +// poll — no extra goroutine/channel. The wait ends when that flush fails, when +// the connection is closed (done), which Close does at the end of a run and +// failConn when the connection dies, or when the server has ended the stream +// (st no longer holds respCh): the backstops against a peer that never grants +// window. // // Without this, a 64KiB body (post-64k-h2 = 65536 B) either trips FRAME_SIZE_ERROR // (64KiB DATA frame vs a 16384-default server) or overruns the 65535 window. -func (hc *h2Conn) writeBodyFlowControlled(streamID uint32, data []byte) error { - hc.curStreamWindow.Store(int64(hc.serverInitWindow)) - hc.curStreamID.Store(streamID) - +func (hc *h2Conn) writeBodyFlowControlled(streamID uint32, st *h2StreamSlot, respCh *chan h2Response, data []byte) error { maxFrame := int(hc.maxFrameSize) if maxFrame < 16384 { maxFrame = 16384 @@ -1269,6 +1289,24 @@ func (hc *h2Conn) writeBodyFlowControlled(streamID uint32, data []byte) error { avail = sw } if avail <= 0 { + select { + case <-hc.done: + return fmt.Errorf("h2client: conn closed mid-body (flow-control wait)") + default: + } + // The server has answered or reset the stream (readLoop took its + // channel out of the slot): RFC 9113 §8.1 lets it answer before + // the body is complete. It grants no more window for a stream it + // has ended, so stop the body here instead of waiting for the run + // to end with every later request of the connection queued + // behind this one. A response that ended without a reset leaves + // our half of the stream open: close it with RST_STREAM(CANCEL). + if respCh != nil && st.ch.Load() != respCh { + if hc.curStreamReset.Load() != streamID { + _ = hc.framer.WriteRSTStream(streamID, h2ErrCodeCancel) + } + return errH2BodyAbandoned + } // Window exhausted: flush so the peer receives what we've sent and // can send WINDOW_UPDATE, then wait for readLoop to replenish. // The flush carries the receive window readLoop has credited @@ -1280,11 +1318,6 @@ func (hc *h2Conn) writeBodyFlowControlled(streamID uint32, data []byte) error { if err := hc.bufWriter.Flush(); err != nil { return err } - select { - case <-hc.done: - return fmt.Errorf("h2client: conn closed mid-body (flow-control wait)") - default: - } time.Sleep(50 * time.Microsecond) continue } diff --git a/h2framer.go b/h2framer.go index d463637..2fbdf0e 100644 --- a/h2framer.go +++ b/h2framer.go @@ -26,6 +26,11 @@ const ( flagPriority = 0x20 // HEADERS: 5 bytes of stream dependency and weight before the block ) +// H2 error codes (RFC 9113 §7) the client sends. +const ( + h2ErrCodeCancel = 0x8 +) + // H2 settings IDs. const ( settingHeaderTableSize = 0x1 @@ -294,6 +299,19 @@ func (fr *h2Framer) WriteWindowUpdate(streamID, incr uint32) error { return err } +// WriteRSTStream writes a RST_STREAM frame. Single 13-byte write via wbuf. +func (fr *h2Framer) WriteRSTStream(streamID, errCode uint32) error { + fr.wbuf[0] = 0 // length = 4 + fr.wbuf[1] = 0 + fr.wbuf[2] = 4 + fr.wbuf[3] = frameRSTStream + fr.wbuf[4] = 0 + binary.BigEndian.PutUint32(fr.wbuf[5:9], streamID) + binary.BigEndian.PutUint32(fr.wbuf[9:13], errCode) + _, err := fr.bw.Write(fr.wbuf[:13]) + return err +} + // WriteGoAway writes a GOAWAY frame. func (fr *h2Framer) WriteGoAway(lastStreamID, errCode uint32, debugData []byte) error { if len(debugData) == 0 { From 50ae9811ccae02c0558ca3b0a0a779f3e2b78383 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 04:50:48 +0200 Subject: [PATCH 21/31] fix(h2client): a request queued after the writer's last drain is retried, not an error (#89) roundTrip hands a request to writeCh in a select that also watches done. When both are ready, select picks either, so a request can be queued after writeLoop's final drain (answerQueued) has run. Nobody wrote it and nobody answered it, and await's done branch then returned "connection closing", an error, for a request that never reached the server. Once both loops have returned, await drains the queue itself: anything still in writeCh was never written, and each entry is answered errH2NotSent once, as it is taken off the queue. DoRequest takes it to the redialed connection. --- h2client.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/h2client.go b/h2client.go index 980ae60..1c16550 100644 --- a/h2client.go +++ b/h2client.go @@ -1227,6 +1227,12 @@ func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (i hc.streamSem <- struct{}{} return 0, ctx.Err() } + // writeLoop has returned, so whatever is still queued for it was + // never written: roundTrip's hand-off can win its race with done + // after the writer's final drain. Answer those requests too, this + // one among them (each is taken off the queue once, so answered + // once). + hc.answerQueued() select { case resp := <-*chPtr: return hc.finish(chPtr, resp) From 5cb2197f16c0a229b8d1b0684a6732dd20415667 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 04:50:48 +0200 Subject: [PATCH 22/31] fix(h2client): a stream with only an interim status, or HEADERS too short for their priority fields, is not a success (#85) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A stream that ends while its only :status is an interim 1xx has no final response, which RFC 9113 §8.1 makes malformed: a HEADERS frame with a 1xx and END_STREAM, or a 1xx followed by DATA. finish counted it a success; it is now errH2NoStatus, whose text says "final :status". - A HEADERS frame flagged PRIORITY whose payload has no room for the 5 priority bytes was read as an empty field block (errH2NoStatus on the stream). It is a frame size error on a frame that carries a field block, so a connection error (RFC 9113 §4.2), as a Pad Length that does not fit is since round 1: readLoop fails the connection. Both only matter against a broken server. --- h2client.go | 22 ++++++++++++++++------ h2framer.go | 8 ++++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/h2client.go b/h2client.go index 1c16550..26fd141 100644 --- a/h2client.go +++ b/h2client.go @@ -975,6 +975,14 @@ func (hc *h2Conn) readLoop() { frame.Type, frame.StreamID, frame.Length)) return } + // So is a HEADERS frame too short for the priority fields its + // PRIORITY flag announces: a frame size error on a frame that + // carries a field block (RFC 9113 §4.2). + if frame.badPriority() { + hc.failConn(fmt.Errorf("h2client: FRAME_SIZE_ERROR: HEADERS on stream %d: its %d-byte payload has no room for its priority fields", + frame.StreamID, frame.Length)) + return + } switch frame.Type { case frameHeaders: @@ -1090,10 +1098,11 @@ func (hc *h2Conn) stream(streamID uint32) *h2StreamSlot { // server looks down, so the request fails (see reconnectSlot). var errH2NeverServed = errors.New("h2client: the connection ended before it answered a request") -// errH2NoStatus is DoRequest's error for a response without a :status the -// client can read: no :status at all, or none that extractStatus decodes. It -// is not a success, as h1client fails a response without a status line. -var errH2NoStatus = errors.New("h2client: response without a readable :status") +// errH2NoStatus is DoRequest's error for a response without a final :status +// the client can read: no :status at all, none that extractStatus decodes, or +// only an interim 1xx. It is not a success, as h1client fails a response +// without a status line. +var errH2NoStatus = errors.New("h2client: response without a final :status") // errH2NotSent reports a request that never reached the server: the // connection died before the request was handed to it (roundTrip), or took @@ -1245,14 +1254,15 @@ func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (i // finish returns the stream's channel and token and turns its answer into // DoRequest's result: a status >= 400 is an error, as h1client counts it, and -// so is a response whose :status could not be read. +// so is a response with no final :status: none that could be read, or only an +// interim 1xx, which RFC 9113 §8.1 makes a malformed response. func (hc *h2Conn) finish(chPtr *chan h2Response, resp h2Response) (int, error) { hc.chanPool.Put(chPtr) hc.streamSem <- struct{}{} if resp.err != nil { return 0, resp.err } - if resp.status < 100 { + if resp.status < 200 { return resp.bytesRead, errH2NoStatus } if resp.status >= 400 { diff --git a/h2framer.go b/h2framer.go index 2fbdf0e..5f5a58c 100644 --- a/h2framer.go +++ b/h2framer.go @@ -93,6 +93,14 @@ func (f *h2Frame) badPadding() bool { (len(f.payload) == 0 || int(f.payload[0]) >= len(f.payload)) } +// badPriority reports a HEADERS frame flagged PRIORITY whose payload, without +// its padding, is shorter than the 5 bytes of priority fields: a frame size +// error on a frame that carries a field block, so a connection error (RFC 9113 +// §4.2, §6.2). Check badPadding first. +func (f *h2Frame) badPriority() bool { + return f.Type == frameHeaders && f.Flags&flagPriority != 0 && len(f.unpadded()) < 5 +} + // Data returns the data of a DATA frame, without its padding. Flow control // counts the whole payload, padding included (RFC 9113 §6.9.1): use Length. func (f *h2Frame) Data() []byte { return f.unpadded() } From 81a3f7571674f9e9a41f51c31f50ba5e2511651a Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 10:22:10 +0200 Subject: [PATCH 23/31] test(h2client): failing tests for the round-3 review of #91, and three tests that could not fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New tests, each failing on 147edb4 for the reason it names: - TestH2BodyStopsAtTheServersReset: after the server's RST_STREAM the request-body writer kept sending DATA for as long as send window remained (RFC 9113 §6.4). The server grants 128 MiB, resets a 32 MiB upload at its HEADERS and pauses, and counts the DATA of that stream it reads before the next stream's HEADERS. - TestH2ConnectionOutOfStreamIDsIsFailedWhenItDies (2 subtests): a connection out of stream IDs is marked closed without being closed; when the server then closes it, or a write to it fails, readLoop and writeFailed took it for the client's own close and failed nothing, so its last stream waited for the rest of the run. - TestH2RedialTakesTheResponseTheConnectionHadReceived: reconnectSlot read served while readLoop still held the connection's first response, so a connection that had served was treated as never served. Tests that could not fail on their defect (RULE 27), reworked: - TestH2RedialBacksOffOnlyWhenTheServerLooksDown/grows_until_a_connection_serves installed each connection itself (cl.redial, slot.cur.Store), which skips reconnectSlot's dial-success branch. Every connection is now dialed by DoRequest against a server that drops each connection at its first request while it is down. - TestH2ServerThatDropsEveryConnectionShowsErrors had no floor: a client that ignored the GOAWAY would wait on its only connection and pass with 1 connection and 0 errors. It now requires at least 3 connections and 1 error. - TestH2HeadersTooShortForTheirPriorityFieldsEndTheConnection tested only a 1-byte payload. It now has 4 bytes (a connection error), 5 bytes (an empty field block: a stream error, the connection goes on) and 6 bytes (a success). The scripted server gains a connection-window grant and counts the DATA it reads on streams it has reset. --- h2client_accounting_test.go | 382 ++++++++++++++++++++++++++++++------ 1 file changed, 326 insertions(+), 56 deletions(-) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 70b3ed5..bc1219c 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -61,17 +61,19 @@ type rawH2Server struct { killAfter time.Duration // > 0: the server closes each connection (no GOAWAY) this long after accepting it closeAfterData int // > 0: the server closes a connection (no GOAWAY) once it has read this many DATA bytes on it settings []http2.Setting // the server's SETTINGS (none: every default) + connWindow uint32 // > 0: grant the client this much more connection send window in the handshake answerUpgrade bool // answer the h2c upgrade request on stream 1, as celeris does (see rawH2Handshake) goAwayAtOnce bool // send GOAWAY as soon as the client's SETTINGS are acked, before any request - accepted atomic.Int64 // connections accepted - killed atomic.Int64 // connections the server ended (rawClose or killAfter) - clientClosed atomic.Int64 // connections the client ended first - dataSent atomic.Int64 // DATA frame payload bytes written, padding included - windowCredit atomic.Int64 // connection WINDOW_UPDATE increments received, minus each handshake's own - answeredLate atomic.Int64 // requests answered on a connection other than the first one - clientResets atomic.Int64 // RST_STREAM frames received from the client - clientCancel atomic.Int64 // of which with error code CANCEL + accepted atomic.Int64 // connections accepted + killed atomic.Int64 // connections the server ended (rawClose or killAfter) + clientClosed atomic.Int64 // connections the client ended first + dataSent atomic.Int64 // DATA frame payload bytes written, padding included + windowCredit atomic.Int64 // connection WINDOW_UPDATE increments received, minus each handshake's own + answeredLate atomic.Int64 // requests answered on a connection other than the first one + clientResets atomic.Int64 // RST_STREAM frames received from the client + clientCancel atomic.Int64 // of which with error code CANCEL + dataAfterReset atomic.Int64 // DATA payload bytes received on streams the server had reset (rawH2Conn.reset) mu sync.Mutex conns []net.Conn @@ -89,6 +91,7 @@ type rawH2Conn struct { requests int // request streams seen on this connection, the current one included held int // streams a handler started answering and left open dataRecv int // DATA payload bytes read on this connection + resetIDs map[uint32]bool } // rawH2Opts are the rawH2Server options of the same names. @@ -96,6 +99,7 @@ type rawH2Opts struct { killAfter time.Duration closeAfterData int settings []http2.Setting + connWindow uint32 answerUpgrade bool goAwayAtOnce bool } @@ -111,7 +115,7 @@ func startRawH2With(t *testing.T, opts rawH2Opts, h rawH2Handler) *rawH2Server { t.Fatal(err) } s := &rawH2Server{ln: ln, handler: h, killAfter: opts.killAfter, closeAfterData: opts.closeAfterData, settings: opts.settings, - answerUpgrade: opts.answerUpgrade, goAwayAtOnce: opts.goAwayAtOnce} + connWindow: opts.connWindow, answerUpgrade: opts.answerUpgrade, goAwayAtOnce: opts.goAwayAtOnce} s.wg.Add(1) go func() { defer s.wg.Done() @@ -170,7 +174,10 @@ func (s *rawH2Server) serve(nc net.Conn) { _ = nc.Close() return } - c := &rawH2Conn{srv: s, nc: nc, fr: fr, index: index} + if s.connWindow > 0 { + _ = fr.WriteWindowUpdate(0, s.connWindow) + } + c := &rawH2Conn{srv: s, nc: nc, fr: fr, index: index, resetIDs: map[uint32]bool{}} c.enc = hpack.NewEncoder(&c.hbuf) c.enc.SetMaxDynamicTableSizeLimit(0) // the client advertises SETTINGS_HEADER_TABLE_SIZE=0 dec := hpack.NewDecoder(4096, nil) @@ -212,6 +219,9 @@ func (s *rawH2Server) serve(nc net.Conn) { } case *http2.DataFrame: c.dataRecv += len(f.Data()) + if c.resetIDs[f.StreamID] { + s.dataAfterReset.Add(int64(len(f.Data()))) + } if s.closeAfterData > 0 && c.dataRecv >= s.closeAfterData { serverEnd() return @@ -315,8 +325,10 @@ func (c *rawH2Conn) tornData(streamID uint32, declared, sent int) { _, _ = c.nc.Write(append(hdr, make([]byte, sent)...)) } -// reset ends a stream with RST_STREAM(code). +// reset ends a stream with RST_STREAM(code). DATA the client sends on it +// afterwards is counted in dataAfterReset. func (c *rawH2Conn) reset(streamID uint32, code http2.ErrCode) { + c.resetIDs[streamID] = true _ = c.fr.WriteRSTStream(streamID, code) } @@ -1421,56 +1433,67 @@ func TestH2RedialBacksOffOnlyWhenTheServerLooksDown(t *testing.T) { } }) t.Run("grows until a connection serves", func(t *testing.T) { - cl := newClient(t) + // Every connection is dialed by DoRequest (reconnectSlot), as in a + // run: while down, the server completes each handshake and drops the + // connection at its first request, unanswered; up, it answers. + var up atomic.Bool + flaky := startRawH2(t, func(c *rawH2Conn, sid uint32, path string) rawH2Action { + if up.Load() { + return respondOK(c, sid, path) + } + return rawClose + }) + fh, fp := flaky.hostPort() + cl, err := newH2Client(fh, fp, "/", testH2Cfg("GET", nil, nil, 1, 1)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(cl.Close) + dials := countDials(cl, 0) slot := cl.conns[0] ctx := ctxFor(t, 5*time.Second) for round := 1; round <= 3; round++ { - if round > 1 { - hc, err := cl.redial() - if err != nil { - t.Fatal(err) - } - slot.cur.Store(hc) - } - endConn(slot, "the server dropped the connection before it answered anything") + // The request on the slot's connection (from round 2 on, one + // DoRequest dialed after the round before): the server drops it. if _, err := cl.DoRequest(ctx, 0); err == nil { - t.Fatalf("round %d: the request that found a connection which never answered succeeded (backoff.next=%v)", round, slot.backoff.next) + t.Fatalf("round %d: the request the server dropped unanswered succeeded", round) + } + // The next request finds a connection that never answered. + if _, err := cl.DoRequest(ctx, 0); !errors.Is(err, errH2NeverServed) { + t.Fatalf("round %d: the request that found a connection which never answered got %v, want errH2NeverServed", round, err) } want := min(reconnectBackoffMin< time.Second { - t.Errorf("the first backoff after a connection served took %v: the pace was not reset (want 5-10 ms)", d) + if d := time.Since(start); d > time.Second || slot.backoff.next != reconnectBackoffMin<<1 { + t.Errorf("the first backoff after a connection served took %v and left backoff.next=%v: the pace was not reset (want 5-10 ms and %v)", + d, slot.backoff.next, reconnectBackoffMin<<1) } }) t.Run("h2c: only the upgrade's own stream answered", func(t *testing.T) { @@ -1841,7 +1864,10 @@ func TestH2CUpgradeServerThatAnswersOnlyTheUpgradeIsPaced(t *testing.T) { // as errors and a handful of connections. On e37777e the pace restarted at // every dial that succeeded (44 and 46 connections in two runs), and whether // a request was an error or retried silently depended on whether it was -// written before the GOAWAY was read (43 and 24 errors). +// written before the GOAWAY was read (43 and 24 errors). The server never +// answers a request, so a client that ignored the GOAWAY would wait on its +// first connection for the whole run: 1 connection and no error, which the +// floor below fails (fixed code: 10 connections and 10-16 errors in 400 ms). func TestH2ServerThatDropsEveryConnectionShowsErrors(t *testing.T) { srv := startRawH2With(t, rawH2Opts{goAwayAtOnce: true}, func(*rawH2Conn, uint32, string) rawH2Action { return rawKeep @@ -1850,6 +1876,10 @@ func TestH2ServerThatDropsEveryConnectionShowsErrors(t *testing.T) { HTTP2: true, HTTP2Options: HTTP2Options{Connections: 1, MaxStreams: 4}}) accepted := srv.accepted.Load() t.Logf("connections=%d requests=%d errors=%d connect_errors=%d", accepted, res.Requests, res.Errors, res.ConnectErrors) + if accepted < 3 || res.Errors == 0 { + t.Errorf("connections=%d errors=%d, want at least 3 and 1: the server drops every connection at once, so the client must keep redialing it and count the outage, not wait on one connection (#89)", + accepted, res.Errors) + } if res.Errors < accepted-2 { t.Errorf("errors=%d over %d connections that each ended before answering anything: the outage is not counted", res.Errors, accepted) } @@ -1991,28 +2021,268 @@ func TestH2ResponseWithOnlyAnInterimStatusIsNotASuccess(t *testing.T) { // priority fields is a frame size error on a frame that carries a field // block, so a connection error (RFC 9113 §4.2, §6.2), as a Pad Length that // does not fit is since round 1. On e37777e it was read as an empty block: -// the stream failed with errH2NoStatus and the connection went on. +// the stream failed with errH2NoStatus and the connection went on. The +// boundary is exact: 4 bytes are too short, while 5 bytes are the priority +// fields and an empty field block, which is only a response without a +// :status (a stream error), and 6 carry :status 200. func TestH2HeadersTooShortForTheirPriorityFieldsEndTheConnection(t *testing.T) { - srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { - // PRIORITY flagged, 1 byte of payload: :status 200 where the 5 priority bytes should be. - _ = c.fr.WriteRawFrame(http2.FrameHeaders, http2.FlagHeadersEndHeaders|http2.FlagHeadersEndStream|http2.FlagHeadersPriority, sid, []byte{0x88}) + priority := []byte{0, 0, 0, 0, 15} // stream dependency 0, weight 16 + for _, tc := range []struct { + name string + payload []byte + connError bool // a connection error; else the connection goes on + success bool + }{ + {"1 byte: :status 200 where the priority fields should be", []byte{0x88}, true, false}, + {"4 bytes", priority[:4], true, false}, + {"5 bytes: the priority fields, an empty field block", priority, false, false}, + {"6 bytes: the priority fields, :status 200", append(append([]byte{}, priority...), 0x88), false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := startRawH2(t, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + _ = c.fr.WriteRawFrame(http2.FrameHeaders, http2.FlagHeadersEndHeaders|http2.FlagHeadersEndStream|http2.FlagHeadersPriority, sid, tc.payload) + return rawKeep + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 1)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + hc := cl.conns[0].cur.Load() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + _, err = cl.DoRequest(ctx, 0) + t.Logf("%d-byte payload: err=%v connection closed=%t", len(tc.payload), err, hc.closed.Load()) + if tc.success != (err == nil) { + t.Errorf("err=%v, want a success=%t", err, tc.success) + } + if hc.closed.Load() != tc.connError { + t.Errorf("connection closed=%t after a HEADERS frame flagged PRIORITY with a %d-byte payload (err=%v), want %t: only fewer than 5 bytes are a connection error", + hc.closed.Load(), len(tc.payload), err, tc.connError) + } + }) + } +} + +// --------------------------------------------------------------------------- +// Review of the fixes, round 3 (at 147edb4) + +// TestH2BodyStopsAtTheServersReset: once the server resets a stream, the +// client sends no more frames on it (RFC 9113 §6.4, §5.1 "closed"), however +// much send window the body still has. The server grants 128 MiB of window, +// resets a 32 MiB upload at its HEADERS and reads nothing for 300 ms, so the +// client's writer can be ahead of the reset by no more than the socket +// buffers. The second request's HEADERS reach the server after every DATA +// frame the client wrote for the first stream: the server counts those at +// that moment. On 147edb4 the writer checked for the reset only once the +// window ran out, so it sent the whole body into a stream that no longer +// existed (a Go server answers each such frame with RST_STREAM(STREAM_CLOSED)). +func TestH2BodyStopsAtTheServersReset(t *testing.T) { + const body = 32 << 20 + var afterFirst atomic.Int64 // DATA bytes of the first stream the server read before the second stream's HEADERS + srv := startRawH2With(t, rawH2Opts{ + settings: []http2.Setting{{ID: http2.SettingInitialWindowSize, Val: 128 << 20}}, + connWindow: 128 << 20, + }, func(c *rawH2Conn, sid uint32, _ string) rawH2Action { + if c.requests == 2 { + afterFirst.Store(c.srv.dataAfterReset.Load()) + } + c.reset(sid, http2.ErrCodeRefusedStream) + if c.requests == 1 { + time.Sleep(300 * time.Millisecond) // let the client read the reset while its writer is still in the body + } return rawKeep }) host, port := srv.hostPort() - cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 1)) + cl, err := newH2Client(host, port, "/upload", testH2Cfg("POST", nil, make([]byte, body), 1, 4)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + for i := range 2 { + var re *HTTP2ResetError + if _, err := cl.DoRequest(ctx, 0); !errors.As(err, &re) { + t.Fatalf("request %d: got %v, want the server's RST_STREAM(REFUSED_STREAM)", i+1, err) + } + } + sent := afterFirst.Load() + t.Logf("DATA bytes the client sent on the first stream after the server reset it: %d of the %d-byte body; client RST_STREAM=%d connections=%d", + sent, body, srv.clientResets.Load(), srv.accepted.Load()) + if sent >= body/2 { + t.Errorf("the client sent %d of the body's %d bytes on a stream the server had reset: after RST_STREAM it must send no more frames on it (RFC 9113 §6.4)", sent, body) + } + if n := srv.clientResets.Load(); n != 0 { + t.Errorf("the client sent %d RST_STREAM: never one for a stream the server reset (RFC 9113 §5.4.2)", n) + } + if n := srv.accepted.Load(); n != 1 { + t.Errorf("connections=%d, want 1: a stream the server reset is not a connection error", n) + } +} + +// TestH2ConnectionOutOfStreamIDsIsFailedWhenItDies: a connection whose stream +// IDs have run out takes no new request (closed), but its last streams are +// still in flight. If the server then closes it, or a write to it fails, the +// connection has died: its streams must fail and done must close, as on any +// other connection. No request comes to redial it here (the requests are +// handed to the writer directly), so nothing else closes it. On 147edb4 +// readLoop and writeFailed took closed for the client's own close and +// returned without failing it: the stream in flight waited for the rest of +// the run. +func TestH2ConnectionOutOfStreamIDsIsFailedWhenItDies(t *testing.T) { + for _, tc := range []struct { + name string + failWrites bool // a write fails; else the server closes the connection + }{ + {"the server closes it", false}, + {"a write to it fails", true}, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.failWrites { + dial := dialTimeoutFunc + t.Cleanup(func() { dialTimeoutFunc = dial }) + dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { + c, err := dial(network, addr, timeout) + if err != nil { + return nil, err + } + return &failWritesConn{Conn: c, okWrites: 4}, nil // the handshake's 3 writes and the last stream's HEADERS + } + } + held := make(chan *rawH2Conn, 1) + srv := startRawH2(t, func(c *rawH2Conn, _ uint32, _ string) rawH2Action { + held <- c // never answer + return rawKeep + }) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 4)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(cl.Close) + hc := cl.conns[0].cur.Load() + hc.nextStreamID.Store(0x7FFFFFFF) // the last stream ID a connection may use + last := make(chan h2Response, 1) + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: cl.headerBlock, respCh: &last} + var sc *rawH2Conn + select { + case sc = <-held: + case <-time.After(3 * time.Second): + t.Fatal("the stream with the last ID never reached the server") + } + next := make(chan h2Response, 1) + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: cl.headerBlock, respCh: &next} + select { + case r := <-next: + if r.err != errH2NotSent { + t.Fatalf("the request after the last stream ID was answered %+v, want errH2NotSent", r) + } + case <-time.After(3 * time.Second): + t.Fatal("the request after the last stream ID was never answered") + } + if !hc.closed.Load() { + t.Fatal("a connection out of stream IDs still takes new requests") + } + if tc.failWrites { + _ = sc.fr.WritePing(false, [8]byte{1}) // the client's PING ACK is its next write, which fails + } else { + _ = sc.nc.Close() + } + select { + case r := <-last: + t.Logf("the stream in flight was answered: %v", r.err) + if r.err == nil { + t.Errorf("the stream in flight on a connection that died was a success: %+v", r) + } + case <-time.After(3 * time.Second): + t.Fatal("the stream in flight was never answered after its connection died: a connection out of stream IDs was taken for one the client closed, and nothing failed it") + } + select { + case <-hc.done: + case <-time.After(time.Second): + t.Error("done never closed on a connection that died") + } + }) + } +} + +// heldReadConn holds each read, once armed, after the bytes are read from +// the socket and before the reader gets them, until release is closed: the +// reader then parses a response that had arrived before the connection +// ended. +type heldReadConn struct { + net.Conn + armed atomic.Bool + holding chan struct{} // closed when an armed read holds bytes + once sync.Once + release chan struct{} +} + +func (c *heldReadConn) Read(p []byte) (int, error) { + n, err := c.Conn.Read(p) + if n > 0 && c.armed.Load() { + c.once.Do(func() { close(c.holding) }) + <-c.release + } + return n, err +} + +// TestH2RedialTakesTheResponseTheConnectionHadReceived: the writer ends a +// connection (a failed write: failConn) just after the server's first +// response reached the client, and before readLoop has parsed it. That +// connection has served, so the request that finds it gone redials at once. +// reconnectSlot must read served only once readLoop has returned, having +// parsed what it had read. On 147edb4 it read served at once: the connection +// looked as if it had never served, so the request failed (errH2NeverServed) +// and slept the backoff, and nothing was dialed. +func TestH2RedialTakesTheResponseTheConnectionHadReceived(t *testing.T) { + dial := dialTimeoutFunc + t.Cleanup(func() { dialTimeoutFunc = dial }) + gate := &heldReadConn{holding: make(chan struct{}), release: make(chan struct{})} + var dialed atomic.Int64 + dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { + c, err := dial(network, addr, timeout) + if err != nil || dialed.Add(1) > 1 { + return c, err + } + gate.Conn = c + return gate, nil + } + srv := startRawH2(t, respondOK) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 4)) if err != nil { t.Fatal(err) } defer cl.Close() hc := cl.conns[0].cur.Load() - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + gate.armed.Store(true) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - _, err = cl.DoRequest(ctx, 0) - t.Logf("err=%v connection closed=%t", err, hc.closed.Load()) - if err == nil { - t.Fatal("a HEADERS frame too short for its priority fields was a success") + first := make(chan error, 1) + go func() { + _, err := cl.DoRequest(ctx, 0) + first <- err + }() + select { + case <-gate.holding: + case <-time.After(3 * time.Second): + close(gate.release) + t.Fatal("the server's first response never reached the client") } - if !hc.closed.Load() { - t.Errorf("the connection goes on after a HEADERS frame too short for its priority fields (err=%v): it is a connection error", err) + hc.failConn(errors.New("test: a write to the connection failed")) // as writeFailed does + t.Logf("the request whose response was still unparsed: %v", <-first) + go func() { + time.Sleep(100 * time.Millisecond) + close(gate.release) + }() + dials := countDials(cl, 0) + start := time.Now() + _, err = cl.DoRequest(ctx, 0) + t.Logf("the request that found the connection gone: %v, err=%v, dials=%d, served=%t", time.Since(start), err, dials.Load(), hc.served.Load()) + if err != nil || dials.Load() != 1 { + t.Errorf("err=%v dials=%d, want a success after 1 dial: the connection had received the answer to a request, so it served, and the request that finds it gone redials at once", err, dials.Load()) } } From 1133fb833af3860e9df348f4e84e9645106e2fce Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 11:19:04 +0200 Subject: [PATCH 24/31] fix(h2client): the request body stops at the server's RST_STREAM, not when the window runs out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 9113 §6.4: after receiving RST_STREAM, an endpoint sends no more frames on that stream except PRIORITY. Round 2 (ebb34d9) records the reset (curStreamReset), but writeBodyFlowControlled read it only once the send window was exhausted: with window left, it wrote the rest of the body into a stream that no longer existed. A Go server answers each such DATA frame with RST_STREAM(STREAM_CLOSED). The writer now checks for the reset before each DATA frame and stops there, sending no RST_STREAM of its own (§5.4.2). What it had already buffered or flushed before readLoop read the reset still goes out, as the RFC allows for frames sent before the reset arrived. Pre-existing in effect on main (which had no reset check at all). No published cell reaches it: post-4k-h2's body goes out with its HEADERS before any reply can arrive. --- h2client.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/h2client.go b/h2client.go index 26fd141..63d17d1 100644 --- a/h2client.go +++ b/h2client.go @@ -191,7 +191,8 @@ type h2Conn struct { // 64KiB body (post-64k-h2 = 65536 B) exceeds the 65535 window by one byte // and the request hangs until the run ends. curStreamReset is the ID of // that stream once the server has reset it (RST_STREAM): the body writer - // then stops, and sends no RST_STREAM of its own (RFC 9113 §5.4.2). + // then stops before its next DATA frame, whatever window is left (RFC + // 9113 §6.4), and sends no RST_STREAM of its own (§5.4.2). connSendWindow atomic.Int64 curStreamID atomic.Uint32 curStreamWindow atomic.Int64 @@ -1287,7 +1288,7 @@ var errH2BodyAbandoned = errors.New("h2client: request body abandoned: the serve // the connection is closed (done), which Close does at the end of a run and // failConn when the connection dies, or when the server has ended the stream // (st no longer holds respCh): the backstops against a peer that never grants -// window. +// window. A stream the server has reset gets no further DATA frame at all. // // Without this, a 64KiB body (post-64k-h2 = 65536 B) either trips FRAME_SIZE_ERROR // (64KiB DATA frame vs a 16384-default server) or overruns the 65535 window. @@ -1337,6 +1338,12 @@ func (hc *h2Conn) writeBodyFlowControlled(streamID uint32, st *h2StreamSlot, res time.Sleep(50 * time.Microsecond) continue } + // The server has reset the stream: send no more frames on it (RFC + // 9113 §6.4), however much window is left. readLoop records the + // reset before it answers the stream's worker. + if hc.curStreamReset.Load() == streamID { + return errH2BodyAbandoned + } if avail > len(data) { avail = len(data) } From 599d3ca85c6d61d0183e52a7b2663f6abaec9ea3 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 10:22:57 +0200 Subject: [PATCH 25/31] fix(h2client): a connection out of stream IDs that dies is failed like any other (#88) Since round 1 (5ff041c) the writer marks a connection whose stream IDs have run out closed, so it takes no new request, while its last streams are still in flight; DoRequest redials the slot and closes it. readLoop (on a read error) and writeFailed read closed as "the client closed this connection" and returned without failing it. Had the server closed such a connection, or a write to it failed, before a request came to redial it, its streams in flight would have waited until the run ended. Both now ask whether closeConn has run (done is closed): closing(). A connection that closed itself to new requests and then dies is failed: its streams are errors, and done closes. Needs 2^30 streams on one connection and a death before the next request reaches reconnectSlot, so no cell reaches it; the request that runs out of IDs is retried at once and closes the connection in reconnectSlot. --- h2client.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/h2client.go b/h2client.go index 63d17d1..47455d5 100644 --- a/h2client.go +++ b/h2client.go @@ -218,7 +218,9 @@ type h2Conn struct { addr string // closed marks a connection that takes no new request: DoRequest - // redials the slot instead. Set before done is closed. + // redials the slot instead. Set before done is closed, and on its own + // when the connection's stream IDs run out (its last streams are still + // in flight then): closing, not closed, says the client closed it. closed atomic.Bool // served is set by readLoop once the connection carries a response // frame for one of the run's requests (not the h2c upgrade's own stream @@ -852,11 +854,25 @@ func (hc *h2Conn) loopExited() { // this the bufio.Writer keeps the error and fails every later request on the // dead connection at once, a spin of errors that never redials (#89). func (hc *h2Conn) writeFailed(err error) { - if err != nil && !hc.closed.Load() { + if err != nil && !hc.closing() { hc.failConn(fmt.Errorf("connection error: %w", err)) } } +// closing reports whether closeConn has run: the client closed the +// connection (Close, reconnectSlot), or failConn is ending it. closed alone +// does not say so: a connection whose stream IDs ran out is closed to new +// requests while its last streams are still in flight, and if it then dies, +// readLoop and writeFailed must fail it like any other. +func (hc *h2Conn) closing() bool { + select { + case <-hc.done: + return true + default: + return false + } +} + func (hc *h2Conn) flushWindowUpdate() { if pending := hc.pendingConnWindow.Swap(0); pending > 0 { _ = hc.framer.WriteWindowUpdate(0, pending) @@ -958,8 +974,8 @@ func (hc *h2Conn) readLoop() { for { frame, err := hc.framer.ReadFrame() if err != nil { - if hc.closed.Load() { - return // the client closed it (Close, or reconnectSlot replacing it) + if hc.closing() { + return // the client closed it (Close, or reconnectSlot replacing it), or failConn is ending it } // The server or the network ended the connection without a // GOAWAY: a close, a reset, a timeout. Fail it, so its in-flight From f2e3eae0fc5af288cf46e2a50dba02541b733f18 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 10:23:12 +0200 Subject: [PATCH 26/31] fix(h2client): reconnectSlot reads served once readLoop has parsed what it had read (#89) New in round 2 (0c005df): reconnectSlot decides from old.served whether the connection it replaces ever answered a request. When the writer ends a connection (a failed write: writeFailed -> failConn), readLoop can still be parsing frames it read before the close, the connection's first response among them, and marks it served only then. reconnectSlot read served at once, so such a connection counted as never served: the request that found it failed (errH2NeverServed), slept the backoff, and dialed nothing. reconnectSlot now waits, after closing the old connection, until both of its loops have returned (loopsDone), or the run's context ends. Both return soon after the close: readLoop's next read fails on the closed socket, and writeLoop stops on done. Only a connection that ended at its very first response is affected: one error and one backoff sleep fewer. --- h2client.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/h2client.go b/h2client.go index 47455d5..29842e3 100644 --- a/h2client.go +++ b/h2client.go @@ -499,6 +499,14 @@ func (c *h2Client) reconnectSlot(ctx context.Context, slot *h2ConnSlot) (*h2Conn var err error if old != nil { old.closeConn() // release fds/goroutines of the dead conn + // served is final once readLoop has returned: when the writer ended + // the connection, readLoop may still be parsing frames it had read, + // the connection's first response among them. Both loops return soon + // after the close. + select { + case <-old.loopsDone: + case <-ctx.Done(): + } if old.served.Load() { slot.backoff.reset() // the server was up: redial at once, and the pace starts over } else { From 111c42d549ce1cb4d51d3dd7202cd5721cba84f8 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 11:12:21 +0200 Subject: [PATCH 27/31] test(h2client): failing test for two awaits draining one dead connection's queue Round-3 review nit on 90a4e2b: once a connection's loops have returned, every await on it calls answerQueued, which takes each queued request off writeCh and answers it errH2NotSent. An await that finds the queue empty and its own channel empty returns "connection closing". Between another await taking its request off the queue and answering it, that decision is wrong: the request never reached the server, and it is counted as an error instead of being retried. TestH2AwaitAnswersEachQueuedRequestOnce puts 8 never-written requests on each of 20,000 dead connections and runs their 8 awaits at once. On 147edb4's code a probe of this shape counted 913 of 160,000 answered as errors under -race (99-104 without). --- h2client_accounting_test.go | 47 +++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index bc1219c..06a401b 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -2286,3 +2286,50 @@ func TestH2RedialTakesTheResponseTheConnectionHadReceived(t *testing.T) { t.Errorf("err=%v dials=%d, want a success after 1 dial: the connection had received the answer to a request, so it served, and the request that finds it gone redials at once", err, dials.Load()) } } + +// TestH2AwaitAnswersEachQueuedRequestOnce: requests still queued for a +// connection whose writer has returned are answered by whichever await runs +// answerQueued first; several awaits on one dead connection drain the queue +// together. An await that finds the queue empty and its own channel empty +// must not decide that nobody answered it while another await has taken its +// request off the queue and not yet answered it: that request never reached +// the server, so it is errH2NotSent (retried), not "connection closing" (an +// error). 8 awaits per dead connection, 20000 connections. On +// 147edb4 a probe of this shape counted 913 of 160,000 such requests +// answered as errors under -race, and 99-104 without. +func TestH2AwaitAnswersEachQueuedRequestOnce(t *testing.T) { + const trials, waiters = 20000, 8 + var miscounted, total int64 + for range trials { + hc := &h2Conn{done: make(chan struct{}), loopsDone: make(chan struct{}), streamSem: make(chan struct{}, waiters), + writeCh: make(chan h2WriteReq, waiters)} + close(hc.done) + close(hc.loopsDone) + chans := make([]chan h2Response, waiters) + for i := range chans { + chans[i] = make(chan h2Response, 1) + hc.writeCh <- h2WriteReq{kind: h2WriteHeaders, block: []byte{0x82}, respCh: &chans[i]} + } + var wg sync.WaitGroup + var bad atomic.Int64 + start := make(chan struct{}) + for i := range chans { + wg.Add(1) + go func() { + defer wg.Done() + <-start + if _, err := hc.await(context.Background(), &chans[i], 0); err != errH2NotSent { + bad.Add(1) + } + }() + } + close(start) + wg.Wait() + miscounted += bad.Load() + total += waiters + } + t.Logf("requests never written, answered as errors: %d of %d", miscounted, total) + if miscounted != 0 { + t.Errorf("%d of %d requests that were never written were answered \"connection closing\": an await gave up while another await held its request, taken off the queue and not yet answered", miscounted, total) + } +} From d69d9c3aead8631164c6722899078063c9222e93 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 11:12:40 +0200 Subject: [PATCH 28/31] fix(h2client): an await on a dead connection waits for a request another await is answering (#89) 90a4e2b made every await on a connection whose loops have returned drain writeCh itself (answerQueued), so a request queued after the writer's last drain is retried rather than counted as an error. answerQueued takes a request off the queue and then answers it. Between the two, the request's own await could find the queue empty and its channel empty, and return "connection closing": an error for a request that never reached the server. answerQueued now runs under a per-connection mutex (queueMu): taking a request off the queue and answering it is one step for its callers. Once an await's own answerQueued returns, every request anyone took off the queue has been answered, so its non-blocking receive decides correctly. Only closed connections take the lock; the request path does not. --- h2client.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/h2client.go b/h2client.go index 29842e3..80b37c1 100644 --- a/h2client.go +++ b/h2client.go @@ -216,6 +216,13 @@ type h2Conn struct { loopsDone chan struct{} loopsLeft atomic.Int32 + // queueMu makes taking a request off writeCh and answering it one step + // for answerQueued's callers, the writer's last drain and every await on + // a closed connection: once an await's own answerQueued returns, any + // request someone else took off the queue has been answered, its own + // among them, so it can decide that it got no answer. + queueMu sync.Mutex + addr string // closed marks a connection that takes no new request: DoRequest // redials the slot instead. Set before done is closed, and on its own @@ -834,8 +841,11 @@ func (hc *h2Conn) writeLoop() { // answerQueued answers every request still queued for a closed connection's // writer. None of them was written, so none reached the server: each goes // to the redialed connection (errH2NotSent), as a request still waiting for -// a stream does, instead of counting as an error. +// a stream does, instead of counting as an error. Callers take turns +// (queueMu), so each request is off the queue only once it is answered. func (hc *h2Conn) answerQueued() { + hc.queueMu.Lock() + defer hc.queueMu.Unlock() for { select { case req := <-hc.writeCh: @@ -1265,7 +1275,8 @@ func (hc *h2Conn) await(ctx context.Context, chPtr *chan h2Response, idx int) (i // never written: roundTrip's hand-off can win its race with done // after the writer's final drain. Answer those requests too, this // one among them (each is taken off the queue once, so answered - // once). + // once). Another await may have taken this one: answerQueued waits + // for it to answer, so the receive below cannot miss that answer. hc.answerQueued() select { case resp := <-*chPtr: From 629c904965b7b69a04813155c6bf4f1c07432712 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 13:00:52 +0200 Subject: [PATCH 29/31] test(h2client): failing test for a redial that starts after the run has ended reconnectSlot's doc says it dials nothing after the run's context ends, and its first check is there for that. The wait for the old connection's loops (fb092cc) falls through on the context's end and then dials: the cancel can come during that wait, or after the first check with the loops already returned, when select takes either ready case. On 0af01b3 all three subtests fail: 1 dial with the server up, 1 dial and 1 connect error with it down, and a dial on the first of 64 attempts in the direct case. --- h2client_accounting_test.go | 132 ++++++++++++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 06a401b..3b7a5c0 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -2287,6 +2287,138 @@ func TestH2RedialTakesTheResponseTheConnectionHadReceived(t *testing.T) { } } +// endsAfterFirstCheck is a context that ends as its first Err returns: the +// run's context ending just after a check of it. That first Err closes Done +// and returns nil; every later Err returns context.Canceled. +type endsAfterFirstCheck struct { + context.Context + done chan struct{} + checked atomic.Bool +} + +func newEndsAfterFirstCheck() *endsAfterFirstCheck { + return &endsAfterFirstCheck{Context: context.Background(), done: make(chan struct{})} +} + +func (c *endsAfterFirstCheck) Done() <-chan struct{} { return c.done } + +func (c *endsAfterFirstCheck) Err() error { + if c.checked.CompareAndSwap(false, true) { + close(c.done) + return nil + } + return context.Canceled +} + +// TestH2RedialDialsNothingOnceTheRunHasEnded: once the run's context has +// ended, reconnectSlot dials nothing, as its doc says. Close runs after the +// cancel and would not close a connection dialed then, and a dial that fails +// then is a connect error in the Result of a run that had already ended +// (bench.go takes snapshotConnectErrors after the workers return). The +// context can end while reconnectSlot waits for the old connection's loops +// (readLoop still parsing what it had read), or after reconnectSlot's first +// check and before that wait, when the loops may already have returned: the +// wait's two cases are then both ready and select takes either. On 0af01b3 +// the wait fell through on the cancel and dialed: 1 dial, and 1 connect error +// when the server was down. +func TestH2RedialDialsNothingOnceTheRunHasEnded(t *testing.T) { + for _, down := range []bool{false, true} { + name := "the run ends during the wait for the loops, server up" + if down { + name = "the run ends during the wait for the loops, server down" + } + t.Run(name, func(t *testing.T) { + dial := dialTimeoutFunc + t.Cleanup(func() { dialTimeoutFunc = dial }) + gate := &heldReadConn{holding: make(chan struct{}), release: make(chan struct{})} + var dialed atomic.Int64 + dialTimeoutFunc = func(network, addr string, timeout time.Duration) (net.Conn, error) { + c, err := dial(network, addr, timeout) + if err != nil || dialed.Add(1) > 1 { + return c, err + } + gate.Conn = c + return gate, nil + } + srv := startRawH2(t, respondOK) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 4)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + var released sync.Once + release := func() { released.Do(func() { close(gate.release) }) } + defer release() + live, stop := context.WithTimeout(context.Background(), 5*time.Second) + defer stop() + hc := cl.conns[0].cur.Load() + if _, err := cl.DoRequest(live, 0); err != nil || !hc.served.Load() { + t.Fatalf("first request: err=%v served=%t, want a success", err, hc.served.Load()) + } + // readLoop holds the second response's bytes until release, so + // the connection's loops outlast its end. + gate.armed.Store(true) + go func() { _, _ = cl.DoRequest(live, 1) }() + select { + case <-gate.holding: + case <-time.After(3 * time.Second): + t.Fatal("the server's second response never reached the client") + } + hc.failConn(errors.New("test: a write to the connection failed")) // as writeFailed does + if down { + _ = srv.ln.Close() + } + dials := countDials(cl, 0) + before := connectErrorsCounter.Swap(0) + defer connectErrorsCounter.Add(before) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + time.AfterFunc(50*time.Millisecond, cancel) // the run ends + _, err = cl.DoRequest(ctx, 0) + connectErrors := connectErrorsCounter.Swap(0) + t.Logf("the request that found the connection gone as the run ended: err=%v dials=%d connect_errors=%d", err, dials.Load(), connectErrors) + if !errors.Is(err, context.Canceled) || dials.Load() != 0 || connectErrors != 0 { + t.Errorf("err=%v dials=%d connect_errors=%d, want context.Canceled, 0 and 0: the run ended while reconnectSlot waited for the old connection's loops, and a slot dials nothing after the run's end", + err, dials.Load(), connectErrors) + } + }) + } + + t.Run("the run ends just before the wait, the loops already returned", func(t *testing.T) { + srv := startRawH2(t, respondOK) + host, port := srv.hostPort() + cl, err := newH2Client(host, port, "/", testH2Cfg("GET", nil, nil, 1, 4)) + if err != nil { + t.Fatal(err) + } + defer cl.Close() + live, stop := context.WithTimeout(context.Background(), 5*time.Second) + defer stop() + slot := cl.conns[0] + hc := slot.cur.Load() + if _, err := cl.DoRequest(live, 0); err != nil || !hc.served.Load() { + t.Fatalf("first request: err=%v served=%t, want a success", err, hc.served.Load()) + } + hc.failConn(errors.New("test: a write to the connection failed")) + select { + case <-hc.loopsDone: + case <-time.After(3 * time.Second): + t.Fatal("the connection's loops never returned after it ended") + } + dials := countDials(cl, 0) + // Both of the wait's cases are ready and select takes either at + // random, so a check made in only one of them dials half the time. + const attempts = 64 + for i := range attempts { + if _, err := cl.reconnectSlot(newEndsAfterFirstCheck(), slot); !errors.Is(err, context.Canceled) || dials.Load() != 0 { + t.Fatalf("attempt %d of %d: err=%v dials=%d, want context.Canceled and no dial: the run ended after reconnectSlot's first check of it, and a slot dials nothing after the run's end", + i+1, attempts, err, dials.Load()) + } + } + }) +} + // TestH2AwaitAnswersEachQueuedRequestOnce: requests still queued for a // connection whose writer has returned are answered by whichever await runs // answerQueued first; several awaits on one dead connection drain the queue From f14208ad0c934074a7f6b263df936bcb05d9e567 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 13:01:17 +0200 Subject: [PATCH 30/31] fix(h2client): reconnectSlot dials nothing once the run has ended, also after its wait for the loops (#89) fb092cc made reconnectSlot wait for the old connection's loops before it reads served. The wait ends on the loops or on the run's context, and on the context it fell through to the dial: a dial after the run's end, which Close (it runs after the cancel) would not close, and whose failure is a connect error in the Result of a run that had ended. Check the context after the wait, not only in its ctx case: when the run ends just after reconnectSlot's first check with the loops already returned, both cases are ready and select takes either. --- h2client.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/h2client.go b/h2client.go index 80b37c1..0023094 100644 --- a/h2client.go +++ b/h2client.go @@ -514,6 +514,13 @@ func (c *h2Client) reconnectSlot(ctx context.Context, slot *h2ConnSlot) (*h2Conn case <-old.loopsDone: case <-ctx.Done(): } + // Checked after the wait, not in its ctx case only: the run may have + // ended with the loops already returned, and select then takes + // either case. + if ctx.Err() != nil { + slot.mu.Unlock() + return nil, ctx.Err() + } if old.served.Load() { slot.backoff.reset() // the server was up: redial at once, and the pace starts over } else { From f9052f6c39e8b1c8011a43db523581aa64db0ed9 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 13:02:12 +0200 Subject: [PATCH 31/31] test(h2client): the GOAWAY test's doc gives the measured range, 10-11 connections and 10-17 errors in 42 runs --- h2client_accounting_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/h2client_accounting_test.go b/h2client_accounting_test.go index 3b7a5c0..99c6257 100644 --- a/h2client_accounting_test.go +++ b/h2client_accounting_test.go @@ -1867,7 +1867,8 @@ func TestH2CUpgradeServerThatAnswersOnlyTheUpgradeIsPaced(t *testing.T) { // written before the GOAWAY was read (43 and 24 errors). The server never // answers a request, so a client that ignored the GOAWAY would wait on its // first connection for the whole run: 1 connection and no error, which the -// floor below fails (fixed code: 10 connections and 10-16 errors in 400 ms). +// floor below fails. Fixed code, 42 runs: 10-11 connections and 10-17 errors +// in 400 ms. func TestH2ServerThatDropsEveryConnectionShowsErrors(t *testing.T) { srv := startRawH2With(t, rawH2Opts{goAwayAtOnce: true}, func(*rawH2Conn, uint32, string) rawH2Action { return rawKeep