From d736f0703d44a6ece09b02c46c351ce9c88111c9 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 14:22:05 +0200 Subject: [PATCH 01/20] refactor(epoll): move the dirty-list pass into Loop.flushDirty (no behaviour change) Pure code motion, so a unit test can drive the pass the run loop runs once per iteration (celeris#669). The body is byte-identical after the one-tab de-indent. --- engine/epoll/loop.go | 100 +++++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 46 deletions(-) diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index a54f8ab2..d3073b5a 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -675,52 +675,7 @@ func (l *Loop) run(ctx context.Context) { l.drainAdoptQueue(ctx, an) } - for cs := l.dirtyHead; cs != nil; { - next := cs.dirtyNext - if mu := cs.detachMu; mu != nil { - mu.Lock() - } - err := l.flushWrites(cs, true) - if err != nil { - // Surface I/O failure to detached middleware before closing. - if cs.h1State != nil && cs.h1State.OnError != nil { - cs.h1State.OnError(err) - } - if mu := cs.detachMu; mu != nil { - mu.Unlock() - } - l.removeDirty(cs) - l.closeConn(cs.fd) - } else if !csWritePending(cs) { - cs.pendingBytes = 0 - if mu := cs.detachMu; mu != nil { - mu.Unlock() - } - l.removeDirty(cs) - // Deferred peer-close (EPOLLRDHUP arrived mid-response): now flushed. - if cs.peerClosed { - l.closeConn(cs.fd) - } - } else { - // Partial write: kernel send buffer full. Sync pendingBytes, - // then for a non-detached HTTP/H2 conn hand off to level- - // triggered EPOLLOUT (armEpollOut removes it from the dirty - // list) so the loop stops busy-retrying it. Truly-detached - // WS/SSE conns stay on the dirty list — their writes are - // goroutine-driven and re-signalled via the eventfd path. - // `next` was captured above, so the removeDirty inside - // armEpollOut is safe mid-iteration. - cs.pendingBytes = csPendingBytes(cs) - detachedWS := cs.h1State != nil && cs.h1State.Detached.Load() - if mu := cs.detachMu; mu != nil { - mu.Unlock() - } - if !detachedWS { - l.armEpollOut(cs) - } - } - cs = next - } + l.flushDirty() // Drain H2 async write queues. Handler goroutines enqueue response // frame bytes; we drain them into writeBuf and flush to the wire. @@ -2448,6 +2403,59 @@ func (l *Loop) drainDetachQueue() { l.detachQSpare = l.detachQSpare[:0] } +// flushDirty is the event loop's dirty-list pass, run once per iteration +// after drainDetachQueue: flush every connection with bytes still queued, +// close the ones whose flush failed, and hand a still-partial non-detached +// conn to level-triggered EPOLLOUT. Loop thread only. +func (l *Loop) flushDirty() { + for cs := l.dirtyHead; cs != nil; { + next := cs.dirtyNext + if mu := cs.detachMu; mu != nil { + mu.Lock() + } + err := l.flushWrites(cs, true) + if err != nil { + // Surface I/O failure to detached middleware before closing. + if cs.h1State != nil && cs.h1State.OnError != nil { + cs.h1State.OnError(err) + } + if mu := cs.detachMu; mu != nil { + mu.Unlock() + } + l.removeDirty(cs) + l.closeConn(cs.fd) + } else if !csWritePending(cs) { + cs.pendingBytes = 0 + if mu := cs.detachMu; mu != nil { + mu.Unlock() + } + l.removeDirty(cs) + // Deferred peer-close (EPOLLRDHUP arrived mid-response): now flushed. + if cs.peerClosed { + l.closeConn(cs.fd) + } + } else { + // Partial write: kernel send buffer full. Sync pendingBytes, + // then for a non-detached HTTP/H2 conn hand off to level- + // triggered EPOLLOUT (armEpollOut removes it from the dirty + // list) so the loop stops busy-retrying it. Truly-detached + // WS/SSE conns stay on the dirty list — their writes are + // goroutine-driven and re-signalled via the eventfd path. + // `next` was captured above, so the removeDirty inside + // armEpollOut is safe mid-iteration. + cs.pendingBytes = csPendingBytes(cs) + detachedWS := cs.h1State != nil && cs.h1State.Detached.Load() + if mu := cs.detachMu; mu != nil { + mu.Unlock() + } + if !detachedWS { + l.armEpollOut(cs) + } + } + cs = next + } +} + func (l *Loop) markDirty(cs *connState) { if cs.dirty { return From 17b8038e5873a824de2c9546dd3c9f8a64bb9bd3 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 14:28:45 +0200 Subject: [PATCH 02/20] test(epoll): failing-first rigs for celeris#669 and celeris#668 celeris#669 (async_handler_stall_linux_test.go): with AsyncHandlers the dispatch goroutine holds cs.detachMu for the whole handler, and four loop-thread sites take it with a blocking Lock -- closeConn (the timeout reap, EPOLLRDHUP, EPOLLHUP), drainRead's EOF/error branches, the dirty pass and the EPOLLOUT resume. Each unit arm holds the lock as a running handler does and requires the site to return; the negative control (a parked goroutine, i.e. a bounded holder) requires closeConn to keep waiting. The two end-to-end arms are the issue's measurement: an 800 ms async handler and a fast keep-alive conn pinned to the same loop, with the reap (ReadTimeout 100 ms) or a client half-close as the trigger. celeris#668 (hijack_offthread_linux_test.go): an off-thread hijack while the reap or the post-switch sweep walks liveConns (a data race under -race, an ownership failure without it), a guard for the fd-reuse aliasing a deferred live-set removal must survive, and the issue's counter-level witness: async hijacks under accept churn, then every loop must reach SUSPENDED with connCount 0. --- .../epoll/async_handler_stall_linux_test.go | 505 ++++++++++++++++++ engine/epoll/hijack_offthread_linux_test.go | 425 +++++++++++++++ engine/epoll/livecs_linux_test.go | 13 + 3 files changed, 943 insertions(+) create mode 100644 engine/epoll/async_handler_stall_linux_test.go create mode 100644 engine/epoll/hijack_offthread_linux_test.go create mode 100644 engine/epoll/livecs_linux_test.go diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go new file mode 100644 index 00000000..c39a8b4d --- /dev/null +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -0,0 +1,505 @@ +//go:build linux + +package epoll + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "slices" + "strconv" + "strings" + "sync" + "testing" + "time" + + "golang.org/x/sys/unix" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/internal/ctxkit" + "github.com/goceleris/celeris/protocol/h2/stream" + "github.com/goceleris/celeris/resource" +) + +// celeris#669: with AsyncHandlers, runAsyncHandler holds cs.detachMu across +// the whole of ProcessH1, i.e. for as long as the user handler runs. Every +// loop-thread site that took that mutex with a blocking Lock therefore parked +// the loop thread — and so every other connection on the loop: no epoll_wait, +// no accept, no flush — until the handler returned. The epoll twin of the +// io_uring celeris#593, which PR #604 fixed with TryLock-and-skip. +// +// The sites, all on the loop thread: +// +// - closeConn's first detachMu.Lock, reached by the timeout reap +// (checkTimeouts), EPOLLRDHUP, EPOLLERR/EPOLLHUP and every error branch; +// - drainRead's read-error and EOF branches, which lock before they flush, +// notify and close — a client that disconnects mid-handler; +// - the dirty-list pass (flushDirty); +// - the EPOLLOUT resume (handleWritable). +// +// The unit arms below hold detachMu the way a running handler does and ask +// each site to return while it is held. The end-to-end arms are the issue's +// own measurement: a slow async handler on one connection and a fast +// keep-alive connection pinned to the SAME loop, whose latency must not carry +// the handler's duration. + +// stallWait is how long a unit arm lets a site run before calling it parked. +// A site that does not wait returns in microseconds; one that waits returns +// only when the test releases the lock, which it never does before this. +const stallWait = 2 * time.Second + +// holdAsHandler takes cs.detachMu as runAsyncHandler does around ProcessH1 and +// returns the release. running=true marks the dispatch goroutine as running +// (not parked), which is what a goroutine inside a handler is; running=false +// marks it parked, so the holder is someone whose hold is bounded by one +// write — a detached conn's guarded writeFn. +func holdAsHandler(t *testing.T, cs *connState, running bool) (release func()) { + t.Helper() + cs.asyncInMu.Lock() + cs.asyncRun = true + cs.asyncParked = !running + cs.asyncInMu.Unlock() + cs.detachMu.Lock() + var once sync.Once + release = func() { once.Do(cs.detachMu.Unlock) } + t.Cleanup(release) + return release +} + +// returnsWhileHeld runs site on its own goroutine and reports whether it +// returned within stallWait while the caller still holds detachMu. On a +// timeout it releases the lock (so the site can finish and the goroutine does +// not leak) and waits for it. +func returnsWhileHeld(t *testing.T, release func(), site func()) bool { + t.Helper() + done := make(chan struct{}) + go func() { + defer close(done) + site() + }() + select { + case <-done: + return true + case <-time.After(stallWait): + release() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("site never returned even after detachMu was released") + } + return false + } +} + +// exitDispatch runs the REAL runAsyncHandler tail a dispatch goroutine takes +// after its handler returns on a conn whose close was requested: the loop top +// observes asyncClosed and exits. Run synchronously, so whatever it hands back +// is on the detach queue when it returns. +func exitDispatch(l *Loop, cs *connState) { + l.asyncWG.Add(1) + l.runAsyncHandler(cs) +} + +// fdOpen reports whether fd is still an open descriptor. +func fdOpen(fd int) bool { + _, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + return err == nil +} + +// TestReapDoesNotWaitForARunningAsyncHandler is the issue's site 1 at unit +// level: checkTimeouts finds the deadline expired (nothing refreshes +// lastActivity while a handler runs) and calls closeConn, which must not park +// on the handler's detachMu. The close is still owed: once the handler returns +// and its goroutine exits, the conn is closed exactly once, with its hook. +func TestReapDoesNotWaitForARunningAsyncHandler(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + release := holdAsHandler(t, cs, true) + + if !returnsWhileHeld(t, release, l.checkTimeouts) { + t.Fatalf("celeris#669: checkTimeouts -> closeConn waited %v on cs.detachMu held by a running "+ + "async handler; the loop thread, and every connection on it, is parked until the handler returns", + stallWait) + } + + // Nothing may be torn down while the handler still runs: it is writing + // the response into this conn's buffers under the lock the reap skipped. + if l.conns[local] != cs || l.connCount != 1 || l.closeCount.Load() != 0 || rig.disconnects.Load() != 0 { + t.Fatalf("the reap tore the conn down under a running handler: slot=%p connCount=%d closeCount=%d hooks=%d", + l.conns[local], l.connCount, l.closeCount.Load(), rig.disconnects.Load()) + } + if !fdOpen(local) { + t.Fatalf("fd %d closed while the handler still owns it", local) + } + + // The handler returns; its goroutine exits and hands the close back. + release() + exitDispatch(l, cs) + l.drainDetachQueue() + + if l.conns[local] != nil || l.connCount != 0 || l.closeCount.Load() != 1 || rig.disconnects.Load() != 1 { + t.Errorf("the deferred close did not complete exactly once: slot=%p connCount=%d closeCount=%d hooks=%d", + l.conns[local], l.connCount, l.closeCount.Load(), rig.disconnects.Load()) + } + if fdOpen(local) { + t.Errorf("fd %d still open after the deferred close", local) + } +} + +// TestPeerCloseDoesNotWaitForARunningAsyncHandler: drainRead's EOF branch (a +// client that gives up mid-handler) took detachMu before flushing, notifying +// and closing. It must not park either, and the notification it owes a +// detached middleware (OnError(errPeerClosed)) must still be delivered, under +// the lock, when the deferred close runs. +func TestPeerCloseDoesNotWaitForARunningAsyncHandler(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local, peer := rig.l, rig.cs, rig.local, rig.peer + l.async = true + cs.buf = make([]byte, 4096) + var notified []error + cs.h1State.OnError = func(err error) { notified = append(notified, err) } + release := holdAsHandler(t, cs, true) + + if err := unix.Shutdown(peer, unix.SHUT_WR); err != nil { + t.Fatalf("shutdown peer: %v", err) + } + if !returnsWhileHeld(t, release, func() { l.drainRead(local, time.Now().UnixNano()) }) { + t.Fatalf("celeris#669: drainRead's EOF branch waited %v on cs.detachMu held by a running async "+ + "handler; a client that disconnects mid-handler parks the whole loop", stallWait) + } + if l.conns[local] != cs || l.closeCount.Load() != 0 { + t.Fatalf("the EOF tore the conn down under a running handler: slot=%p closeCount=%d", + l.conns[local], l.closeCount.Load()) + } + + release() + exitDispatch(l, cs) + l.drainDetachQueue() + + if l.conns[local] != nil || l.closeCount.Load() != 1 || rig.disconnects.Load() != 1 { + t.Errorf("the deferred close did not complete exactly once: slot=%p closeCount=%d hooks=%d", + l.conns[local], l.closeCount.Load(), rig.disconnects.Load()) + } + if len(notified) != 1 || !errors.Is(notified[0], errPeerClosed) { + t.Errorf("OnError calls = %v, want exactly one errPeerClosed", notified) + } +} + +// TestDirtyPassDoesNotWaitForARunningAsyncHandler: the issue's site 2. A conn +// left on the dirty list by a partial flush is locked by the pass every +// iteration; with its dispatch goroutine inside a handler the pass must move +// on, and stop polling the conn (a dirty list that never empties holds the +// loop at a 0 ms epoll_wait, a spin). The goroutine flushes writeBuf itself +// when its handler returns and hands back any remainder. +func TestDirtyPassDoesNotWaitForARunningAsyncHandler(t *testing.T) { + rig := hijackRaceConn(t) + l, cs := rig.l, rig.cs + cs.writeBuf = append(cs.writeBuf[:0], "pending"...) + cs.pendingBytes = len(cs.writeBuf) + l.markDirty(cs) + release := holdAsHandler(t, cs, true) + + if !returnsWhileHeld(t, release, l.flushDirty) { + t.Fatalf("celeris#669: the dirty-list pass waited %v on cs.detachMu held by a running async handler", + stallWait) + } + if cs.dirty || l.dirtyHead != nil { + t.Errorf("the pass left the conn on the dirty list: the loop would spin at a 0 ms epoll_wait " + + "for as long as the handler runs") + } +} + +// TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler: a conn that hit write +// backpressure is on level-triggered EPOLLOUT; a pipelined request can start a +// handler before the socket drains. The resume must not park, and must drop +// the level-triggered interest, which would otherwise fire on every +// epoll_wait until the handler returns. +func TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler(t *testing.T) { + rig := hijackRaceConn(t) + l, cs := rig.l, rig.cs + cs.writeBuf = append(cs.writeBuf[:0], "pending"...) + cs.pendingBytes = len(cs.writeBuf) + l.armEpollOut(cs) + if !cs.epollOut { + t.Fatal("setup: EPOLLOUT not armed") + } + release := holdAsHandler(t, cs, true) + + if !returnsWhileHeld(t, release, func() { l.handleWritable(cs) }) { + t.Fatalf("celeris#669: the EPOLLOUT resume waited %v on cs.detachMu held by a running async handler", + stallWait) + } + if cs.epollOut { + t.Errorf("EPOLLOUT still armed: level-triggered, it fires on every epoll_wait until the handler returns") + } +} + +// TestCloseStillWaitsForABoundedHolder is the negative control for the unit +// arms. The dispatch goroutine is PARKED, so whoever holds detachMu is a +// guarded writeFn in the middle of one write — a hold bounded by a syscall, +// which closeConn has always waited out so the write cannot race the +// teardown. That must not change: the close waits, then completes. +func TestCloseStillWaitsForABoundedHolder(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + release := holdAsHandler(t, cs, false) + + done := make(chan struct{}) + go func() { + defer close(done) + l.closeConn(local) + }() + select { + case <-done: + t.Fatal("closeConn returned while a bounded holder (a parked dispatch goroutine's writer) held " + + "detachMu; the write could race the teardown") + case <-time.After(200 * time.Millisecond): + } + release() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("closeConn never completed after the holder released detachMu") + } + if l.conns[local] != nil || l.closeCount.Load() != 1 || rig.disconnects.Load() != 1 { + t.Errorf("close incomplete: slot=%p closeCount=%d hooks=%d", + l.conns[local], l.closeCount.Load(), rig.disconnects.Load()) + } +} + +// ---- end to end: the issue's measurement -------------------------------- + +// stallSlow is the async handler's duration. The reap fires ~ReadTimeout +// after the request, the peer close 50 ms after it; either way a parked loop +// holds the fast conn for most of this. +const stallSlow = 800 * time.Millisecond + +// stallBudget is the most one fast request may take. A healthy loop serves it +// in well under a millisecond; a parked one takes ~stallSlow minus the +// trigger's offset (650-750 ms). The gap absorbs a loaded -race runner. +const stallBudget = 300 * time.Millisecond + +// stallHandler answers /fast inline with the serving loop's id and /slow on +// the dispatch goroutine after stallSlow. +type stallHandler struct{} + +func (stallHandler) HandleStream(ctx context.Context, s *stream.Stream) error { + if s.ResponseWriter == nil { + return nil + } + body := "slow" + if s.Path == "/slow" { + time.Sleep(stallSlow) + } else { + id, _ := ctxkit.WorkerIDFrom(ctx) + body = "w=" + strconv.Itoa(id) + } + return s.ResponseWriter.WriteResponse(s, 200, + [][2]string{{"content-type", "text/plain"}, {"content-length", strconv.Itoa(len(body))}}, + []byte(body)) +} +func (stallHandler) RouteAsync(_, path string) bool { return path == "/slow" } +func (stallHandler) HasAsyncRoutes() bool { return true } + +var _ stream.AsyncRouteResolver = stallHandler{} + +func startStallEngine(t *testing.T, cfg resource.Config) *Engine { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("pick port: %v", err) + } + cfg.Addr = ln.Addr().String() + _ = ln.Close() + cfg.Protocol = engine.HTTP1 + cfg.Resources = resource.Resources{Workers: 2} + cfg.AsyncHandlers = true + e, err := New(cfg, stallHandler{}) + if err != nil { + t.Fatalf("epoll engine: %v", err) // not a skip: a skip here would take the witness out of CI silently + } + ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) + go func() { errCh <- e.Listen(ctx) }() + t.Cleanup(func() { + cancel() + select { + case <-errCh: + case <-time.After(5 * time.Second): + } + }) + for dl := time.Now().Add(10 * time.Second); e.Addr() == nil && time.Now().Before(dl); { + time.Sleep(10 * time.Millisecond) + } + if e.Addr() == nil { + t.Fatal("engine did not bind") + } + return e +} + +type stallConn struct { + c net.Conn + br *bufio.Reader +} + +func stallDial(t *testing.T, addr string) *stallConn { + t.Helper() + c, err := net.DialTimeout("tcp", addr, 3*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + return &stallConn{c: c, br: bufio.NewReader(c)} +} + +// get sends one request and returns the body. +func (s *stallConn) get(path string, deadline time.Duration) (string, error) { + _ = s.c.SetDeadline(time.Now().Add(deadline)) + if _, err := fmt.Fprintf(s.c, "GET %s HTTP/1.1\r\nHost: x\r\n\r\n", path); err != nil { + return "", err + } + resp, err := http.ReadResponse(s.br, nil) + if err != nil { + return "", err + } + b, err := io.ReadAll(resp.Body) + _ = resp.Body.Close() + return string(b), err +} + +// colocate dials until a connection lands on the loop that serves slow, which +// SO_REUSEPORT decides per connection. The loop id comes back in the body. +func colocate(t *testing.T, addr string, slow *stallConn) (*stallConn, string) { + t.Helper() + want, err := slow.get("/fast", 3*time.Second) + if err != nil { + t.Fatalf("probe slow conn: %v", err) + } + for range 64 { + f := stallDial(t, addr) + got, err := f.get("/fast", 3*time.Second) + if err != nil { + t.Fatalf("probe candidate: %v", err) + } + if got == want { + return f, want + } + _ = f.c.Close() + } + t.Fatalf("no connection landed on loop %s in 64 dials", want) + return nil, "" +} + +// pingWhile pings the fast conn back to back until stop closes, and returns +// every latency, plus the first error if the fast conn broke. +func pingWhile(f *stallConn, stop <-chan struct{}) (lat []time.Duration, err error) { + for { + select { + case <-stop: + return lat, nil + default: + } + t0 := time.Now() + if _, err := f.get("/fast", 5*time.Second); err != nil { + return lat, err + } + lat = append(lat, time.Since(t0)) + time.Sleep(2 * time.Millisecond) + } +} + +// runStall drives one trigger: the slow conn sends /slow, trigger fires +// whatever makes the loop act on that conn mid-handler, and the co-located +// fast conn is pinged throughout. The slow conn must still get its whole +// response and then the close. +func runStall(t *testing.T, name string, cfg resource.Config, trigger func(*stallConn)) { + e := startStallEngine(t, cfg) + addr := e.Addr().String() + slow := stallDial(t, addr) + fast, loop := colocate(t, addr, slow) + + stop := make(chan struct{}) + type result struct { + lat []time.Duration + err error + } + pinged := make(chan result, 1) + go func() { + lat, err := pingWhile(fast, stop) + pinged <- result{lat, err} + }() + + _ = slow.c.SetDeadline(time.Now().Add(10 * time.Second)) + if _, err := slow.c.Write([]byte("GET /slow HTTP/1.1\r\nHost: x\r\n\r\n")); err != nil { + t.Fatalf("send /slow: %v", err) + } + trigger(slow) + + resp, rerr := http.ReadResponse(slow.br, nil) + var body []byte + if rerr == nil { + body, rerr = io.ReadAll(resp.Body) + _ = resp.Body.Close() + } + // The close follows the response: the reap and the peer close both still + // end this connection, once the handler has answered. + _, eofErr := slow.br.ReadByte() + time.Sleep(50 * time.Millisecond) + close(stop) + r := <-pinged + + var worst time.Duration + for _, d := range r.lat { + worst = max(worst, d) + } + sorted := slices.Clone(r.lat) + slices.Sort(sorted) + var p50 time.Duration + if len(sorted) > 0 { + p50 = sorted[len(sorted)/2] + } + t.Logf("celeris669 STALL trigger=%s loop=%s samples=%d max_ms=%.1f p50_ms=%.2f fast_err=%v slow_body=%q slow_eof=%v", + name, strings.TrimPrefix(loop, "w="), len(r.lat), float64(worst)/1e6, float64(p50)/1e6, r.err, body, eofErr) + + if r.err != nil { + t.Errorf("celeris#669: the fast conn on the same loop broke while the slow handler ran: %v", r.err) + } + if worst > stallBudget { + t.Errorf("celeris#669: a fast request on loop %s took %v (budget %v) while a %v async handler "+ + "ran on another conn of the same loop: the loop thread was parked on that conn's detachMu", + loop, worst, stallBudget, stallSlow) + } + if rerr != nil || string(body) != "slow" { + t.Errorf("slow conn: response %q, err %v; want the handler's full answer", body, rerr) + } + if !errors.Is(eofErr, io.EOF) { + t.Errorf("slow conn: after the response got %v, want EOF (the close was lost)", eofErr) + } +} + +// TestTimeoutReapOfASlowAsyncHandlerDoesNotStallItsLoop is the issue's rig: +// ReadTimeout below the handler's duration, so the reap fires mid-handler. +// ReadHeaderTimeout arms the 25 ms timerfd cadence that runs the reap on time. +func TestTimeoutReapOfASlowAsyncHandlerDoesNotStallItsLoop(t *testing.T) { + runStall(t, "reap", resource.Config{ + ReadTimeout: 100 * time.Millisecond, + ReadHeaderTimeout: 10 * time.Second, + }, func(*stallConn) {}) +} + +// TestPeerCloseDuringASlowAsyncHandlerDoesNotStallItsLoop is the same stall +// through drainRead's EOF branch: the client half-closes 50 ms into the +// handler, as a client that stops waiting does. No timeouts are configured, +// so nothing but the FIN acts on the conn. +func TestPeerCloseDuringASlowAsyncHandlerDoesNotStallItsLoop(t *testing.T) { + runStall(t, "peerclose", resource.Config{}, func(s *stallConn) { + time.Sleep(50 * time.Millisecond) + if err := s.c.(*net.TCPConn).CloseWrite(); err != nil { + t.Fatalf("half-close: %v", err) + } + }) +} diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go new file mode 100644 index 00000000..45e2b6f6 --- /dev/null +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -0,0 +1,425 @@ +//go:build linux + +package epoll + +import ( + "bufio" + "context" + "fmt" + "io" + "net" + "net/http" + "sync" + "sync/atomic" + "testing" + "time" + + "golang.org/x/sys/unix" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/internal/conn" + "github.com/goceleris/celeris/protocol/h2/stream" + "github.com/goceleris/celeris/resource" +) + +// celeris#668: with AsyncHandlers, Context.Hijack runs hijackConn on the +// connection's dispatch goroutine, and hijackConn used to remove the conn from +// l.liveConns and decrement l.connCount right there. Both are loop-thread-only +// state: checkTimeouts, the post-switch sweep (celeris#657, which added the +// sweepDormant/sweepNext writes removeLiveConn makes) and shutdown walk +// liveConns by index, and acceptAll's connCount++ can lose the goroutine's +// decrement, after which the DRAINING -> SUSPENDED gate (connCount == 0) never +// passes on that loop again. +// +// The fix leaves those two to the loop: the dispatch goroutine exits after the +// hijack and hands the connState back through the detach queue, and +// drainDetachQueue's hijacked branch removes it from the live set and +// decrements connCount on the loop thread. Until then the entry stays in the +// live set with the descriptor already closed — and the kernel may hand the +// number to the next accept — so the walkers skip a hijacked entry and the +// live set is kept by connState, not by descriptor number. +// +// Under -race (CI's root step) the two walker arms fail on the unfixed code +// with a data-race report; without -race they fail on the ownership assertion. + +// offThreadRig is the #654 socketpair rig (one async conn whose dispatch +// goroutine is running) plus bystanders: connections the walkers must keep +// visiting while the hijack happens. Bystanders carry fake descriptors; with +// no timeouts configured the walkers never act on them. +func offThreadRig(t *testing.T, bystanders int) (*hijackRaceRig, []*connState) { + t.Helper() + rig := hijackRaceConn(t) + rig.l.cfg.ReadTimeout = 0 + rig.cs.lastActivity = time.Now().UnixNano() + now := time.Now().UnixNano() + bs := make([]*connState, bystanders) + for i := range bs { + b := &connState{fd: 900 + i, liveIdx: -1, lastActivity: now} + rig.l.conns[b.fd] = b + rig.l.addLiveConn(b) + rig.l.connCount++ + bs[i] = b + } + return rig, bs +} + +// assertLiveSet checks that l.liveConns holds exactly want, each once, and +// that every entry's liveIdx is its index. +func assertLiveSet(t *testing.T, l *Loop, want ...*connState) { + t.Helper() + if len(l.liveConns) != len(want) { + t.Errorf("len(liveConns) = %d, want %d", len(l.liveConns), len(want)) + } + seen := make(map[*connState]int) + for i := range l.liveConns { + e := liveEntry(l, i) + if e == nil { + t.Errorf("liveConns[%d] resolves to no connState (a stale entry)", i) + continue + } + if e.liveIdx != i { + t.Errorf("liveConns[%d] is fd %d with liveIdx %d", i, e.fd, e.liveIdx) + } + seen[e]++ + } + for _, w := range want { + if seen[w] != 1 { + t.Errorf("fd %d appears %d times in liveConns, want once", w.fd, seen[w]) + } + } +} + +// handBack is what runAsyncHandler does on its way out after ProcessH1 +// returned ErrHijacked: it clears asyncRun and enqueues the connState. Then +// the loop drains the queue. +func handBack(l *Loop, cs *connState) { + cs.asyncInMu.Lock() + cs.asyncRun = false + cs.asyncInMu.Unlock() + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + l.drainDetachQueue() +} + +// hijackWhileWalking runs walk on its own goroutine, standing in for the loop +// thread, while this goroutine — standing in for the dispatch goroutine, +// holding detachMu as runAsyncHandler does across ProcessH1 — hijacks the conn. +func hijackWhileWalking(t *testing.T, rig *hijackRaceRig, walk func()) net.Conn { + t.Helper() + l, cs := rig.l, rig.cs + stop := make(chan struct{}) + var walked atomic.Int64 + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + walk() + walked.Add(1) + } + }() + for walked.Load() < 3 { + time.Sleep(time.Millisecond) + } + cs.detachMu.Lock() + nc, err := l.hijackConn(rig.local) + for n := walked.Load() + 3; walked.Load() < n; { + time.Sleep(time.Millisecond) + } + cs.detachMu.Unlock() + close(stop) + wg.Wait() + if err != nil { + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + return nc +} + +// TestOffThreadHijackLeavesTheLiveSetToTheLoopDuringAReap: the walker is the +// timeout reap. +func TestOffThreadHijackLeavesTheLiveSetToTheLoopDuringAReap(t *testing.T) { + rig, bs := offThreadRig(t, 16) + l, cs := rig.l, rig.cs + hijackWhileWalking(t, rig, l.checkTimeouts) + checkOffThreadHandBack(t, l, cs, bs) +} + +// TestOffThreadHijackLeavesTheLiveSetToTheLoopDuringASweep: the walker is the +// post-switch sweep (celeris#657), whose dormancy fields removeLiveConn also +// writes. +func TestOffThreadHijackLeavesTheLiveSetToTheLoopDuringASweep(t *testing.T) { + rig, bs := offThreadRig(t, 16) + l, cs := rig.l, rig.cs + target := &countingTarget{} + t.Cleanup(target.closeAll) + l.transplant.Store(&transplantState{target: target}) + hijackWhileWalking(t, rig, l.sweep) + checkOffThreadHandBack(t, l, cs, bs) + if n := target.count(); n != 0 { + t.Errorf("the sweep handed %d conns to the target; none was movable", n) + } +} + +// checkOffThreadHandBack asserts the ownership rule on both sides of the +// hand-back: the loop's live set and connCount still count the hijacked conn +// until its dispatch goroutine has exited, and drop it exactly once after. +// The public gauges move at the hijack itself. +func checkOffThreadHandBack(t *testing.T, l *Loop, cs *connState, bs []*connState) { + t.Helper() + if got := l.activeConns.Load(); got != 0 { + t.Errorf("activeConns = %d after the hijack, want 0 (the gauge moves at the hijack)", got) + } + if got := l.closeCount.Load(); got != 1 { + t.Errorf("closeCount = %d after the hijack, want 1", got) + } + if cs.liveIdx < 0 || l.connCount != len(bs)+1 { + t.Fatalf("celeris#668: hijackConn changed loop-thread-only state from the dispatch goroutine: "+ + "liveIdx=%d connCount=%d, want the conn still in the live set and connCount %d until the "+ + "goroutine has exited", cs.liveIdx, l.connCount, len(bs)+1) + } + handBack(l, cs) + if l.connCount != len(bs) { + t.Errorf("connCount = %d after the hand-back, want %d", l.connCount, len(bs)) + } + assertLiveSet(t, l, bs...) + if cs.fd != 0 { + t.Errorf("cs.fd = %d after the hand-back, want 0 (connState not released)", cs.fd) + } +} + +// TestHijackedEntryDoesNotAliasTheNextOwnerOfItsDescriptor guards the fix's +// own hazard. Between the hijack and the hand-back the hijacked conn's entry +// is still in the live set, but its descriptor is closed and the kernel may +// give the number to the next accept. A live set kept by descriptor number +// then has two entries for one number: swap-removes fix up the wrong +// connState's liveIdx, the hand-back cannot find the stale entry, and it stays +// forever (shutdown would close that number twice). Leaving removeLiveConn +// where it was and deferring only the call site — the literal "option (b)" — +// fails here. +func TestHijackedEntryDoesNotAliasTheNextOwnerOfItsDescriptor(t *testing.T) { + rig, bs := offThreadRig(t, 3) + l, cs, local := rig.l, rig.cs, rig.local + l.cfg.ReadTimeout = 100 * time.Millisecond + // Put the hijacked conn at the tail, so a later swap-remove moves it. + l.removeLiveConn(cs) + l.addLiveConn(cs) + // Expired: if its stale entry were read as the next owner, the reap + // would close that owner. + cs.lastActivity = time.Now().Add(-time.Hour).UnixNano() + + cs.detachMu.Lock() + nc, err := l.hijackConn(local) + if err != nil { + cs.detachMu.Unlock() + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + + // The number is reissued and accepted again on this loop. + pipeWrite := hijackRaceRecycle(t, local) + next := &connState{fd: local, liveIdx: -1, lastActivity: time.Now().UnixNano()} + next.h1State = conn.NewH1State() + l.driverMu.Lock() + l.conns[local] = next + l.driverMu.Unlock() + l.addLiveConn(next) + l.connCount++ + l.activeConns.Add(1) + + // Two other conns leave: the first swap moves the new owner, the second + // moves the hijacked conn's entry. + l.removeLiveConn(bs[0]) + l.connCount-- + l.removeLiveConn(bs[1]) + l.connCount-- + + l.checkTimeouts() + if l.conns[local] != next || !fdOpen(local) { + t.Fatalf("the reap acted on the new owner of fd %d through the hijacked conn's entry", local) + } + + cs.detachMu.Unlock() + handBack(l, cs) + + assertLiveSet(t, l, next, bs[2]) + if l.connCount != 2 { + t.Errorf("connCount = %d, want 2 (the new owner and one bystander)", l.connCount) + } + if _, err := unix.Write(pipeWrite, []byte("z")); err != nil { + t.Errorf("write to the new owner's pipe: %v", err) + } else { + hijackRaceExpectRead(t, local, "z", "the new owner's descriptor still works") + } +} + +// ---- end to end: accept churn and async hijacks on a real engine -------- + +// hijackChurnHandler: /hj is async and hijacks, answering on the raw conn; +// everything else answers inline. +type hijackChurnHandler struct{ hijacked *atomic.Int64 } + +func (h hijackChurnHandler) HandleStream(_ context.Context, s *stream.Stream) error { + if s.ResponseWriter == nil { + return nil + } + if s.Path != "/hj" { + return s.ResponseWriter.WriteResponse(s, 200, + [][2]string{{"content-type", "text/plain"}, {"content-length", "2"}}, []byte("ok")) + } + // Give the loop time to accept and close other conns after this + // goroutine last synchronised with it. + time.Sleep(5 * time.Millisecond) + hj, ok := s.ResponseWriter.(stream.Hijacker) + if !ok { + return fmt.Errorf("response writer %T cannot hijack", s.ResponseWriter) + } + c, err := hj.Hijack(s) + if err != nil { + return err + } + h.hijacked.Add(1) + _, _ = io.WriteString(c, "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nhj") + return c.Close() +} +func (hijackChurnHandler) RouteAsync(_, path string) bool { return path == "/hj" } +func (hijackChurnHandler) HasAsyncRoutes() bool { return true } + +// TestAsyncHijackUnderAcceptChurnLeavesTheLoopsSuspendable is the issue's +// counter-level witness: hijack from dispatch goroutines while the loops +// accept and close other conns, then pause accepting. Every loop must reach +// SUSPENDED with connCount 0 and an empty live set. +func TestAsyncHijackUnderAcceptChurnLeavesTheLoopsSuspendable(t *testing.T) { + var hijacked atomic.Int64 + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("pick port: %v", err) + } + addr := ln.Addr().String() + _ = ln.Close() + e, err := New(resource.Config{ + Addr: addr, + Protocol: engine.HTTP1, + Resources: resource.Resources{Workers: 2}, + AsyncHandlers: true, + }, hijackChurnHandler{hijacked: &hijacked}) + if err != nil { + t.Fatalf("epoll engine: %v", err) + } + ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) + go func() { errCh <- e.Listen(ctx) }() + defer func() { + cancel() + select { + case <-errCh: + case <-time.After(5 * time.Second): + } + }() + for dl := time.Now().Add(10 * time.Second); e.Addr() == nil && time.Now().Before(dl); { + time.Sleep(10 * time.Millisecond) + } + if e.Addr() == nil { + t.Fatal("engine did not bind") + } + + get := func(path string) error { + c, err := net.DialTimeout("tcp", addr, 3*time.Second) + if err != nil { + return err + } + defer func() { _ = c.Close() }() + _ = c.SetDeadline(time.Now().Add(5 * time.Second)) + if _, err := fmt.Fprintf(c, "GET %s HTTP/1.1\r\nHost: x\r\n\r\n", path); err != nil { + return err + } + resp, err := http.ReadResponse(bufio.NewReader(c), nil) + if err != nil { + return err + } + _, err = io.ReadAll(resp.Body) + _ = resp.Body.Close() + return err + } + + const hijackers, perHijacker, churners = 4, 16, 4 + stop := make(chan struct{}) + var churned, failures atomic.Int64 + var churnWG, hjWG sync.WaitGroup + for range churners { + churnWG.Add(1) + go func() { + defer churnWG.Done() + for { + select { + case <-stop: + return + default: + } + if get("/churn") == nil { + churned.Add(1) + } else { + failures.Add(1) + } + } + }() + } + for range hijackers { + hjWG.Add(1) + go func() { + defer hjWG.Done() + for range perHijacker { + if get("/hj") != nil { + failures.Add(1) + } + } + }() + } + hjWG.Wait() + close(stop) + churnWG.Wait() + + for dl := time.Now().Add(5 * time.Second); e.Metrics().ActiveConnections != 0 && time.Now().Before(dl); { + time.Sleep(10 * time.Millisecond) + } + if err := e.PauseAccept(); err != nil { + t.Fatalf("PauseAccept: %v", err) + } + suspended := 0 + for dl := time.Now().Add(5 * time.Second); time.Now().Before(dl); { + suspended = 0 + for _, l := range e.loops { + if l.suspended.Load() { + suspended++ + } + } + if suspended == len(e.loops) { + break + } + time.Sleep(10 * time.Millisecond) + } + t.Logf("celeris668 HIJACKCHURN hijacked=%d churned=%d failures=%d active=%d suspended=%d/%d", + hijacked.Load(), churned.Load(), failures.Load(), e.Metrics().ActiveConnections, suspended, len(e.loops)) + + if got := hijacked.Load(); got != hijackers*perHijacker { + t.Errorf("hijacked %d conns, want %d", got, hijackers*perHijacker) + } + if suspended != len(e.loops) { + t.Fatalf("celeris#668: %d/%d loops reached SUSPENDED after the pause; a loop whose connCount "+ + "lost an update to an off-thread hijack never passes the connCount == 0 gate", suspended, len(e.loops)) + } + // Each loop is parked; its last writes happened before it published + // suspended, so reading them here is ordered. + for i, l := range e.loops { + if l.connCount != 0 || len(l.liveConns) != 0 { + t.Errorf("loop %d suspended with connCount=%d liveConns=%d, want 0 and 0", i, l.connCount, len(l.liveConns)) + } + } +} diff --git a/engine/epoll/livecs_linux_test.go b/engine/epoll/livecs_linux_test.go new file mode 100644 index 00000000..a8cb5514 --- /dev/null +++ b/engine/epoll/livecs_linux_test.go @@ -0,0 +1,13 @@ +//go:build linux + +package epoll + +// liveEntry returns the connState at index i of l.liveConns, or nil when the +// entry no longer resolves to one. +func liveEntry(l *Loop, i int) *connState { + fd := l.liveConns[i] + if fd < 0 || fd >= len(l.conns) { + return nil + } + return l.conns[fd] +} From 350b94cd0abaf3cbf0cc7362b2393448fdf1852c Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 14:41:00 +0200 Subject: [PATCH 03/20] fix(epoll): never park the loop on a running async handler, and leave the live set to the loop on an async hijack (celeris#669, celeris#668) celeris#669. With AsyncHandlers the dispatch goroutine holds cs.detachMu for the whole handler, and four loop-thread sites took it with a blocking Lock, so one slow handler parked every connection on its loop (no epoll_wait, accept or flush) until it returned. Each now TryLocks, and when the lock is held while the conn's dispatch goroutine is running (dispatchBusy: asyncRun && !asyncParked under asyncInMu) it does not wait: - closeConn leaves the close to the goroutine (closeOwed). asyncClosed is already set, so the goroutine exits at its next check, and its exit hands cs back through the detach queue, whose asyncClosed branch runs closeConn again with the lock free. The conn stays whole meanwhile. This covers the timeout reap, EPOLLRDHUP, EPOLLHUP and error closes. - drainRead's read-error and EOF branches (a client that gives up on a slow handler) leave their flush and OnError to that close (closeErr), which delivers them under the lock. - the dirty pass takes the conn off the list and the EPOLLOUT resume drops the level-triggered interest: the holder flushes writeBuf itself and hands a remainder back. A deferred peer close keeps its place. A parked or absent goroutine means the holder is a guarded writeFn in one write, so those sites still wait for it, as before. celeris#668. hijackConn on the dispatch goroutine no longer touches liveConns or connCount (nor, through removeLiveConn, the sweep's dormancy fields); drainDetachQueue's hijacked branch does both on the loop once the goroutine has exited. Because the descriptor is closed at the hijack and its number can be reissued before that, liveConns now holds connStates rather than descriptor numbers, walkers skip a hijacked entry (hijacked is now atomic, stored before the descriptor is released), and accept/adopt install their slot under driverMu so the install is ordered after the hijack's clear. Also: an owed close blocks a new dispatch goroutine, a transplant, and the EPOLLRDHUP branch's unlocked look at the buffers the handler writes. The celeris#654 tests reach closeConn's wait with a parked goroutine now; the production shape (close left to a handler that then hijacks) has its own test. --- engine/epoll/adopt.go | 11 +- .../epoll/async_handler_stall_linux_test.go | 40 ++ engine/epoll/conn.go | 36 +- engine/epoll/detach_reap_drain_linux_test.go | 2 +- .../driver_epollctl_after_shutdown_test.go | 2 +- .../epoll/hijack_closeconn_race_linux_test.go | 145 ++++- engine/epoll/livecs_linux_test.go | 6 +- engine/epoll/loop.go | 495 +++++++++++++----- engine/epoll/review_v150_test.go | 32 +- engine/epoll/sweep.go | 15 +- engine/epoll/transplant.go | 7 + engine/epoll/transplant_accounting_test.go | 2 +- engine/epoll/wakefd_after_shutdown_test.go | 2 +- 13 files changed, 614 insertions(+), 181 deletions(-) diff --git a/engine/epoll/adopt.go b/engine/epoll/adopt.go index d306a054..89feb1dd 100644 --- a/engine/epoll/adopt.go +++ b/engine/epoll/adopt.go @@ -161,7 +161,14 @@ func (l *Loop) attachAdoptedFD(ctx context.Context, fd int, carry engine.Carryov if fd >= len(l.conns) { l.growConns(fd) } - if l.conns[fd] != nil { + // The slot is read and, below, filled under driverMu: an async Hijack on + // this loop clears a slot from its dispatch goroutine under this lock and + // then closes the descriptor, whose number the kernel may reissue to the + // connection being adopted here (celeris#668). + l.driverMu.Lock() + occupied := l.conns[fd] != nil + l.driverMu.Unlock() + if occupied { // Slot occupied — the source detached fd before handing it off, so this // should not happen; refuse rather than clobber a live conn. Do not close // (the slot holder may close the same descriptor later). The connection @@ -186,7 +193,9 @@ func (l *Loop) attachAdoptedFD(ctx context.Context, fd int, carry engine.Carryov cs := acquireConnState(ctxkit.WithWorkerID(ctx, l.id), fd, l.resolved.BufferSize, l.async) cs.remoteAddr = carry.RemoteAddr + l.driverMu.Lock() l.conns[fd] = cs + l.driverMu.Unlock() l.addLiveConn(cs) l.connCount++ if fd > l.maxFD { diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go index c39a8b4d..7cf1f8fc 100644 --- a/engine/epoll/async_handler_stall_linux_test.go +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -238,6 +238,46 @@ func TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler(t *testing.T) { } } +// TestDeferredPeerCloseSurvivesARunningHandler guards the one exception to +// the two passes above. A conn whose peer half-closed while its response was +// still queued (peerClosed) is closed by whichever pass sees the flush +// complete; a holder whose own flush completes hands nothing back. So while a +// handler runs, the dirty pass keeps such a conn and the EPOLLOUT resume keeps +// its interest — and once the handler returns, the next pass closes it. +func TestDeferredPeerCloseSurvivesARunningHandler(t *testing.T) { + for _, site := range []string{"dirty", "epollout"} { + t.Run(site, func(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + cs.writeBuf = append(cs.writeBuf[:0], "pending"...) + cs.pendingBytes = len(cs.writeBuf) + cs.peerClosed = true + pass := l.flushDirty + if site == "dirty" { + l.markDirty(cs) + } else { + l.armEpollOut(cs) + pass = func() { l.handleWritable(cs) } + } + release := holdAsHandler(t, cs, true) + if !returnsWhileHeld(t, release, pass) { + t.Fatalf("the %s pass waited on a running handler (celeris#669)", site) + } + if !cs.dirty && !cs.epollOut { + t.Fatalf("the %s pass dropped a conn with a deferred peer close: nothing would close it "+ + "once the handler's own flush completes", site) + } + release() + pass() + if l.conns[local] != nil || rig.disconnects.Load() != 1 { + t.Errorf("the %s pass did not close the conn once flushed: slot=%p hooks=%d", + site, l.conns[local], rig.disconnects.Load()) + } + hijackRaceExpectRead(t, rig.peer, "pending", "the queued response reached the peer before the close") + }) + } +} + // TestCloseStillWaitsForABoundedHolder is the negative control for the unit // arms. The dispatch goroutine is PARKED, so whoever holds detachMu is a // guarded writeFn in the middle of one write — a hold bounded by a syscall, diff --git a/engine/epoll/conn.go b/engine/epoll/conn.go index b98f1b8b..40501588 100644 --- a/engine/epoll/conn.go +++ b/engine/epoll/conn.go @@ -195,8 +195,9 @@ type connState struct { asyncDetachPending bool // liveIdx is this conn's index into l.liveConns (the dense slice of - // active FDs iterated by checkTimeouts/shutdown). -1 when not present. - // Maintained by addLiveConn/removeLiveConn for O(1) removal. (#318) + // live connStates iterated by checkTimeouts, the sweep and shutdown). + // -1 when not present. Maintained by addLiveConn/removeLiveConn for O(1) + // removal (#318). Loop-thread-only, like liveConns itself (celeris#668). liveIdx int // hijacked is set by hijackConn (Context.Hijack) once the fd has been @@ -207,7 +208,32 @@ type connState struct { // The release is deferred to the worker via the asyncClosed + // detachQueue → drainDetachQueue handoff, which sees this flag and runs // releaseConnState only after the goroutine has exited. (#3.1) - hijacked bool + // + // That hand-off also takes the conn out of l.liveConns and l.connCount, + // which are the loop's and which the dispatch goroutine must not touch + // (celeris#668). Until it runs, the conn's entry stays in the live set + // with its descriptor already closed, so the live-set walkers skip an + // entry with this flag set. They read it without any lock the + // goroutine holds, hence atomic; hijackConn stores it before it + // releases the descriptor number. + hijacked atomic.Bool + + // closeOwed (guarded by asyncInMu) is set by closeConn when it finds + // detachMu held by this conn's RUNNING dispatch goroutine — i.e. held + // across a user handler — and leaves the close to that goroutine + // instead of parking the loop on the lock for the rest of the handler + // (celeris#669). asyncClosed is already set, so the goroutine exits at + // its next check; the exit path that finds closeOwed hands cs back + // through the detach queue, whose asyncClosed branch runs closeConn + // again, on the loop thread, with the lock free. + closeOwed bool + + // closeErr (loop-thread-only) is the I/O error a drainRead branch met + // while a running handler held detachMu. That branch used to flush and + // deliver it to OnError under the lock before closing; it now leaves + // both to closeConn, which does them under the lock when the close + // actually runs (celeris#669). + closeErr error } var connStatePool = sync.Pool{ @@ -278,7 +304,9 @@ func releaseConnState(cs *connState) { cs.asyncDetachUnlocked = false cs.asyncDetachPending = false cs.liveIdx = -1 - cs.hijacked = false + cs.hijacked.Store(false) + cs.closeOwed = false + cs.closeErr = nil cs.fd = 0 connStatePool.Put(cs) } diff --git a/engine/epoll/detach_reap_drain_linux_test.go b/engine/epoll/detach_reap_drain_linux_test.go index 4b73f8ce..c8b2d11d 100644 --- a/engine/epoll/detach_reap_drain_linux_test.go +++ b/engine/epoll/detach_reap_drain_linux_test.go @@ -28,7 +28,7 @@ func newReapLoop(t *testing.T) *Loop { return &Loop{ epollFD: epfd, conns: make([]*connState, connTableSize), - liveConns: make([]int, 0, 4), + liveConns: make([]*connState, 0, 4), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, diff --git a/engine/epoll/driver_epollctl_after_shutdown_test.go b/engine/epoll/driver_epollctl_after_shutdown_test.go index d71bb7dc..ed4a7bd2 100644 --- a/engine/epoll/driver_epollctl_after_shutdown_test.go +++ b/engine/epoll/driver_epollctl_after_shutdown_test.go @@ -103,7 +103,7 @@ func TestRegisterConnAfterShutdownDoesNotTouchTheClosedEpollFD(t *testing.T) { timerFD: -1, wakeFD: wakefd.New(-1), conns: make([]*connState, connTableSize), - liveConns: make([]int, 0, 4), + liveConns: make([]*connState, 0, 4), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, diff --git a/engine/epoll/hijack_closeconn_race_linux_test.go b/engine/epoll/hijack_closeconn_race_linux_test.go index 8b069ed3..51583329 100644 --- a/engine/epoll/hijack_closeconn_race_linux_test.go +++ b/engine/epoll/hijack_closeconn_race_linux_test.go @@ -36,6 +36,24 @@ import ( // BEFORE blocking on detachMu, so that flag is an exact happens-before // barrier. The loop is not running, so the "loop thread" here is just // another goroutine calling the sweep. +// +// Since celeris#669 closeConn no longer waits for a RUNNING dispatch +// goroutine: it leaves the close to it (see dispatchBusy). It still waits +// when the goroutine is parked, because then the holder is a bounded one (a +// guarded writeFn), and the ownership re-check guards that wait against any +// holder that detaches the conn meanwhile. The arms that pin the re-check +// therefore mark the goroutine parked (parkDispatch) to reach the wait, and +// let the hijack stand in for such a holder. The shape a running handler now +// produces — the close left to it, then a Hijack — is pinned separately by +// TestCloseLeftToAHandlerThatHijacksIsNotRedone. + +// parkDispatch marks the rig's dispatch goroutine as parked in its +// asyncCond.Wait, so closeConn takes its (bounded) wait on detachMu. +func parkDispatch(cs *connState) { + cs.asyncInMu.Lock() + cs.asyncParked = true + cs.asyncInMu.Unlock() +} // hijackRaceRig is one bare Loop carrying a single async-mode HTTP/1 conn, // plus the OnDisconnect counter every arm asserts on. @@ -237,6 +255,7 @@ func hijackRaceRecycle(t *testing.T, fd int) (writeEnd int) { func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) { rig := hijackRaceConn(t) l, cs, local, peer := rig.l, rig.cs, rig.local, rig.peer + parkDispatch(cs) // (a) The dispatch goroutine is inside ProcessH1: runAsyncHandler holds // cs.detachMu for the whole user handler call. @@ -278,8 +297,11 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) if got := l.activeConns.Load(); got != 0 { t.Errorf("activeConns = %d, want 0: a double decrement drives the live gauge negative", got) } - if got := l.connCount; got != 0 { - t.Errorf("connCount = %d, want 0: a negative count never satisfies the DRAINING->SUSPENDED gate", got) + // connCount is the loop's: an off-thread hijack leaves it to the + // hand-back (celeris#668), so it still counts the conn here and must + // reach exactly 0 after, below. + if got := l.connCount; got != 1 { + t.Errorf("connCount = %d before the hand-back, want 1", got) } // OnDisconnect is a public lifecycle callback. The connection did not @@ -324,12 +346,15 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) l.detachQPending.Store(1) l.detachQMu.Unlock() l.drainDetachQueue() - if cs.hijacked { + if cs.hijacked.Load() { t.Error("drainDetachQueue skipped the hijacked pool release (detachClosed short-circuit)") } if cs.fd != 0 { t.Errorf("cs.fd = %d after the hand-off, want 0 (connState not released)", cs.fd) } + if got := l.connCount; got != 0 { + t.Errorf("connCount = %d, want 0: a negative count never satisfies the DRAINING->SUSPENDED gate", got) + } } // TestCloseConnLeavesAReissuedSlotAloneAfterWaitingOnDetachMu is the arm that @@ -347,6 +372,7 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) func TestCloseConnLeavesAReissuedSlotAloneAfterWaitingOnDetachMu(t *testing.T) { rig := hijackRaceConn(t) l, cs, local := rig.l, rig.cs, rig.local + parkDispatch(cs) cs.detachMu.Lock() done := hijackRaceSweep(l) @@ -378,16 +404,27 @@ func TestCloseConnLeavesAReissuedSlotAloneAfterWaitingOnDetachMu(t *testing.T) { t.Errorf("l.conns[%d] = %p, want the new owner %p: closeConn cleared a slot it no longer owned", local, l.conns[local], newCS) } - if newCS.liveIdx < 0 || len(l.liveConns) != 1 { - t.Errorf("new owner liveIdx = %d, liveConns = %v, want it still in the live set", - newCS.liveIdx, l.liveConns) - } if got := l.closeCount.Load(); got != 1 { t.Errorf("closeCount = %d, want 1 (only the hijack)", got) } if got := l.activeConns.Load(); got != 1 { t.Errorf("activeConns = %d, want 1 (the new owner)", got) } + // The hijacked conn's live-set entry and count are the loop's until its + // dispatch goroutine hands it back (celeris#668); then only the new + // owner is left, found by its own entry. + cs.asyncInMu.Lock() + cs.asyncRun = false + cs.asyncInMu.Unlock() + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + l.drainDetachQueue() + if newCS.liveIdx != 0 || len(l.liveConns) != 1 || l.liveConns[0] != newCS { + t.Errorf("new owner liveIdx = %d, len(liveConns) = %d, want it alone in the live set", + newCS.liveIdx, len(l.liveConns)) + } if got := l.connCount; got != 1 { t.Errorf("connCount = %d, want 1 (the new owner)", got) } @@ -438,6 +475,19 @@ func TestCloseConnAfterHijackIsNoOpWithoutTheRace(t *testing.T) { if got := l.activeConns.Load(); got != 0 { t.Errorf("activeConns = %d, want 0", got) } + if cs.detachClosed { + t.Error("closeConn touched a connState it no longer owns") + } + // The dispatch goroutine exits and hands cs back; only then is the + // loop's connCount decremented (celeris#668). + cs.asyncInMu.Lock() + cs.asyncRun = false + cs.asyncInMu.Unlock() + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + l.drainDetachQueue() if got := l.connCount; got != 0 { t.Errorf("connCount = %d, want 0", got) } @@ -453,14 +503,15 @@ func TestCloseConnAfterHijackIsNoOpWithoutTheRace(t *testing.T) { hijackRaceExpectRead(t, local, "y", "recycled pipe survives an uncontended closeConn") } } - if cs.detachClosed { - t.Error("closeConn touched a connState it no longer owns") + if cs.fd != 0 { + t.Error("the hand-back did not release the hijacked connState") } } // TestCloseConnClosesOnceWhenHandlerDoesNotHijack is negative control 2: the -// identical interleaving — the timeout sweep parked on detachMu behind a -// running async handler — with no Hijack. closeConn still owns the conn when +// identical interleaving — the timeout sweep waiting on detachMu behind a +// holder (with the dispatch goroutine parked, see parkDispatch) — with no +// Hijack. closeConn still owns the conn when // it wakes, so it must close it exactly once AND still deliver OnDisconnect. // It passes before and after the fix, showing the harness does not by itself // produce a double count, and that the ownership re-check suppresses neither @@ -468,6 +519,7 @@ func TestCloseConnAfterHijackIsNoOpWithoutTheRace(t *testing.T) { func TestCloseConnClosesOnceWhenHandlerDoesNotHijack(t *testing.T) { rig := hijackRaceConn(t) l, cs, local := rig.l, rig.cs, rig.local + parkDispatch(cs) cs.detachMu.Lock() done := hijackRaceSweep(l) @@ -528,7 +580,7 @@ func TestHijackReleaseUnlinksTheConnFromTheDirtyList(t *testing.T) { t.Fatalf("hijackConn: %v", err) } t.Cleanup(func() { _ = nc.Close() }) - if !cs.hijacked { + if !cs.hijacked.Load() { t.Fatalf("setup: async hijack did not defer the release") } @@ -566,7 +618,7 @@ func TestHijackReleaseUnlinksTheConnFromTheDirtyList(t *testing.T) { t.Fatalf("hijackConn: %v", err) } t.Cleanup(func() { _ = nc.Close() }) - if cs.hijacked { + if cs.hijacked.Load() { t.Fatalf("setup: sync hijack deferred the release instead of doing it inline") } @@ -575,3 +627,70 @@ func TestHijackReleaseUnlinksTheConnFromTheDirtyList(t *testing.T) { } }) } + +// TestCloseLeftToAHandlerThatHijacksIsNotRedone is the shape the celeris#654 +// race takes since celeris#669. The timeout reap finds the handler running and +// leaves the close to its dispatch goroutine instead of waiting; the handler +// then hijacks. The goroutine's exit (runAsyncHandler's ErrHijacked path) +// hands the conn back, and drainDetachQueue must take the hijack's branch — +// pool release, live set, connCount — and not run the owed close: no second +// close count, no OnDisconnect, and the reissued descriptor survives. +func TestCloseLeftToAHandlerThatHijacksIsNotRedone(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + release := holdAsHandler(t, cs, true) + + if !returnsWhileHeld(t, release, l.checkTimeouts) { + t.Fatal("the reap waited on a running handler (celeris#669)") + } + cs.asyncInMu.Lock() + owed := cs.closeOwed + cs.asyncInMu.Unlock() + if !owed { + t.Fatal("setup: the reap did not leave the close to the dispatch goroutine") + } + + nc, err := l.hijackConn(local) + if err != nil { + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + pipeWrite := hijackRaceRecycle(t, local) + release() + + // runAsyncHandler's ErrHijacked exit: asyncClosed, the goroutine gone, + // cs enqueued — the enqueue is the hand-back. + cs.asyncClosed.Store(true) + cs.asyncInMu.Lock() + cs.endDispatch() + cs.asyncInMu.Unlock() + l.enqueueDetach(cs) + l.drainDetachQueue() + + if got := l.closeCount.Load(); got != 1 { + t.Errorf("closeCount = %d, want 1 (the hijack only)", got) + } + if got := l.activeConns.Load(); got != 0 { + t.Errorf("activeConns = %d, want 0", got) + } + if got := l.connCount; got != 0 { + t.Errorf("connCount = %d, want 0", got) + } + if got := rig.disconnects.Load(); got != 0 { + t.Errorf("OnDisconnect fired %d times for a hijacked conn, want 0", got) + } + if len(l.liveConns) != 0 { + t.Errorf("len(liveConns) = %d after the hand-back, want 0", len(l.liveConns)) + } + if cs.fd != 0 { + t.Errorf("cs.fd = %d after the hand-back, want 0 (connState not released)", cs.fd) + } + if !fdOpen(local) { + t.Fatalf("fd %d, reissued after the hijack, was closed by the owed close", local) + } + if _, err := unix.Write(pipeWrite, []byte("w")); err != nil { + t.Errorf("write to the recycled pipe: %v", err) + } else { + hijackRaceExpectRead(t, local, "w", "the reissued descriptor survives") + } +} diff --git a/engine/epoll/livecs_linux_test.go b/engine/epoll/livecs_linux_test.go index a8cb5514..a75bca9f 100644 --- a/engine/epoll/livecs_linux_test.go +++ b/engine/epoll/livecs_linux_test.go @@ -5,9 +5,5 @@ package epoll // liveEntry returns the connState at index i of l.liveConns, or nil when the // entry no longer resolves to one. func liveEntry(l *Loop, i int) *connState { - fd := l.liveConns[i] - if fd < 0 || fd >= len(l.conns) { - return nil - } - return l.conns[fd] + return l.liveConns[i] } diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index d3073b5a..54e66096 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -85,12 +85,22 @@ type Loop struct { conns []*connState connCount int // number of active connections (local, for draining check) maxFD int // upper bound fd for iteration in checkTimeouts/shutdown - // liveConns is a dense slice of currently-active FDs. checkTimeouts - // and shutdown iterate it instead of the sparse 0..maxFD range, so - // their cost is O(active conns) regardless of the FD space (maxFD - // never shrinks). Maintained by addLiveConn/removeLiveConn on every - // accept/close path. Worker-thread-only — no synchronization. (#318) - liveConns []int + // liveConns is a dense slice of the connections this loop holds. + // checkTimeouts, the sweep and shutdown iterate it instead of the sparse + // 0..maxFD range, so their cost is O(active conns) regardless of the FD + // space (maxFD never shrinks). Maintained by addLiveConn/removeLiveConn + // on every accept/close path. Worker-thread-only — no synchronization. + // (#318) + // + // It holds connStates, not descriptor numbers (celeris#668). An async + // hijack leaves its entry here until the dispatch goroutine has exited + // and handed the conn back, but closes the descriptor at once, so the + // kernel can reissue the number to the next accept on this loop before + // the entry goes. Keyed by number, the two entries would alias: the + // swap-remove fix-up would move the wrong connState's liveIdx and the + // stale entry could never be found again. Walkers skip an entry whose + // connState is hijacked. + liveConns []*connState // listenHot is set by acceptAll when it stops with the listen backlog // possibly non-empty (per-call cap hit, or EMFILE/ENFILE back-off). // The listen socket is edge-triggered, so the queued SYNs won't @@ -298,7 +308,7 @@ func newLoop(id, cpuID int, handler stream.Handler, timerFD: -1, events: make([]unix.EpollEvent, min(resolved.MaxEvents, maxEpollEvents)), conns: make([]*connState, connTableInitSize), - liveConns: make([]int, 0, 1024), + liveConns: make([]*connState, 0, 1024), handler: handler, resolved: resolved, cfg: cfg, @@ -603,9 +613,16 @@ func (l *Loop) run(ctx context.Context) { // the response is not truncated. Detached (WS/SSE) conns keep their // middleware's close lifecycle, but they still have to LEARN the // peer is gone — see notifyDetachedPeerClosed. + // + // A conn whose close is already under way (asyncClosed) is left + // alone. drainRead's EOF branch, on this same event, may have + // left the close to a dispatch goroutine that is still inside its + // handler (celeris#669); that goroutine is writing the response + // under detachMu, so csWritePending below would read its buffers + // unlocked, and closeConn would only repeat the hand-off. if ev.Events&unix.EPOLLRDHUP != 0 { if fd >= 0 && fd < len(l.conns) { - if cs := l.conns[fd]; cs != nil && !cs.detachClosed { + if cs := l.conns[fd]; cs != nil && !cs.detachClosed && !cs.asyncClosed.Load() { switch { case cs.h1State != nil && cs.h1State.Detached.Load(): l.notifyDetachedPeerClosed(cs) @@ -937,7 +954,15 @@ func (l *Loop) acceptAll(ctx context.Context, now int64) acceptStop { connCtx := ctxkit.WithWorkerID(ctx, l.id) cs := acquireConnState(connCtx, newFD, l.resolved.BufferSize, l.async) cs.remoteAddr = sockaddrString(sa) + // Install under driverMu. An async Hijack clears its slot from the + // dispatch goroutine under this lock and then closes the + // descriptor, and the kernel's lowest-free-fd rule can hand the + // number straight back to this accept: the install must be ordered + // after that write (celeris#668). One uncontended lock per accept, + // next to accept4, setsockopt and epoll_ctl. + l.driverMu.Lock() l.conns[newFD] = cs + l.driverMu.Unlock() l.addLiveConn(cs) l.connCount++ if newFD > l.maxFD { @@ -1030,7 +1055,10 @@ func (l *Loop) acceptQueuedOnPause(ctx context.Context) { // reads a slot a hijack is concurrently niling, and no reader observes a torn // header (base + len). Allocation stays outside the lock to keep the critical // section to a copy + pointer store; growth is rare (only on a new high-water -// fd), so the lock is well off the steady hot path. +// fd), so the lock is well off the steady hot path. The slot INSTALLS +// (acceptAll, attachAdoptedFD) take l.driverMu too, so installing a descriptor +// number a hijack has just released is ordered after the hijack's clear +// (celeris#668). func (l *Loop) growConns(fd int) { newLen := len(l.conns) for newLen <= fd { @@ -1075,33 +1103,13 @@ func (l *Loop) drainRead(fd int, now int64) { if err == unix.EAGAIN || err == unix.EWOULDBLOCK { return } - if mu := cs.detachMu; mu != nil { - mu.Lock() - } - _ = l.flushWrites(cs, true) // Surface read failure to detached middleware (e.g. WS). - if cs.h1State != nil && cs.h1State.OnError != nil { - cs.h1State.OnError(err) - } - if mu := cs.detachMu; mu != nil { - mu.Unlock() - } - l.closeConn(fd) + l.closeOnReadEnd(fd, cs, err) return } if n == 0 { - if mu := cs.detachMu; mu != nil { - mu.Lock() - } - _ = l.flushWrites(cs, true) // Surface peer-close (EOF) to detached middleware. - if cs.h1State != nil && cs.h1State.OnError != nil { - cs.h1State.OnError(errPeerClosed) - } - if mu := cs.detachMu; mu != nil { - mu.Unlock() - } - l.closeConn(fd) + l.closeOnReadEnd(fd, cs, errPeerClosed) return } @@ -1234,7 +1242,12 @@ func (l *Loop) drainRead(fd int, now int64) { // ProcessH1, so the worker's next read into cs.buf can't // overwrite in-flight bytes. Zero allocation on steady state. cs.asyncInBuf = append(cs.asyncInBuf, data...) - starting := !cs.asyncRun + // Never start a dispatch goroutine on a conn whose close is + // owed: its last goroutine can exit before the loop drains the + // hand-back that finishes the close (celeris#669), and a new one + // would only exit again. Evaluated only when no goroutine runs, + // so the steady state pays nothing. + starting := !cs.asyncRun && !cs.asyncClosed.Load() if starting { cs.asyncRun = true } @@ -1491,6 +1504,89 @@ func (l *Loop) drainRead(fd int, now int64) { } } +// closeOnReadEnd is drainRead's read-error and EOF branch: flush what is +// queued, surface err to a detached middleware (OnError, under detachMu, as +// every OnError site in this file runs), and close. +// +// When detachMu is held and the conn's dispatch goroutine is running, the +// holder is that goroutine inside a user handler (celeris#669), and waiting +// for the lock parked the loop — and every connection on it — until the +// handler returned. The common shape is a client that gives up on a slow +// handler and disconnects. So the flush and the notification ride on the +// close instead: closeConn leaves it to the goroutine, which exits at its next +// check, and then runs it on this thread with the lock free, delivering +// closeErr first. +func (l *Loop) closeOnReadEnd(fd int, cs *connState, err error) { + mu := cs.detachMu + if mu != nil && !mu.TryLock() { + if dispatchBusy(cs) { + cs.closeErr = err + l.closeConn(fd) + return + } + mu.Lock() + } + _ = l.flushWrites(cs, true) + if cs.h1State != nil && cs.h1State.OnError != nil { + cs.h1State.OnError(err) + } + if mu != nil { + mu.Unlock() + } + l.closeConn(fd) +} + +// dispatchBusy reports whether cs has a dispatch goroutine that is running: +// alive (asyncRun) and not parked in asyncCond.Wait. Loop thread. +// +// It is how a loop-thread site that found cs.detachMu held tells the holders +// apart (celeris#669). The dispatch goroutine holds the lock across +// ProcessH1, i.e. for as long as the user handler runs, and it is running +// whenever it does. Every other holder — a detached conn's guarded writeFn, +// the goroutine's own asyncClosed re-check — holds it for one write or less. +// So a site that finds the lock held and the goroutine running must not wait, +// and one that finds it held with the goroutine parked or absent may wait as +// it always has: that wait is bounded. +// +// A running goroutine is not necessarily the holder (it may be delivering a +// frame after Detach while a guarded writeFn holds the lock). Every site that +// acts on a true only leaves work to be done later — a flush the holder does +// anyway, a close the goroutine hands back on its way out — so a false +// positive costs a hand-off, never a lost write or close. +// +// Parked is exact, not a heuristic: asyncParked is set and cleared under +// asyncInMu in the same critical section as the park loop's condition, so a +// goroutine seen parked here can only leave Wait by re-taking asyncInMu, after +// this read, and a caller that has already set asyncClosed makes it exit +// without taking detachMu. +func dispatchBusy(cs *connState) bool { + if cs.asyncCond.L == nil { + return false // no async machinery: sync mode, no dispatch goroutine + } + cs.asyncInMu.Lock() + busy := cs.asyncRun && !cs.asyncParked + cs.asyncInMu.Unlock() + return busy +} + +// leaveCloseToDispatch is dispatchBusy for closeConn, which must record the +// hand-off in the same critical section it decides it in: closeOwed is read +// by the goroutine's exit path under asyncInMu, so a goroutine that exits +// after this section sees it, and one that exited before it made asyncRun +// false here. Loop thread; asyncClosed must already be set. +func leaveCloseToDispatch(cs *connState) bool { + if cs.asyncCond.L == nil { + return false + } + cs.asyncInMu.Lock() + busy := cs.asyncRun && !cs.asyncParked + if busy { + cs.closeOwed = true + } + cs.asyncInMu.Unlock() + return busy +} + func (l *Loop) hijackConn(fd int) (net.Conn, error) { // l.conns access is guarded by driverMu: in async mode this runs on a // dispatch goroutine, so the read of the header/slot must be serialized @@ -1501,38 +1597,56 @@ func (l *Loop) hijackConn(fd int) (net.Conn, error) { if cs == nil { return nil, errors.New("celeris: connection not found") } - // Detach the conn from the engine: drop it from epoll, the live set and - // the conn table, and update the counters. + // Which thread is this? A dispatch goroutine is alive for this conn only + // once the conn was promoted to one, and from then on the handler — and + // so this call — runs ON that goroutine (inside ProcessH1 → ErrHijacked). + // Otherwise the call is inline on the loop thread: sync mode, or an + // async-mode conn not yet promoted running its first request inline. + // asyncRun is read under asyncInMu, the lock the goroutine's lifetime is + // published under. + offThread := false + if cs.detachMu != nil { + cs.asyncInMu.Lock() + offThread = cs.asyncRun + cs.asyncInMu.Unlock() + } + // Detach the conn from the engine: drop it from epoll and the conn table, + // and move the public gauges. // - // WHAT IS AND IS NOT SERIALIZED HERE. In async mode this runs ON the - // dispatch goroutine (inside ProcessH1 → ErrHijacked), not on the loop - // thread, so the lines below do not all stand on the same ground: + // WHAT RUNS WHERE (celeris#668). Off-thread, this may touch only what is + // serialized against the loop: // // - EPOLL_CTL_DEL is a kernel call and needs no lock; after it the // worker sees no further EPOLLIN for this fd. // - the l.conns[fd] write takes driverMu, so it cannot race growConns' // copy + header swap, RegisterConn's occupancy check, or closeConn's // capture and ownership re-check (celeris#654). - // - activeConns and closeCount are atomics. - // - removeLiveConn and connCount-- are NEITHER. Both are documented - // worker-thread-only state (see the Loop fields), and mutating them - // from this goroutine is unsound: checkTimeouts and shutdown read - // len(l.liveConns) once and then index it, so a concurrent - // swap-and-shrink can index out of range or skip a live conn, and - // connCount-- can lose an update against acceptAll's connCount++, - // which leaves the connCount == 0 DRAINING→SUSPENDED gate - // permanently unsatisfiable on this loop. + // - activeConns and closeCount are atomics, and so is hijacked. // - // That last bullet is PRE-EXISTING and is NOT fixed by celeris#654, which - // removes the duplicate teardown closeConn used to perform but not the - // fact that this teardown runs off-thread. The counters below are not - // race-free; do not cite them as such. Tracked in celeris#668. + // The live set and connCount are the loop's (see the Loop fields), and so + // are the sweep's dormancy fields that removeLiveConn writes. Changing + // them from here raced the index walks of checkTimeouts, the sweep and + // shutdown, and connCount-- could lose an update against acceptAll's + // connCount++, after which the connCount == 0 DRAINING→SUSPENDED gate + // never passes on this loop again. So off-thread both are left to + // drainDetachQueue's hijacked branch, which runs on the loop once this + // goroutine has exited and handed cs back — the hand-off that already + // defers the pool release (below). Until then the entry stays in the live + // set and the walkers skip it on hijacked, which is stored BEFORE the + // descriptor is released: by the time the kernel can reissue the number + // to another accept, the entry already reads as not the loop's. _ = unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_DEL, fd, nil) - l.removeLiveConn(cs) + if offThread { + cs.hijacked.Store(true) + } else { + l.removeLiveConn(cs) + } l.driverMu.Lock() l.conns[fd] = nil l.driverMu.Unlock() - l.connCount-- + if !offThread { + l.connCount-- + } l.activeConns.Add(-1) l.closeCount.Add(1) @@ -1540,33 +1654,21 @@ func (l *Loop) hijackConn(fd int) (net.Conn, error) { c, err := net.FileConn(f) _ = f.Close() - // CRITICAL (#3.1): defer the pool release whenever a dispatch goroutine - // (runAsyncHandler) is alive for this conn. When the handler calls - // Hijack from inside that goroutine's ProcessH1, the goroutine STILL - // touches cs after ProcessH1 returns ErrHijacked — it Unlocks - // cs.detachMu, resyncs cs.pendingBytes, sets cs.asyncClosed, clears - // cs.asyncInBuf, and enqueues cs on detachQueue. Recycling cs now would - // hand pooled-and-reissued memory to that goroutine. Mark it hijacked - // and let the worker-thread teardown (drainDetachQueue, reached via the - // goroutine's asyncClosed + detachQueue handoff) run the pool release - // once the goroutine has exited. + // CRITICAL (#3.1): off-thread, defer the pool release as well. When the + // handler calls Hijack from inside the dispatch goroutine's ProcessH1, the + // goroutine STILL touches cs after ProcessH1 returns ErrHijacked — it + // Unlocks cs.detachMu, resyncs cs.pendingBytes, sets cs.asyncClosed, + // clears cs.asyncInBuf, and enqueues cs on detachQueue. Recycling cs now + // would hand pooled-and-reissued memory to that goroutine. The + // worker-thread teardown (drainDetachQueue, reached via the goroutine's + // asyncClosed + detachQueue handoff) runs the pool release once the + // goroutine has exited. // - // When no dispatch goroutine is running, hijackConn was called inline on - // the worker thread (sync mode, or an async-mode conn not yet promoted - // running its first request inline). drainRead returns immediately on - // ErrHijacked without touching cs again and nothing will enqueue cs, so - // release synchronously here — gating only on detachMu would leak the - // connState in the inline-async case. asyncRun is read under asyncInMu - // (the same lock the dispatch goroutine sets it under). - deferRelease := false - if cs.detachMu != nil { - cs.asyncInMu.Lock() - deferRelease = cs.asyncRun - cs.asyncInMu.Unlock() - } - if deferRelease { - cs.hijacked = true - } else { + // Inline, drainRead returns immediately on ErrHijacked without touching + // cs again and nothing will enqueue cs, so release synchronously here — + // gating only on detachMu would leak the connState in the inline-async + // case. + if !offThread { // Unlink before the pool release. releaseConnState clears cs's own // dirty links but never repairs l.dirtyHead or a predecessor's // dirtyNext, so handing back a still-linked connState leaves the @@ -2057,7 +2159,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncClosed.Store(true) cs.asyncInMu.Lock() cs.asyncInBuf = cs.asyncInBuf[:0] - cs.asyncRun = false + cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() // Signal worker via detachQueue + eventfd (never close the // fd from this goroutine — races with drainRead on the @@ -2092,8 +2194,14 @@ func (l *Loop) runAsyncHandler(cs *connState) { } cs.asyncParked = false if cs.asyncClosed.Load() { - cs.asyncRun = false + // The one exit that does not enqueue cs: a close requested while + // this goroutine ran may have been left to it (celeris#669), and + // then this is where it is handed back. + owed := cs.endDispatch() cs.asyncInMu.Unlock() + if owed { + l.enqueueDetach(cs) + } return } // #383: transplant quiesce requested and input drained — exit cleanly @@ -2101,7 +2209,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { // io_uring. tryTransplant only sets asyncQuiesce on a parked, flushed // conn whose fd it has already detached, so asyncInBuf is empty here. if cs.asyncQuiesce.Load() && len(cs.asyncInBuf) == 0 { - cs.asyncRun = false + cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() l.detachQMu.Lock() l.detachQueue = append(l.detachQueue, cs) @@ -2141,9 +2249,13 @@ func (l *Loop) runAsyncHandler(cs *connState) { if acquiredDetachMu { cs.detachMu.Unlock() } + // Nor does this one; see the loop-top exit. cs.asyncInMu.Lock() - cs.asyncRun = false + owed := cs.endDispatch() cs.asyncInMu.Unlock() + if owed { + l.enqueueDetach(cs) + } return } processErr := conn.ProcessH1(cs.ctx, data, cs.h1State, l.handler, cs.writeFn) @@ -2173,12 +2285,12 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncClosed.Store(true) cs.asyncInMu.Lock() cs.asyncInBuf = cs.asyncInBuf[:0] - cs.asyncRun = false + cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() } else { cs.asyncInMu.Lock() cs.asyncInBuf = cs.asyncInBuf[:0] - cs.asyncRun = false + cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() cs.asyncH2Promoted.Store(true) } @@ -2204,7 +2316,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncClosed.Store(true) cs.asyncInMu.Lock() cs.asyncInBuf = cs.asyncInBuf[:0] - cs.asyncRun = false + cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() l.detachQMu.Lock() l.detachQueue = append(l.detachQueue, cs) @@ -2279,7 +2391,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncClosed.Store(true) cs.asyncInMu.Lock() cs.asyncInBuf = cs.asyncInBuf[:0] - cs.asyncRun = false + cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() l.detachQMu.Lock() l.detachQueue = append(l.detachQueue, cs) @@ -2291,6 +2403,30 @@ func (l *Loop) runAsyncHandler(cs *connState) { } } +// endDispatch marks cs's dispatch goroutine as gone and reports whether a +// close was left to it (closeOwed, celeris#669). The goroutine calls it on +// every exit path, holding cs.asyncInMu. A path that enqueues cs on its way +// out ignores the result: that enqueue is the hand-back, and +// drainDetachQueue's asyncClosed branch runs the close. The two paths that +// exit WITHOUT enqueuing must enqueue when it reports true, or the close is +// lost and the conn stays open, owned by no goroutine. +func (cs *connState) endDispatch() (closeOwed bool) { + cs.asyncRun = false + closeOwed = cs.closeOwed + cs.closeOwed = false + return closeOwed +} + +// enqueueDetach hands cs to the loop thread's drainDetachQueue and wakes the +// loop. Dispatch goroutine. +func (l *Loop) enqueueDetach(cs *connState) { + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + l.wakeFD.Signal() +} + func (l *Loop) drainDetachQueue() { if l.detachQPending.Load() == 0 { return @@ -2317,13 +2453,21 @@ func (l *Loop) drainDetachQueue() { continue } // Hijacked conn (#3.1): hijackConn already detached the fd from - // epoll/live set/conn table and handed it to the caller as a + // epoll and the conn table and handed it to the caller as a // net.Conn (the original fd is closed; the caller owns a dup). It - // deferred ONLY the pool release to here, where the dispatch - // goroutine that referenced cs has now exited (it enqueued cs on - // its way out). Release cs and skip closeConn — there is no fd to - // close and l.conns[fd] is already nil. - if cs.hijacked { + // deferred to here, where the dispatch goroutine that referenced cs + // has now exited (it enqueued cs on its way out), the pool release + // and the two pieces of loop-thread state it may not touch from + // that goroutine: the live-set entry and connCount (celeris#668). + // Skip closeConn — there is no fd to close and l.conns[fd] is + // already nil. + // + // This branch runs once per hijack: releaseConnState clears the + // flag, so a second queue entry for the same connState cannot + // decrement connCount again. + if cs.hijacked.Load() { + l.removeLiveConn(cs) + l.connCount-- // Unlink from the dirty list first: hijackConn does not, and // releaseConnState clears only cs's own links, so the pool would // otherwise get a connState that l.dirtyHead or a predecessor @@ -2410,7 +2554,32 @@ func (l *Loop) drainDetachQueue() { func (l *Loop) flushDirty() { for cs := l.dirtyHead; cs != nil; { next := cs.dirtyNext - if mu := cs.detachMu; mu != nil { + if mu := cs.detachMu; mu != nil && !mu.TryLock() { + if dispatchBusy(cs) { + // The conn's dispatch goroutine holds detachMu across a + // user handler (celeris#669): waiting here parked the loop, + // and every connection on it, until the handler returned. + // Take the conn off the list instead of retrying it — a + // dirty list that never empties holds epoll_wait at 0 ms. + // Nothing is lost: whoever holds the lock flushes writeBuf + // from writePos when it is done (the goroutine after + // ProcessH1, a guarded writeFn after its write) and hands a + // remainder back through the detach queue, which puts the + // conn back on this list. + // + // Except a deferred peer close (peerClosed): it fires only + // when THIS pass sees the flush complete, and a holder + // whose flush completes hands nothing back. Such a conn + // stays on the list and is retried each pass until the + // handler returns — a spin, but only for a peer that + // half-closed with a response still queued while a + // pipelined request's handler runs. + if !cs.peerClosed { + l.removeDirty(cs) + } + cs = next + continue + } mu.Lock() } err := l.flushWrites(cs, true) @@ -2519,7 +2688,21 @@ func (l *Loop) disarmEpollOut(cs *connState) { // read-only interest. A still-partial flush leaves EPOLLOUT armed so the // next writable edge resumes. Returns false if the conn was closed. func (l *Loop) handleWritable(cs *connState) { - if mu := cs.detachMu; mu != nil { + if mu := cs.detachMu; mu != nil && !mu.TryLock() { + if dispatchBusy(cs) { + // A pipelined request started a handler before the socket + // drained (celeris#669). Do not wait for it, and drop the + // level-triggered interest, which would otherwise fire on every + // epoll_wait until the handler returns. The goroutine flushes + // writeBuf itself when the handler returns and hands any + // remainder back to the dirty list, which re-arms EPOLLOUT. + // A deferred peer close keeps the interest, for the reason + // given in flushDirty. + if !cs.peerClosed { + l.disarmEpollOut(cs) + } + return + } mu.Lock() } err := l.flushWrites(cs, true) @@ -2582,7 +2765,7 @@ func (l *Loop) removeDirty(cs *connState) { // stamps cs.liveIdx so removeLiveConn is O(1). Worker-thread-only. (#318) func (l *Loop) addLiveConn(cs *connState) { cs.liveIdx = len(l.liveConns) - l.liveConns = append(l.liveConns, cs.fd) + l.liveConns = append(l.liveConns, cs) // A dormant sweep judged the set this loop held; it holds a different // one now (celeris#657 P7). l.wakeSweep() @@ -2598,17 +2781,20 @@ func (l *Loop) removeLiveConn(cs *connState) { return } i := cs.liveIdx - if i < 0 || i >= len(l.liveConns) || l.liveConns[i] != cs.fd { + if i < 0 || i >= len(l.liveConns) || l.liveConns[i] != cs { return } last := len(l.liveConns) - 1 if i != last { - movedFD := l.liveConns[last] - l.liveConns[i] = movedFD - if moved := l.conns[movedFD]; moved != nil { - moved.liveIdx = i - } - } + // The fix-up follows the entry itself, not l.conns[fd]: the + // entry may be a hijacked conn whose slot is already clear, or + // whose descriptor number now belongs to another conn + // (celeris#668). + moved := l.liveConns[last] + l.liveConns[i] = moved + moved.liveIdx = i + } + l.liveConns[last] = nil // connStates are pooled: no stale reference past len l.liveConns = l.liveConns[:last] cs.liveIdx = -1 // The residue this loop last published counted this conn; it no longer @@ -2684,11 +2870,15 @@ const detachDrainGrace = time.Second func (l *Loop) checkTimeouts() { now := time.Now().UnixNano() for i := len(l.liveConns) - 1; i >= 0; i-- { - fd := l.liveConns[i] - cs := l.conns[fd] - if cs == nil { + cs := l.liveConns[i] + if cs.hijacked.Load() { + // Handed to the application by an async Hijack, and waiting + // for its dispatch goroutine to hand it back (celeris#668): + // not this loop's to time out, and its descriptor number may + // already be another conn's. continue } + fd := cs.fd // Detached connections (e.g. WebSocket): honor an explicit deadline // supplied by the middleware via SetWSIdleDeadline. Skip the // engine-config-driven idle/read/write timeouts since the middleware @@ -2791,27 +2981,47 @@ func (l *Loop) closeConn(fd int) { } // Signal the detached goroutine's writeFn to stop writing. // The mutex serializes with any in-progress write — if the - // goroutine is mid-write, we block until it finishes. Under - // AsyncHandlers that wait is unbounded by the handler: this is where - // a timeout reap parks the whole loop thread for as long as the user - // handler runs (celeris#669, the epoll twin of the closed #593). - cs.detachMu.Lock() + // goroutine is mid-write, we wait until it finishes. + // + // But not for a handler (celeris#669, the epoll twin of the closed + // #593). The dispatch goroutine holds this mutex across ProcessH1, + // i.e. for as long as the user handler runs, and a blocking Lock + // here parked the loop thread — every connection on it, no + // epoll_wait, no accept, no flush — until the handler returned: + // through the timeout reap, EPOLLRDHUP, EPOLLHUP and every error + // path that closes. When the lock is held and that goroutine is + // running, leave the close to it: asyncClosed is set, so it exits at + // its next check, and its exit hands cs back through the detach + // queue, whose asyncClosed branch calls here again with the lock + // free. Until then the conn stays whole — in the table, the live set + // and epoll, its descriptor open — because the handler is still + // writing its response into it. When the goroutine is parked or + // gone, the holder is a guarded writeFn in one write, and waiting + // for it is bounded; see dispatchBusy. + if !cs.detachMu.TryLock() { + if leaveCloseToDispatch(cs) { + return + } + cs.detachMu.Lock() + } // celeris#654: re-validate ownership now that we hold the lock. // Under AsyncHandlers the user handler runs inside ProcessH1 with - // detachMu held, and Context.Hijack → hijackConn does the ENTIRE - // engine-side teardown right there on the dispatch goroutine: - // EPOLL_CTL_DEL, removeLiveConn, l.conns[fd] = nil, the three - // counters, and close() of the original descriptor (the caller keeps - // a dup). We captured cs BEFORE that ran and then parked on this - // lock, so without a re-check we redo every one of those steps on a - // conn the engine no longer owns: the close is counted twice and the - // live gauge goes negative (which also means the DRAINING→SUSPENDED - // connCount == 0 gate can never pass again on this loop), and the - // OnDisconnect fires for a connection that was handed to the - // application, and the SHUT_WR + Close below lands on a descriptor - // NUMBER the kernel's lowest-free-fd rule has already reissued to - // something else — another loop's accept, an fd the handler opened, a - // driver conn registered off-thread into this same epfd. + // detachMu held, and Context.Hijack → hijackConn detaches the conn + // right there on the dispatch goroutine: EPOLL_CTL_DEL, + // l.conns[fd] = nil, the public counters, and close() of the + // original descriptor (the caller keeps a dup). A closeConn that + // captured cs BEFORE that ran and then waited on this lock would, + // without a re-check, redo every one of those steps on a conn the + // engine no longer owns: the close counted twice, the live gauge + // negative, an OnDisconnect for a connection handed to the + // application, and a SHUT_WR + Close on a descriptor NUMBER the + // kernel's lowest-free-fd rule has already reissued to something + // else — another loop's accept, an fd the handler opened, a driver + // conn registered off-thread into this same epfd. + // + // Since celeris#669 closeConn no longer waits on a running handler, + // so this wait-then-hijack interleaving needs a holder that is not + // the dispatch goroutine; the check stays as the guard for it. // // One check suffices. The slot is only cleared off-thread by // hijackConn, which in async mode runs under this very mutex, and @@ -2842,9 +3052,12 @@ func (l *Loop) closeConn(fd int) { // ProcessH1 — and therefore Hijack — cannot run on an H2 // conn); if that ever changes, the unlink belongs in // hijackConn, the site that knows it is hijacking. - // - the three counters, EPOLL_CTL_DEL, the fd close, - // removeLiveConn and the slot nil: all already done by - // hijackConn. + // - the public counters, EPOLL_CTL_DEL, the fd close and the + // slot nil: all already done by hijackConn. + // - removeLiveConn and connCount: not ours to do either. An + // off-thread hijack leaves both to drainDetachQueue's + // hijacked branch, on this thread, once the dispatch + // goroutine has exited (celeris#668). // - CloseH1 and the pool release: not ours to do. Leaving // detachClosed false is what lets drainDetachQueue reach its // cs.hijacked branch and return the connState to the pool @@ -2858,6 +3071,17 @@ func (l *Loop) closeConn(fd int) { cs.detachMu.Unlock() return } + // A drainRead branch that met a running handler left its I/O error + // here rather than wait for the lock (closeErr, celeris#669). Do + // what that branch did, now that the lock is held: flush what is + // queued, then tell a detached middleware. + if err := cs.closeErr; err != nil { + cs.closeErr = nil + _ = l.flushWrites(cs, true) + if cs.h1State != nil && cs.h1State.OnError != nil { + cs.h1State.OnError(err) + } + } cs.detachClosed = true // Acquire barrier: only invoke OnDetachClose once the WS upgrade has // fully wired the conn (WSReady). Otherwise the read of OnDetachClose — @@ -2995,10 +3219,9 @@ func (l *Loop) shutdown() { // index for consistency with checkTimeouts (no removal happens here, but // keeping the idiom avoids surprises). for i := len(l.liveConns) - 1; i >= 0; i-- { - fd := l.liveConns[i] - cs := l.conns[fd] - if cs == nil { - continue + cs := l.liveConns[i] + if cs.hijacked.Load() { + continue // the application's since the Hijack (celeris#668) } detached := cs.detachMu != nil if detached { @@ -3056,11 +3279,14 @@ func (l *Loop) shutdown() { // asyncWG and may still hold a reference, so we let GC reclaim cs once // that goroutine drops its closure refs rather than recycling it here. for i := len(l.liveConns) - 1; i >= 0; i-- { - fd := l.liveConns[i] - cs := l.conns[fd] - if cs == nil { + cs := l.liveConns[i] + if cs.hijacked.Load() { + // hijackConn closed this conn's descriptor already, and the + // number may be reissued: closing it here would close another + // conn's (celeris#668). continue } + fd := cs.fd _ = unix.Close(fd) if cs.detachMu == nil { l.dropAsk(cs) // celeris#657 P8: never pool a connState an ask still names @@ -3068,6 +3294,7 @@ func (l *Loop) shutdown() { } l.conns[fd] = nil } + clear(l.liveConns) l.liveConns = l.liveConns[:0] // celeris#657 R2: this loop holds nothing now, so it must not leave its // last cycle's residue standing in the engine-wide gauges. Nothing else diff --git a/engine/epoll/review_v150_test.go b/engine/epoll/review_v150_test.go index bc692c0b..e453ae34 100644 --- a/engine/epoll/review_v150_test.go +++ b/engine/epoll/review_v150_test.go @@ -32,7 +32,7 @@ func regLive(l *Loop, fd int) *connState { // live_conns test, but using the index-based removeLiveConn (NOT the O(N) // scan). func TestLiveConnsAddRemoveO1(t *testing.T) { - l := &Loop{conns: make([]*connState, 1024), liveConns: make([]int, 0, 16)} + l := &Loop{conns: make([]*connState, 1024), liveConns: make([]*connState, 0, 16)} cs5 := regLive(l, 5) cs10 := regLive(l, 10) @@ -46,8 +46,8 @@ func TestLiveConnsAddRemoveO1(t *testing.T) { // Remove the middle — swap-with-last moves 15 into slot 1: [5, 15]. l.removeLiveConn(cs10) - if l.liveConns[0] != 5 || l.liveConns[1] != 15 || len(l.liveConns) != 2 { - t.Fatalf("liveConns = %v, want [5 15]", l.liveConns) + if len(l.liveConns) != 2 || l.liveConns[0] != cs5 || l.liveConns[1] != cs15 { + t.Fatalf("liveConns = %d entries, want [cs5 cs15]", len(l.liveConns)) } if cs15.liveIdx != 1 { t.Errorf("swapped-in cs15.liveIdx = %d, want 1", cs15.liveIdx) @@ -83,7 +83,7 @@ func TestCheckTimeoutsClosesAllExpired(t *testing.T) { l := &Loop{ epollFD: epfd, conns: make([]*connState, 1024), - liveConns: make([]int, 0, 8), + liveConns: make([]*connState, 0, 8), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, @@ -176,7 +176,7 @@ func TestAcceptAllDrainsBacklogNoStrand(t *testing.T) { epollFD: epfd, listenFD: lfd, conns: make([]*connState, connTableSize), - liveConns: make([]int, 0, 256), + liveConns: make([]*connState, 0, 256), activeConns: &atomic.Int64{}, errs: &errclass.Counters{}, reqCount: &atomic.Uint64{}, @@ -271,7 +271,7 @@ func TestHijackDefersReleaseWhileAsyncGoroutineActive(t *testing.T) { l := &Loop{ epollFD: epfd, conns: make([]*connState, connTableSize), - liveConns: make([]int, 0, 8), + liveConns: make([]*connState, 0, 8), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, @@ -299,15 +299,18 @@ func TestHijackDefersReleaseWhileAsyncGoroutineActive(t *testing.T) { t.Cleanup(func() { _ = c.Close() }) } - // Detached synchronously: slot nil'd, removed from live set. + // Detached from the conn table synchronously. The live set and + // connCount are the loop's and are left to the hand-back below + // (celeris#668). if l.conns[local] != nil { t.Errorf("hijackConn did not nil l.conns[%d]", local) } - if cs.liveIdx != -1 || len(l.liveConns) != 0 { - t.Errorf("hijackConn did not remove from liveConns (liveIdx=%d, len=%d)", cs.liveIdx, len(l.liveConns)) + if cs.liveIdx != 0 || len(l.liveConns) != 1 || l.connCount != 1 { + t.Errorf("hijackConn changed loop-thread-only state off-thread (liveIdx=%d, len=%d, connCount=%d)", + cs.liveIdx, len(l.liveConns), l.connCount) } // Pool release MUST be deferred while the async goroutine is alive. - if !cs.hijacked { + if !cs.hijacked.Load() { t.Fatal("hijackConn released cs synchronously while async goroutine active (UAR risk)") } // releaseConnState zeroes fd; deferred means fd is still intact. @@ -326,9 +329,12 @@ func TestHijackDefersReleaseWhileAsyncGoroutineActive(t *testing.T) { l.drainDetachQueue() // releaseConnState clears hijacked + zeroes fd. - if cs.hijacked { + if cs.hijacked.Load() { t.Error("drainDetachQueue did not release hijacked cs (hijacked still set)") } + if len(l.liveConns) != 0 || l.connCount != 0 { + t.Errorf("the hand-back left liveConns=%d connCount=%d, want 0 and 0", len(l.liveConns), l.connCount) + } if cs.fd != 0 { t.Errorf("cs.fd = %d after release, want 0", cs.fd) } @@ -358,7 +364,7 @@ func TestHijackSyncReleasesImmediately(t *testing.T) { l := &Loop{ epollFD: epfd, conns: make([]*connState, connTableSize), - liveConns: make([]int, 0, 8), + liveConns: make([]*connState, 0, 8), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, @@ -380,7 +386,7 @@ func TestHijackSyncReleasesImmediately(t *testing.T) { if c != nil { t.Cleanup(func() { _ = c.Close() }) } - if cs.hijacked { + if cs.hijacked.Load() { t.Error("sync hijack deferred release (hijacked set) — should release inline") } if cs.fd != 0 { diff --git a/engine/epoll/sweep.go b/engine/epoll/sweep.go index 2cbf2608..266f0cc6 100644 --- a/engine/epoll/sweep.go +++ b/engine/epoll/sweep.go @@ -262,14 +262,15 @@ func (l *Loop) sweep() { if i >= len(l.liveConns) { continue } - fd := l.liveConns[i] - if fd < 0 || fd >= len(l.conns) { - continue - } - cs := l.conns[fd] - if cs == nil { + cs := l.liveConns[i] + if cs.hijacked.Load() { + // Handed to the application by an async Hijack and waiting for + // its dispatch goroutine to hand it back (celeris#668): not this + // loop's, so neither examined nor counted as residue — exactly + // as when hijackConn removed it from the live set itself. continue } + fd := cs.fd examined++ // Permanent residue: a detached WebSocket or SSE conn, an H2 one, a // hijacked one, one that has never spoken. tryTransplant refuses @@ -408,7 +409,7 @@ func (l *Loop) classifyLocked(cs *connState) int { if cs.h2State != nil || cs.asyncH2Promoted.Load() || (cs.detected && cs.protocol != engine.HTTP1) { return resH2 } - if cs.hijacked { + if cs.hijacked.Load() { return resPinned } if !cs.detected || cs.h1State == nil { diff --git a/engine/epoll/transplant.go b/engine/epoll/transplant.go index 75745436..9a8c939b 100644 --- a/engine/epoll/transplant.go +++ b/engine/epoll/transplant.go @@ -85,6 +85,13 @@ func (l *Loop) tryTransplant(fd int) { // touched only by this loop thread, so the reads are inherently safe. hasGoroutine := false if l.async { + // A close is owed (celeris#669): closeConn left it to the dispatch + // goroutine, which may already have exited without the loop having + // drained its hand-back. Never hand such a conn to the target — the + // hand-back would then close through a connState the move released. + if cs.asyncClosed.Load() { + return + } cs.asyncInMu.Lock() running := cs.asyncRun parked := cs.asyncParked diff --git a/engine/epoll/transplant_accounting_test.go b/engine/epoll/transplant_accounting_test.go index e8dc6fba..ed16f718 100644 --- a/engine/epoll/transplant_accounting_test.go +++ b/engine/epoll/transplant_accounting_test.go @@ -41,7 +41,7 @@ func newLedgerLoop(t *testing.T) *Loop { return &Loop{ epollFD: epfd, conns: make([]*connState, 4096), - liveConns: make([]int, 0, 8), + liveConns: make([]*connState, 0, 8), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, diff --git a/engine/epoll/wakefd_after_shutdown_test.go b/engine/epoll/wakefd_after_shutdown_test.go index c23b27b6..c299f5f4 100644 --- a/engine/epoll/wakefd_after_shutdown_test.go +++ b/engine/epoll/wakefd_after_shutdown_test.go @@ -149,7 +149,7 @@ func TestDetachedResumeRecvAfterShutdownDoesNotWriteTheClosedWakeupFD(t *testing timerFD: -1, wakeFD: wakefd.New(efd), conns: make([]*connState, connTableSize), - liveConns: make([]int, 0, 4), + liveConns: make([]*connState, 0, 4), activeConns: &atomic.Int64{}, closeCount: &atomic.Uint64{}, acceptCount: &atomic.Uint64{}, From e3c043f097efdf519d85333f0bca37a5e41d39d2 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 14:45:36 +0200 Subject: [PATCH 04/20] test(epoll): pin the accept-install lock and the owed-close transplant guard (celeris#668, celeris#669) TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack: the loop learns that a hijacked descriptor number is free from the kernel alone, as accept4 does, so under -race only driverMu can order accept's slot install after the hijack's clear. TestAnOwedCloseIsNeverTransplanted: between the dispatch goroutine's exit and the drain of its hand-back the conn must not be handed to the other engine. Drops the dispatch-spawn guard on asyncClosed added with the fix: a goroutine started on a conn whose close is owed exits at its first check and owes nothing, so the guard changed no outcome a test could pin. --- .../epoll/async_handler_stall_linux_test.go | 33 +++++ engine/epoll/hijack_offthread_linux_test.go | 119 ++++++++++++++++++ engine/epoll/loop.go | 7 +- 3 files changed, 153 insertions(+), 6 deletions(-) diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go index 7cf1f8fc..3f5fe69f 100644 --- a/engine/epoll/async_handler_stall_linux_test.go +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -278,6 +278,39 @@ func TestDeferredPeerCloseSurvivesARunningHandler(t *testing.T) { } } +// TestAnOwedCloseIsNeverTransplanted: between the dispatch goroutine's exit +// and the loop draining its hand-back, the conn is still in the table with no +// goroutine — the shape tryTransplant hands straight to the other engine. It +// must not: the move would release the connState the queued hand-back then +// closes through. +func TestAnOwedCloseIsNeverTransplanted(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + l.async = true + cs.protocol = engine.HTTP1 + cs.detected = true + release := holdAsHandler(t, cs, true) + if !returnsWhileHeld(t, release, l.checkTimeouts) { + t.Fatal("the reap waited on a running handler (celeris#669)") + } + release() + exitDispatch(l, cs) // the goroutine is gone; its hand-back is queued, not drained + + target := &countingTarget{} + t.Cleanup(target.closeAll) + l.transplant.Store(&transplantState{target: target}) + l.tryTransplant(local) + l.transplant.Store(nil) + if n := target.count(); n != 0 { + t.Fatalf("a conn whose close is owed was handed to the other engine (%d adopted)", n) + } + l.drainDetachQueue() + if l.conns[local] != nil || l.closeCount.Load() != 1 || rig.disconnects.Load() != 1 { + t.Errorf("the owed close did not complete: slot=%p closeCount=%d hooks=%d", + l.conns[local], l.closeCount.Load(), rig.disconnects.Load()) + } +} + // TestCloseStillWaitsForABoundedHolder is the negative control for the unit // arms. The dispatch goroutine is PARKED, so whoever holds detachMu is a // guarded writeFn in the middle of one write — a hold bounded by a syscall, diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index 45e2b6f6..9435307f 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -17,6 +17,7 @@ import ( "golang.org/x/sys/unix" "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/engine/internal/errclass" "github.com/goceleris/celeris/internal/conn" "github.com/goceleris/celeris/protocol/h2/stream" "github.com/goceleris/celeris/resource" @@ -423,3 +424,121 @@ func TestAsyncHijackUnderAcceptChurnLeavesTheLoopsSuspendable(t *testing.T) { } } } + +// TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack pins why accept +// installs its slot under driverMu. An off-thread hijack clears its slot under +// that lock and then closes the descriptor; the kernel hands the lowest free +// number to the next accept4, so the loop can install a new conn at the very +// slot the dispatch goroutine just wrote, with nothing else ordering the two. +// The loop side below learns that the number is free from the kernel alone +// (F_GETFD), exactly as accept4 does, so under -race the only thing that can +// order the two slot writes is the lock. +func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { + epfd, err := unix.EpollCreate1(unix.EPOLL_CLOEXEC) + if err != nil { + t.Fatalf("epoll_create1: %v", err) + } + t.Cleanup(func() { _ = unix.Close(epfd) }) + lfd, err := createListenSocket("127.0.0.1:0") + if err != nil { + t.Fatalf("listen socket: %v", err) + } + t.Cleanup(func() { _ = unix.Close(lfd) }) + addr := boundAddr(lfd).String() + l := &Loop{ + epollFD: epfd, + listenFD: lfd, + async: true, + conns: make([]*connState, connTableSize), + activeConns: &atomic.Int64{}, + errs: &errclass.Counters{}, + reqCount: &atomic.Uint64{}, + acceptCount: &atomic.Uint64{}, + closeCount: &atomic.Uint64{}, + bytesRead: &atomic.Uint64{}, + bytesWritten: &atomic.Uint64{}, + timerFD: -1, + resolved: resource.ResolvedResources{BufferSize: 4096}, + } + dial := func() net.Conn { + c, err := net.DialTimeout("tcp", addr, 3*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + return c + } + acceptOne := func() { + for dl := time.Now().Add(3 * time.Second); time.Now().Before(dl); { + before := len(l.liveConns) + l.acceptAll(context.Background(), time.Now().UnixNano()) + if len(l.liveConns) > before { + return + } + time.Sleep(time.Millisecond) + } + t.Fatal("accept timed out") + } + + dial() + acceptOne() + cs := l.liveConns[0] + fd := cs.fd + cs.protocol = engine.HTTP1 + cs.detected = true + cs.h1State = conn.NewH1State() + cs.asyncInMu.Lock() + cs.asyncRun = true // the conn's dispatch goroutine is alive: the hijack is off-thread + cs.asyncInMu.Unlock() + dial() // queued before the hijack frees fd, so the next accept4 takes fd + + hijacked := make(chan net.Conn, 1) + go func() { // the dispatch goroutine, inside the handler + cs.detachMu.Lock() + nc, herr := l.hijackConn(fd) + cs.detachMu.Unlock() + if herr != nil { + nc = nil + } + hijacked <- nc + }() + accepted := make(chan struct{}) + go func() { // the loop thread + defer close(accepted) + for { + if _, ferr := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0); ferr != nil { + break + } + time.Sleep(50 * time.Microsecond) + } + for range 3000 { + if l.acceptAll(context.Background(), time.Now().UnixNano()) != acceptDrained || len(l.liveConns) > 1 { + return + } + time.Sleep(time.Millisecond) + } + }() + <-accepted + nc := <-hijacked + if nc == nil { + t.Fatal("hijackConn failed") + } + t.Cleanup(func() { _ = nc.Close() }) + + if len(l.liveConns) != 2 { + t.Fatalf("liveConns = %d entries after the second accept, want 2 (the hijacked conn until its hand-back, "+ + "and the new one)", len(l.liveConns)) + } + next := l.liveConns[1] + if next.fd != fd { + t.Skipf("the kernel gave the new conn fd %d, not the released %d; the reuse under test did not happen", next.fd, fd) + } + if l.conns[fd] != next { + t.Fatalf("slot %d holds %p, want the new conn %p", fd, l.conns[fd], next) + } + handBack(l, cs) + assertLiveSet(t, l, next) + if l.connCount != 1 { + t.Errorf("connCount = %d after the hand-back, want 1", l.connCount) + } +} diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index 54e66096..ca32dc7d 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -1242,12 +1242,7 @@ func (l *Loop) drainRead(fd int, now int64) { // ProcessH1, so the worker's next read into cs.buf can't // overwrite in-flight bytes. Zero allocation on steady state. cs.asyncInBuf = append(cs.asyncInBuf, data...) - // Never start a dispatch goroutine on a conn whose close is - // owed: its last goroutine can exit before the loop drains the - // hand-back that finishes the close (celeris#669), and a new one - // would only exit again. Evaluated only when no goroutine runs, - // so the steady state pays nothing. - starting := !cs.asyncRun && !cs.asyncClosed.Load() + starting := !cs.asyncRun if starting { cs.asyncRun = true } From 6ce9cdb7124d0d1f283a496c58bbdd41f7d36a20 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 14:46:46 +0200 Subject: [PATCH 05/20] test(epoll): correct the celeris#654 rig note about drainRead's EOF branch, which no longer waits (celeris#669) --- engine/epoll/hijack_closeconn_race_linux_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/engine/epoll/hijack_closeconn_race_linux_test.go b/engine/epoll/hijack_closeconn_race_linux_test.go index 51583329..ea399c63 100644 --- a/engine/epoll/hijack_closeconn_race_linux_test.go +++ b/engine/epoll/hijack_closeconn_race_linux_test.go @@ -26,10 +26,10 @@ import ( // The loop-thread entry these tests use is the real one: checkTimeouts. It // calls closeConn directly with no async guard, and nothing refreshes // cs.lastActivity while a handler runs (it is written only at accept, -// on read, and on adopt). drainRead's EOF and error branches — the -// EPOLLRDHUP route the issue proposed — take detachMu BEFORE calling -// closeConn, so they wait behind the handler and then re-read a cleared -// slot; that is why the issue's own proposed repro would mostly pass. +// on read, and on adopt). (When these tests were written, drainRead's EOF +// and error branches — the EPOLLRDHUP route the issue proposed — took +// detachMu BEFORE calling closeConn, so they waited behind the handler and +// then re-read a cleared slot; since celeris#669 they do not wait at all.) // // The interleaving is pinned by a lock and an atomic rather than by timing: // closeConn stores cs.asyncClosed only AFTER capturing l.conns[fd] and From fdd747f05f4045097e390ce0274da6a00dd9fc8b Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 15:05:00 +0200 Subject: [PATCH 06/20] fix(epoll): hand given-up conns back instead of dropping them, settle an async hijack at once, and never pool its connState (celeris#669, celeris#668) Review of the first fix found five holes; each now has a test and a mutant it kills. - A conn the dirty pass or the EPOLLOUT resume gave up mid-handler was only re-examined if the holder left a remainder. A deferred peer close (peerClosed) set after that, or a flush that completed, was lost. The pass now records relinkOwed, and the dispatch goroutine hands the conn back at its next park, after its own flush; the loop puts it on the dirty list again. That also removes the spin the peerClosed exception had. relinkPending keeps tryTransplant off the conn until then, since the hand-back is a queue entry and a transplant pools cs. - dispatchBusy now excludes a goroutine that released detachMu at Detach: it never holds the lock across a handler again, so the loop waits out a guarded writeFn as before, instead of leaving a close to a goroutine whose inline post-Detach handler waits for that close. - An off-thread hijack settled the loop's state only when the dispatch goroutine exited, i.e. after an in-handler hijacked session, and never after Detach then Hijack. hijackConn now enqueues cs at once; the drain settles it (live set, connCount, dirty list, ask) exactly once. - Such a connState is no longer returned to the pool: queue entries made before the hijack (a partial pipelined flush) could otherwise name reissued memory, and the second of two entries ended in markDirty on a pooled connState. - The dirty pass skips a hijacked conn, whose descriptor number may already be another file's. Also: one dispatchBusy(cs, owe) replaces the two helpers, the six inline detach-queue enqueues in runAsyncHandler use enqueueDetach, and the askAtPark comment no longer calls hijacked loop-thread state. --- engine/epoll/ask.go | 6 +- .../epoll/async_handler_stall_linux_test.go | 142 ++++++-- engine/epoll/conn.go | 38 ++- .../epoll/hijack_closeconn_race_linux_test.go | 26 +- engine/epoll/hijack_offthread_linux_test.go | 94 +++++- engine/epoll/loop.go | 305 +++++++++--------- engine/epoll/review_v150_test.go | 20 +- engine/epoll/transplant.go | 6 +- 8 files changed, 439 insertions(+), 198 deletions(-) diff --git a/engine/epoll/ask.go b/engine/epoll/ask.go index f75c0c9e..adf3569b 100644 --- a/engine/epoll/ask.go +++ b/engine/epoll/ask.go @@ -42,9 +42,9 @@ package epoll // // It reads only what THIS goroutine owns: cs.h1State and cs.h2State are // written by switchToH2Local on this goroutine, and Detached and -// asyncH2Promoted are atomics. cs.protocol, cs.detected and cs.hijacked are -// the loop thread's and are deliberately NOT read here — the sweep, which -// runs there, screens those. +// asyncH2Promoted are atomics. cs.protocol and cs.detected are the loop +// thread's, and cs.hijacked is set by a hijack on this very goroutine; none +// is read here — the sweep, which runs on the loop thread, screens them. func (l *Loop) askAtPark(cs *connState) { if l.transplant.Load() == nil { return diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go index 3f5fe69f..616335cb 100644 --- a/engine/epoll/async_handler_stall_linux_test.go +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -193,8 +193,8 @@ func TestPeerCloseDoesNotWaitForARunningAsyncHandler(t *testing.T) { // left on the dirty list by a partial flush is locked by the pass every // iteration; with its dispatch goroutine inside a handler the pass must move // on, and stop polling the conn (a dirty list that never empties holds the -// loop at a 0 ms epoll_wait, a spin). The goroutine flushes writeBuf itself -// when its handler returns and hands back any remainder. +// loop at a 0 ms epoll_wait, a spin). The goroutine owes the conn back +// (relink); TestAConnGivenUpMidHandlerIsHandedBack follows it home. func TestDirtyPassDoesNotWaitForARunningAsyncHandler(t *testing.T) { rig := hijackRaceConn(t) l, cs := rig.l, rig.cs @@ -211,6 +211,13 @@ func TestDirtyPassDoesNotWaitForARunningAsyncHandler(t *testing.T) { t.Errorf("the pass left the conn on the dirty list: the loop would spin at a 0 ms epoll_wait " + "for as long as the handler runs") } + cs.asyncInMu.Lock() + owed := cs.relinkOwed + cs.asyncInMu.Unlock() + if !owed || !cs.relinkPending { + t.Errorf("the pass gave the conn up without a hand-back owed (relinkOwed=%v relinkPending=%v)", + owed, cs.relinkPending) + } } // TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler: a conn that hit write @@ -238,20 +245,22 @@ func TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler(t *testing.T) { } } -// TestDeferredPeerCloseSurvivesARunningHandler guards the one exception to -// the two passes above. A conn whose peer half-closed while its response was -// still queued (peerClosed) is closed by whichever pass sees the flush -// complete; a holder whose own flush completes hands nothing back. So while a -// handler runs, the dirty pass keeps such a conn and the EPOLLOUT resume keeps -// its interest — and once the handler returns, the next pass closes it. -func TestDeferredPeerCloseSurvivesARunningHandler(t *testing.T) { +// TestAConnGivenUpMidHandlerIsHandedBack follows a conn the dirty pass or the +// EPOLLOUT resume gave up while a handler held detachMu. What those passes do +// when a flush completes — close a conn whose peer half-closed (peerClosed), +// drop the EPOLLOUT interest — must still happen, including when the peer's +// half-close is only noticed after the conn was given up, and although the +// holder's own flush may complete and leave no remainder to hand back. The +// REAL dispatch loop hands the conn back at its park; the loop puts it on the +// dirty list, flushes it and closes it. +func TestAConnGivenUpMidHandlerIsHandedBack(t *testing.T) { for _, site := range []string{"dirty", "epollout"} { t.Run(site, func(t *testing.T) { rig := hijackRaceConn(t) l, cs, local := rig.l, rig.cs, rig.local + l.async = true cs.writeBuf = append(cs.writeBuf[:0], "pending"...) cs.pendingBytes = len(cs.writeBuf) - cs.peerClosed = true pass := l.flushDirty if site == "dirty" { l.markDirty(cs) @@ -263,21 +272,118 @@ func TestDeferredPeerCloseSurvivesARunningHandler(t *testing.T) { if !returnsWhileHeld(t, release, pass) { t.Fatalf("the %s pass waited on a running handler (celeris#669)", site) } - if !cs.dirty && !cs.epollOut { - t.Fatalf("the %s pass dropped a conn with a deferred peer close: nothing would close it "+ - "once the handler's own flush completes", site) + if cs.dirty || cs.epollOut || !cs.relinkPending { + t.Fatalf("the %s pass did not give the conn up (dirty=%v epollOut=%v relinkPending=%v)", + site, cs.dirty, cs.epollOut, cs.relinkPending) + } + // The peer half-closes; the RDHUP branch finds a response still + // queued and defers the close until it is flushed. + cs.peerClosed = true + release() // the handler returns + + // The dispatch goroutine reaches its park. + l.asyncWG.Add(1) + go l.runAsyncHandler(cs) + for dl := time.Now().Add(5 * time.Second); l.detachQPending.Load() == 0; { + if time.Now().After(dl) { + t.Fatal("the dispatch goroutine parked without handing the conn back") + } + time.Sleep(time.Millisecond) + } + l.drainDetachQueue() + if !cs.dirty || cs.relinkPending { + t.Fatalf("the hand-back did not put the conn back on the dirty list (dirty=%v relinkPending=%v)", + cs.dirty, cs.relinkPending) } - release() - pass() + l.flushDirty() + l.asyncWG.Wait() // the close woke the parked goroutine, which exits + + hijackRaceExpectRead(t, rig.peer, "pending", "the queued response reached the peer") if l.conns[local] != nil || rig.disconnects.Load() != 1 { - t.Errorf("the %s pass did not close the conn once flushed: slot=%p hooks=%d", - site, l.conns[local], rig.disconnects.Load()) + t.Errorf("the deferred peer close was lost: slot=%p hooks=%d", l.conns[local], rig.disconnects.Load()) } - hijackRaceExpectRead(t, rig.peer, "pending", "the queued response reached the peer before the close") }) } } +// TestATransplantWaitsForARelink: between the pass giving a conn up and the +// loop draining the goroutine's hand-back, a queue entry names cs, and a +// transplant would return cs to the pool under it. The conn must not move +// until the hand-back is drained, and then it may. +func TestATransplantWaitsForARelink(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + l.async = true + cs.protocol = engine.HTTP1 + cs.detected = true + cs.lastActivity = time.Now().UnixNano() + l.markDirty(cs) + release := holdAsHandler(t, cs, true) + if !returnsWhileHeld(t, release, l.flushDirty) { + t.Fatal("the dirty pass waited on a running handler (celeris#669)") + } + release() + // The goroutine exits (a later close, say) after queuing its hand-back; + // the loop has not drained it yet. + cs.asyncInMu.Lock() + cs.relinkOwed = false + cs.asyncRun = false + cs.asyncInMu.Unlock() + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + + target := &countingTarget{} + t.Cleanup(target.closeAll) + l.transplant.Store(&transplantState{target: target}) + l.tryTransplant(local) + if n := target.count(); n != 0 { + t.Fatalf("a conn with a hand-back still queued was transplanted (%d adopted)", n) + } + l.drainDetachQueue() // the hand-back: back on the dirty list + l.flushDirty() // nothing queued: off it again + l.tryTransplant(local) + if n := target.count(); n != 1 { + t.Errorf("after the hand-back the conn was not transplanted (%d adopted): the refusal was not the relink's", n) + } +} + +// TestPostDetachHandlerIsWaitedOut: after Detach the dispatch goroutine may +// keep running — a handler that streams inline — but it never holds detachMu +// across ProcessH1 again, so whoever holds the lock is a guarded writeFn in +// one write. closeConn must wait for it as before, not leave the close to a +// goroutine that will not look until its handler returns, and the handler +// may be waiting for exactly that close's OnDetachClose. +func TestPostDetachHandlerIsWaitedOut(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + cs.asyncInMu.Lock() + cs.asyncDetachUnlocked = true + cs.asyncInMu.Unlock() + release := holdAsHandler(t, cs, true) + + done := make(chan struct{}) + go func() { + defer close(done) + l.closeConn(local) + }() + select { + case <-done: + t.Fatal("closeConn left the close to a post-Detach goroutine instead of waiting out a guarded write") + case <-time.After(200 * time.Millisecond): + } + release() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("closeConn never completed after the writer released detachMu") + } + if l.conns[local] != nil || rig.disconnects.Load() != 1 { + t.Errorf("close incomplete: slot=%p hooks=%d", l.conns[local], rig.disconnects.Load()) + } +} + // TestAnOwedCloseIsNeverTransplanted: between the dispatch goroutine's exit // and the loop draining its hand-back, the conn is still in the table with no // goroutine — the shape tryTransplant hands straight to the other engine. It diff --git a/engine/epoll/conn.go b/engine/epoll/conn.go index 40501588..ecb9f99d 100644 --- a/engine/epoll/conn.go +++ b/engine/epoll/conn.go @@ -204,20 +204,37 @@ type connState struct { // detached from the engine and handed to the caller as a net.Conn. In // async mode hijackConn runs ON the dispatch goroutine (inside // ProcessH1 → ErrHijacked), which still touches cs after ProcessH1 - // returns — so the pooled connState MUST NOT be released by hijackConn. - // The release is deferred to the worker via the asyncClosed + - // detachQueue → drainDetachQueue handoff, which sees this flag and runs - // releaseConnState only after the goroutine has exited. (#3.1) + // returns — so the connState MUST NOT be released by hijackConn (#3.1), + // and since celeris#668 it is never returned to the pool at all. // - // That hand-off also takes the conn out of l.liveConns and l.connCount, + // Off-thread, hijackConn also enqueues cs at once, and the loop's + // drainDetachQueue takes the conn out of l.liveConns and l.connCount, // which are the loop's and which the dispatch goroutine must not touch - // (celeris#668). Until it runs, the conn's entry stays in the live set - // with its descriptor already closed, so the live-set walkers skip an - // entry with this flag set. They read it without any lock the - // goroutine holds, hence atomic; hijackConn stores it before it + // (celeris#668). Until that runs, the conn's entry stays in the live set + // with its descriptor already closed, so the live-set walkers and the + // dirty pass skip a conn with this flag set. They read it without any + // lock the goroutine holds, hence atomic; hijackConn stores it before it // releases the descriptor number. hijacked atomic.Bool + // hijackSettled (loop-thread-only) records that drainDetachQueue has + // taken an off-thread-hijacked conn out of the loop's state. Such a + // connState is never returned to the pool — queue entries made before + // or after the hijack may still name it — so every later entry finds + // this set and does nothing (celeris#668). + hijackSettled bool + + // relinkOwed (guarded by asyncInMu) is set by the dirty pass or the + // EPOLLOUT resume when they give the conn up because its dispatch + // goroutine holds detachMu across a handler (celeris#669). The goroutine + // hands cs back through the detach queue at its next park, and + // drainDetachQueue puts it on the dirty list again. relinkPending + // (loop-thread-only) is the loop's side of the same debt: while it is + // set the conn is not offered to a transplant, which would return cs to + // the pool under the queue entry the hand-back makes. + relinkOwed bool + relinkPending bool + // closeOwed (guarded by asyncInMu) is set by closeConn when it finds // detachMu held by this conn's RUNNING dispatch goroutine — i.e. held // across a user handler — and leaves the close to that goroutine @@ -305,8 +322,11 @@ func releaseConnState(cs *connState) { cs.asyncDetachPending = false cs.liveIdx = -1 cs.hijacked.Store(false) + cs.hijackSettled = false cs.closeOwed = false cs.closeErr = nil + cs.relinkOwed = false + cs.relinkPending = false cs.fd = 0 connStatePool.Put(cs) } diff --git a/engine/epoll/hijack_closeconn_race_linux_test.go b/engine/epoll/hijack_closeconn_race_linux_test.go index ea399c63..77487107 100644 --- a/engine/epoll/hijack_closeconn_race_linux_test.go +++ b/engine/epoll/hijack_closeconn_race_linux_test.go @@ -324,9 +324,9 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) } // drainDetachQueue tests detachClosed BEFORE the hijacked branch, so a - // stray detachClosed strands the connState outside the pool. + // stray detachClosed would skip the hijack's settling. if cs.detachClosed { - t.Error("closeConn marked a hijacked conn detachClosed; the pool release is then skipped") + t.Error("closeConn marked a hijacked conn detachClosed; the hijack is then never settled") } // The hijacked conn belongs to its new owner and must keep working. @@ -337,7 +337,8 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) } // The dispatch goroutine exits and hands cs back via the detachQueue - // (runAsyncHandler's ErrHijacked path). The pool release must happen. + // (runAsyncHandler's ErrHijacked path); the loop drains that and the + // notice hijackConn enqueued, and settles the hijack once. cs.asyncInMu.Lock() cs.asyncRun = false cs.asyncInMu.Unlock() @@ -346,11 +347,8 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) l.detachQPending.Store(1) l.detachQMu.Unlock() l.drainDetachQueue() - if cs.hijacked.Load() { - t.Error("drainDetachQueue skipped the hijacked pool release (detachClosed short-circuit)") - } - if cs.fd != 0 { - t.Errorf("cs.fd = %d after the hand-off, want 0 (connState not released)", cs.fd) + if !cs.hijackSettled || cs.liveIdx != -1 { + t.Errorf("drainDetachQueue did not settle the hijack (settled=%v liveIdx=%d)", cs.hijackSettled, cs.liveIdx) } if got := l.connCount; got != 0 { t.Errorf("connCount = %d, want 0: a negative count never satisfies the DRAINING->SUSPENDED gate", got) @@ -503,8 +501,8 @@ func TestCloseConnAfterHijackIsNoOpWithoutTheRace(t *testing.T) { hijackRaceExpectRead(t, local, "y", "recycled pipe survives an uncontended closeConn") } } - if cs.fd != 0 { - t.Error("the hand-back did not release the hijacked connState") + if !cs.hijackSettled { + t.Error("the hand-back did not settle the hijack") } } @@ -594,8 +592,8 @@ func TestHijackReleaseUnlinksTheConnFromTheDirtyList(t *testing.T) { l.drainDetachQueue() if l.dirtyHead != nil { - t.Errorf("dirtyHead = %p after the hijacked conn was released to the pool, want nil: "+ - "the loop's dirty pass would flush pooled memory to a reissued fd", l.dirtyHead) + t.Errorf("dirtyHead = %p after the hijacked conn was settled, want nil: "+ + "the loop's dirty pass would flush its bytes to a reissued fd", l.dirtyHead) } }) @@ -682,8 +680,8 @@ func TestCloseLeftToAHandlerThatHijacksIsNotRedone(t *testing.T) { if len(l.liveConns) != 0 { t.Errorf("len(liveConns) = %d after the hand-back, want 0", len(l.liveConns)) } - if cs.fd != 0 { - t.Errorf("cs.fd = %d after the hand-back, want 0 (connState not released)", cs.fd) + if !cs.hijackSettled { + t.Error("the hand-back did not settle the hijack") } if !fdOpen(local) { t.Fatalf("fd %d, reissued after the hijack, was closed by the owed close", local) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index 9435307f..b101ed7c 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -191,8 +191,8 @@ func checkOffThreadHandBack(t *testing.T, l *Loop, cs *connState, bs []*connStat t.Errorf("connCount = %d after the hand-back, want %d", l.connCount, len(bs)) } assertLiveSet(t, l, bs...) - if cs.fd != 0 { - t.Errorf("cs.fd = %d after the hand-back, want 0 (connState not released)", cs.fd) + if !cs.hijackSettled { + t.Error("the hand-back did not settle the hijack") } } @@ -542,3 +542,93 @@ func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { t.Errorf("connCount = %d after the hand-back, want 1", l.connCount) } } + +// TestOffThreadHijackIsSettledBeforeItsGoroutineExits: a handler that hijacks +// may go on serving the hijacked conn for as long as it likes, on the +// dispatch goroutine. The loop must not wait for that goroutine to exit +// before taking the conn out of its live set and connCount — for a session +// served in-handler that would pin the DRAINING -> SUSPENDED gate for the +// whole session, and a handler that returns into a Detach-ed loop never +// exits at all. hijackConn's notice settles it at the next drain; the +// goroutine's own exit entry, later, is a no-op. +func TestOffThreadHijackIsSettledBeforeItsGoroutineExits(t *testing.T) { + rig, bs := offThreadRig(t, 2) + l, cs, local := rig.l, rig.cs, rig.local + cs.detachMu.Lock() + nc, err := l.hijackConn(local) + cs.detachMu.Unlock() + if err != nil { + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + + // The goroutine is still running (asyncRun): only the notice is queued. + l.drainDetachQueue() + if !cs.hijackSettled || cs.liveIdx != -1 || l.connCount != len(bs) { + t.Fatalf("the hijack was not settled while its goroutine still runs: settled=%v liveIdx=%d connCount=%d, "+ + "want true, -1, %d", cs.hijackSettled, cs.liveIdx, l.connCount, len(bs)) + } + assertLiveSet(t, l, bs...) + + // Its exit, later, hands cs back again: nothing more may happen. + handBack(l, cs) + if l.connCount != len(bs) { + t.Errorf("the goroutine's exit entry moved connCount to %d, want %d", l.connCount, len(bs)) + } + assertLiveSet(t, l, bs...) +} + +// TestDirtyPassSkipsAHijackedConn: a conn still on the dirty list — a +// pipelined response left partial — when its next request's handler hijacks. +// Once the handler returns and releases detachMu, the dirty pass can take the +// lock before the loop has drained the hijack's notice; the descriptor it +// would write to is closed, and its number already reissued. Nothing queued +// may reach the number's new owner. +func TestDirtyPassSkipsAHijackedConn(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + cs.writeBuf = append(cs.writeBuf[:0], "pending"...) + cs.pendingBytes = len(cs.writeBuf) + l.markDirty(cs) + + cs.detachMu.Lock() + nc, err := l.hijackConn(local) + if err != nil { + cs.detachMu.Unlock() + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + + // The number is reissued to the WRITE end of a pipe, so a stray write + // lands where the test can read it. + var p [2]int + if err := unix.Pipe2(p[:], unix.O_CLOEXEC|unix.O_NONBLOCK); err != nil { + cs.detachMu.Unlock() + t.Fatalf("pipe2: %v", err) + } + t.Cleanup(func() { _ = unix.Close(p[0]) }) + if _, err := unix.FcntlInt(uintptr(local), unix.F_GETFD, 0); err == nil { + cs.detachMu.Unlock() + t.Skipf("fd %d in use again before the test could take it", local) + } + if err := unix.Dup3(p[1], local, unix.O_CLOEXEC); err != nil { + cs.detachMu.Unlock() + t.Fatalf("dup3: %v", err) + } + _ = unix.Close(p[1]) + t.Cleanup(func() { _ = unix.Close(local) }) + + cs.detachMu.Unlock() // the handler returns + l.flushDirty() // before the loop drains the hijack's notice + + buf := make([]byte, 64) + if n, err := unix.Read(p[0], buf); n > 0 { + t.Fatalf("the dirty pass wrote %q to fd %d, which the hijack had released and another file now owns", + buf[:n], local) + } else if err != unix.EAGAIN { + t.Fatalf("read the pipe: %v", err) + } + if cs.dirty { + t.Error("the hijacked conn is still on the dirty list") + } +} diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index ca32dc7d..f760baeb 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -1503,18 +1503,17 @@ func (l *Loop) drainRead(fd int, now int64) { // queued, surface err to a detached middleware (OnError, under detachMu, as // every OnError site in this file runs), and close. // -// When detachMu is held and the conn's dispatch goroutine is running, the -// holder is that goroutine inside a user handler (celeris#669), and waiting -// for the lock parked the loop — and every connection on it — until the -// handler returned. The common shape is a client that gives up on a slow -// handler and disconnects. So the flush and the notification ride on the -// close instead: closeConn leaves it to the goroutine, which exits at its next -// check, and then runs it on this thread with the lock free, delivering -// closeErr first. +// When detachMu is held across a handler (dispatchBusy), waiting for the +// lock parked the loop — and every connection on it — until the handler +// returned (celeris#669); the common shape is a client that gives up on a +// slow handler and disconnects. So the flush and the notification ride on +// the close instead: closeConn leaves it to the dispatch goroutine, which +// exits at its next check, and then runs it on this thread with the lock +// free, delivering closeErr first. func (l *Loop) closeOnReadEnd(fd int, cs *connState, err error) { mu := cs.detachMu if mu != nil && !mu.TryLock() { - if dispatchBusy(cs) { + if dispatchBusy(cs, nil) { cs.closeErr = err l.closeConn(fd) return @@ -1531,52 +1530,45 @@ func (l *Loop) closeOnReadEnd(fd int, cs *connState, err error) { l.closeConn(fd) } -// dispatchBusy reports whether cs has a dispatch goroutine that is running: -// alive (asyncRun) and not parked in asyncCond.Wait. Loop thread. +// dispatchBusy reports whether cs's dispatch goroutine may be holding +// cs.detachMu across a user handler: it is alive (asyncRun), not parked in +// asyncCond.Wait (asyncParked), and has not released the lock for good at a +// Detach (asyncDetachUnlocked). All three are read under asyncInMu. Loop +// thread. // // It is how a loop-thread site that found cs.detachMu held tells the holders // apart (celeris#669). The dispatch goroutine holds the lock across -// ProcessH1, i.e. for as long as the user handler runs, and it is running +// ProcessH1, i.e. for as long as the handler runs, and it is running // whenever it does. Every other holder — a detached conn's guarded writeFn, // the goroutine's own asyncClosed re-check — holds it for one write or less. -// So a site that finds the lock held and the goroutine running must not wait, -// and one that finds it held with the goroutine parked or absent may wait as -// it always has: that wait is bounded. +// So a site that finds the lock held while this reports true must not wait, +// and one that finds it held while this reports false may wait as it always +// has: that wait is bounded. // -// A running goroutine is not necessarily the holder (it may be delivering a -// frame after Detach while a guarded writeFn holds the lock). Every site that -// acts on a true only leaves work to be done later — a flush the holder does -// anyway, a close the goroutine hands back on its way out — so a false -// positive costs a hand-off, never a lost write or close. +// After Detach the goroutine never takes the lock across ProcessH1 again, so +// it is excluded even while it runs — a handler may keep streaming inline +// after Detach, and a close left to it would wait for that handler, which in +// turn waits for the close's OnDetachClose to learn it should stop. // -// Parked is exact, not a heuristic: asyncParked is set and cleared under -// asyncInMu in the same critical section as the park loop's condition, so a -// goroutine seen parked here can only leave Wait by re-taking asyncInMu, after -// this read, and a caller that has already set asyncClosed makes it exit -// without taking detachMu. -func dispatchBusy(cs *connState) bool { +// If owe is non-nil and the result is true, *owe is set in the same critical +// section: the goroutine reads it under asyncInMu on its way to its next park +// or its exit, so it cannot miss it. Both owed hand-backs (closeOwed, +// relinkOwed) are recorded this way. +// +// A running goroutine is not necessarily the holder, and every caller acts on +// a true only by leaving work the goroutine hands back; a false positive +// costs a hand-back, never a lost write or close. Parked is exact: asyncParked +// is set and cleared under asyncInMu in the park loop's own critical section, +// so a goroutine seen parked can leave Wait only by re-taking asyncInMu after +// this read. +func dispatchBusy(cs *connState, owe *bool) bool { if cs.asyncCond.L == nil { return false // no async machinery: sync mode, no dispatch goroutine } cs.asyncInMu.Lock() - busy := cs.asyncRun && !cs.asyncParked - cs.asyncInMu.Unlock() - return busy -} - -// leaveCloseToDispatch is dispatchBusy for closeConn, which must record the -// hand-off in the same critical section it decides it in: closeOwed is read -// by the goroutine's exit path under asyncInMu, so a goroutine that exits -// after this section sees it, and one that exited before it made asyncRun -// false here. Loop thread; asyncClosed must already be set. -func leaveCloseToDispatch(cs *connState) bool { - if cs.asyncCond.L == nil { - return false - } - cs.asyncInMu.Lock() - busy := cs.asyncRun && !cs.asyncParked - if busy { - cs.closeOwed = true + busy := cs.asyncRun && !cs.asyncParked && !cs.asyncDetachUnlocked + if busy && owe != nil { + *owe = true } cs.asyncInMu.Unlock() return busy @@ -1623,13 +1615,14 @@ func (l *Loop) hijackConn(fd int) (net.Conn, error) { // them from here raced the index walks of checkTimeouts, the sweep and // shutdown, and connCount-- could lose an update against acceptAll's // connCount++, after which the connCount == 0 DRAINING→SUSPENDED gate - // never passes on this loop again. So off-thread both are left to - // drainDetachQueue's hijacked branch, which runs on the loop once this - // goroutine has exited and handed cs back — the hand-off that already - // defers the pool release (below). Until then the entry stays in the live - // set and the walkers skip it on hijacked, which is stored BEFORE the - // descriptor is released: by the time the kernel can reissue the number - // to another accept, the entry already reads as not the loop's. + // never passes on this loop again. So off-thread this enqueues cs for + // drainDetachQueue's hijacked branch, which does both on the loop at its + // next iteration — not when this goroutine exits, which for a handler + // that serves the hijacked conn itself is the end of that session. + // Until then the entry stays in the live set and the walkers skip it on + // hijacked, which is stored BEFORE the descriptor is released: by the + // time the kernel can reissue the number to another accept, the entry + // already reads as not the loop's. _ = unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_DEL, fd, nil) if offThread { cs.hijacked.Store(true) @@ -1649,21 +1642,24 @@ func (l *Loop) hijackConn(fd int) (net.Conn, error) { c, err := net.FileConn(f) _ = f.Close() - // CRITICAL (#3.1): off-thread, defer the pool release as well. When the + // CRITICAL (#3.1): off-thread, never release cs to the pool. When the // handler calls Hijack from inside the dispatch goroutine's ProcessH1, the // goroutine STILL touches cs after ProcessH1 returns ErrHijacked — it // Unlocks cs.detachMu, resyncs cs.pendingBytes, sets cs.asyncClosed, - // clears cs.asyncInBuf, and enqueues cs on detachQueue. Recycling cs now - // would hand pooled-and-reissued memory to that goroutine. The - // worker-thread teardown (drainDetachQueue, reached via the goroutine's - // asyncClosed + detachQueue handoff) runs the pool release once the - // goroutine has exited. + // clears cs.asyncInBuf, and enqueues cs on detachQueue — and detach-queue + // entries made before the hijack (a partial flush of a pipelined + // response) may still name it. Recycling cs would hand pooled-and-reissued + // memory to all of them, so it is left to the garbage collector, as + // closeConn leaves every conn a goroutine may still reference + // (celeris#668). The enqueue below is the loop's notice. // // Inline, drainRead returns immediately on ErrHijacked without touching // cs again and nothing will enqueue cs, so release synchronously here — // gating only on detachMu would leak the connState in the inline-async // case. - if !offThread { + if offThread { + l.enqueueDetach(cs) + } else { // Unlink before the pool release. releaseConnState clears cs's own // dirty links but never repairs l.dirtyHead or a predecessor's // dirtyNext, so handing back a still-linked connState leaves the @@ -1907,7 +1903,14 @@ func (l *Loop) initProtocol(cs *connState) { // nil-derefs WSRawWriteFn under a peer RST mid-upgrade. unlockDetachMu := l.async && cs.asyncPromoted && cs.detachMu != nil && !cs.asyncDetachUnlocked if unlockDetachMu { + // Under asyncInMu: dispatchBusy reads it there, and from here + // on this goroutine never holds detachMu across a handler + // again, so the loop must go back to waiting out the (bounded) + // holders of this conn's lock (celeris#669). detachMu is held + // here, and detachMu → asyncInMu is the order hijackConn uses. + cs.asyncInMu.Lock() cs.asyncDetachUnlocked = true + cs.asyncInMu.Unlock() } // Async mode: enqueue cs so drainDetachQueue picks up the // deferred bookkeeping (asyncDetachPending). The first @@ -2159,15 +2162,20 @@ func (l *Loop) runAsyncHandler(cs *connState) { // Signal worker via detachQueue + eventfd (never close the // fd from this goroutine — races with drainRead on the // worker's stale l.conns slot). - l.detachQMu.Lock() - l.detachQueue = append(l.detachQueue, cs) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) } }() for { cs.asyncInMu.Lock() + if cs.relinkOwed { + // The loop gave this conn up while our handler held detachMu + // (celeris#669: the dirty pass or the EPOLLOUT resume); hand it + // back now that the handler's writes are flushed as far as they + // go, so the loop re-examines it. asyncInMu → detachQMu: nothing + // takes asyncInMu under detachQMu. + cs.relinkOwed = false + l.enqueueDetach(cs) + } cs.asyncParked = true for len(cs.asyncInBuf) == 0 && !cs.asyncClosed.Load() && !cs.asyncQuiesce.Load() { // Park until the worker appends more bytes or the conn is @@ -2206,11 +2214,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { if cs.asyncQuiesce.Load() && len(cs.asyncInBuf) == 0 { cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() - l.detachQMu.Lock() - l.detachQueue = append(l.detachQueue, cs) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) return } // Double-buffer swap: hand asyncInBuf to the goroutine, reuse @@ -2289,11 +2293,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncInMu.Unlock() cs.asyncH2Promoted.Store(true) } - l.detachQMu.Lock() - l.detachQueue = append(l.detachQueue, cs) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) return } // celeris#273: a user handler may have called c.Detach() inside @@ -2313,11 +2313,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncInBuf = cs.asyncInBuf[:0] cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() - l.detachQMu.Lock() - l.detachQueue = append(l.detachQueue, cs) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) return } // Post-Detach: loop back to wait for more recv bytes (WS @@ -2367,11 +2363,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.detachMu.Unlock() if partial { - l.detachQMu.Lock() - l.detachQueue = append(l.detachQueue, cs) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) } if processErr != nil || flushErr != nil { @@ -2388,11 +2380,7 @@ func (l *Loop) runAsyncHandler(cs *connState) { cs.asyncInBuf = cs.asyncInBuf[:0] cs.endDispatch() // enqueued below: that is the hand-back cs.asyncInMu.Unlock() - l.detachQMu.Lock() - l.detachQueue = append(l.detachQueue, cs) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) return } } @@ -2407,6 +2395,10 @@ func (l *Loop) runAsyncHandler(cs *connState) { // lost and the conn stays open, owned by no goroutine. func (cs *connState) endDispatch() (closeOwed bool) { cs.asyncRun = false + // A relink owed at exit needs no hand-back of its own: the conn is being + // closed (asyncClosed), hijacked, handed to the other engine, or its exit + // enqueues cs anyway (the H2 upgrade), and each of those settles it. + cs.relinkOwed = false closeOwed = cs.closeOwed cs.closeOwed = false return closeOwed @@ -2450,27 +2442,26 @@ func (l *Loop) drainDetachQueue() { // Hijacked conn (#3.1): hijackConn already detached the fd from // epoll and the conn table and handed it to the caller as a // net.Conn (the original fd is closed; the caller owns a dup). It - // deferred to here, where the dispatch goroutine that referenced cs - // has now exited (it enqueued cs on its way out), the pool release - // and the two pieces of loop-thread state it may not touch from - // that goroutine: the live-set entry and connCount (celeris#668). - // Skip closeConn — there is no fd to close and l.conns[fd] is - // already nil. - // - // This branch runs once per hijack: releaseConnState clears the - // flag, so a second queue entry for the same connState cannot - // decrement connCount again. + // enqueued cs at once for what it may not do from the dispatch + // goroutine (celeris#668): take the conn out of the live set, + // connCount and the dirty list, and withdraw any transplant ask. + // That is done here, once; the goroutine's own exit and any entry + // made before the hijack find hijackSettled and do nothing. The + // connState is never pooled (see hijackConn), so none of those + // entries can name reissued memory. Skip closeConn — there is no fd + // to close and l.conns[fd] is already nil. if cs.hijacked.Load() { - l.removeLiveConn(cs) - l.connCount-- - // Unlink from the dirty list first: hijackConn does not, and - // releaseConnState clears only cs's own links, so the pool would - // otherwise get a connState that l.dirtyHead or a predecessor - // still points at (celeris#654). No-op unless the conn really was - // dirty — a partial write from a prior pipelined response. - l.removeDirty(cs) - l.dropAsk(cs) // celeris#657 P8: never pool a connState an ask still names - releaseConnState(cs) + if !cs.hijackSettled { + cs.hijackSettled = true + l.removeLiveConn(cs) + l.connCount-- + // Unlink from the dirty list: hijackConn does not, and a + // conn left on it would have the dirty pass write its + // queued bytes to a descriptor number that is no longer + // its own (celeris#654). + l.removeDirty(cs) + l.dropAsk(cs) // celeris#657 P8: no ask may name it once it is not ours + } continue } // Dispatch goroutine signaled close via asyncClosed. Only the @@ -2488,6 +2479,7 @@ func (l *Loop) drainDetachQueue() { if cs.asyncH2Promoted.Load() { cs.asyncH2Promoted.Store(false) l.h2Conns = append(l.h2Conns, cs.fd) + cs.relinkPending = false // celeris#669: back on the dirty list l.markDirty(cs) continue } @@ -2532,6 +2524,9 @@ func (l *Loop) drainDetachQueue() { }) cs.recvPaused = desired } + // A relink hand-back (celeris#669) lands here too: the conn is back + // on the dirty list, whose next pass sees it as it now is. + cs.relinkPending = false l.markDirty(cs) } // Drop the strong refs before reusing the array (see the io_uring @@ -2542,6 +2537,29 @@ func (l *Loop) drainDetachQueue() { l.detachQSpare = l.detachQSpare[:0] } +// relink gives up, for now, a conn whose flush the loop could not do because +// its dispatch goroutine holds detachMu across a handler (celeris#669). The +// caller has already set cs.relinkOwed through dispatchBusy; the goroutine +// hands cs back through the detach queue at its next park — after it has +// flushed what its handler wrote — and drainDetachQueue puts it on the dirty +// list, whose next pass sees the conn as it now is. Until then the conn is on +// neither the dirty list nor EPOLLOUT. +// +// Waiting for the hand-back, rather than for "a remainder", is what keeps the +// actions tied to a completed flush — a deferred peer close (peerClosed), the +// EPOLLOUT disarm — from being lost: a holder whose own flush completes has no +// remainder to hand back, and peerClosed may be set after this call. +// relinkPending keeps tryTransplant off the conn meanwhile: the hand-back is +// a queue entry naming cs, and a transplant returns cs to the pool. Loop +// thread. +func (l *Loop) relink(cs *connState) { + l.removeDirty(cs) + if cs.epollOut { + l.disarmEpollOut(cs) + } + cs.relinkPending = true +} + // flushDirty is the event loop's dirty-list pass, run once per iteration // after drainDetachQueue: flush every connection with bytes still queued, // close the ones whose flush failed, and hand a still-partial non-detached @@ -2549,34 +2567,34 @@ func (l *Loop) drainDetachQueue() { func (l *Loop) flushDirty() { for cs := l.dirtyHead; cs != nil; { next := cs.dirtyNext - if mu := cs.detachMu; mu != nil && !mu.TryLock() { - if dispatchBusy(cs) { + mu := cs.detachMu + if mu != nil && !mu.TryLock() { + if dispatchBusy(cs, &cs.relinkOwed) { // The conn's dispatch goroutine holds detachMu across a // user handler (celeris#669): waiting here parked the loop, - // and every connection on it, until the handler returned. - // Take the conn off the list instead of retrying it — a - // dirty list that never empties holds epoll_wait at 0 ms. - // Nothing is lost: whoever holds the lock flushes writeBuf - // from writePos when it is done (the goroutine after - // ProcessH1, a guarded writeFn after its write) and hands a - // remainder back through the detach queue, which puts the - // conn back on this list. - // - // Except a deferred peer close (peerClosed): it fires only - // when THIS pass sees the flush complete, and a holder - // whose flush completes hands nothing back. Such a conn - // stays on the list and is retried each pass until the - // handler returns — a spin, but only for a peer that - // half-closed with a response still queued while a - // pipelined request's handler runs. - if !cs.peerClosed { - l.removeDirty(cs) - } + // and every connection on it, until the handler returned, + // and retrying it each pass would hold epoll_wait at 0 ms + // — a spin — for as long. So give the conn up until the + // goroutine hands it back (relink): it does so at its next + // park, after it has flushed what its handler wrote. + l.relink(cs) cs = next continue } mu.Lock() } + if cs.hijacked.Load() { + // An async Hijack took the conn (celeris#668) and closed its + // descriptor, whose number may already be someone else's: + // writing the bytes still queued here would put them there. + // drainDetachQueue settles the rest. + if mu != nil { + mu.Unlock() + } + l.removeDirty(cs) + cs = next + continue + } err := l.flushWrites(cs, true) if err != nil { // Surface I/O failure to detached middleware before closing. @@ -2684,18 +2702,13 @@ func (l *Loop) disarmEpollOut(cs *connState) { // next writable edge resumes. Returns false if the conn was closed. func (l *Loop) handleWritable(cs *connState) { if mu := cs.detachMu; mu != nil && !mu.TryLock() { - if dispatchBusy(cs) { + if dispatchBusy(cs, &cs.relinkOwed) { // A pipelined request started a handler before the socket - // drained (celeris#669). Do not wait for it, and drop the - // level-triggered interest, which would otherwise fire on every - // epoll_wait until the handler returns. The goroutine flushes - // writeBuf itself when the handler returns and hands any - // remainder back to the dirty list, which re-arms EPOLLOUT. - // A deferred peer close keeps the interest, for the reason - // given in flushDirty. - if !cs.peerClosed { - l.disarmEpollOut(cs) - } + // drained (celeris#669). Do not wait for it, and do not keep + // the level-triggered interest either, which would fire on + // every epoll_wait until the handler returns: give the conn + // up until its goroutine hands it back (see relink). + l.relink(cs) return } mu.Lock() @@ -2994,7 +3007,7 @@ func (l *Loop) closeConn(fd int) { // gone, the holder is a guarded writeFn in one write, and waiting // for it is bounded; see dispatchBusy. if !cs.detachMu.TryLock() { - if leaveCloseToDispatch(cs) { + if dispatchBusy(cs, &cs.closeOwed) { return } cs.detachMu.Lock() @@ -3051,13 +3064,13 @@ func (l *Loop) closeConn(fd int) { // slot nil: all already done by hijackConn. // - removeLiveConn and connCount: not ours to do either. An // off-thread hijack leaves both to drainDetachQueue's - // hijacked branch, on this thread, once the dispatch - // goroutine has exited (celeris#668). + // hijacked branch, on this thread, when it drains the + // notice hijackConn enqueued (celeris#668). // - CloseH1 and the pool release: not ours to do. Leaving // detachClosed false is what lets drainDetachQueue reach its - // cs.hijacked branch and return the connState to the pool - // (detachClosed is tested first and would strand it); that - // branch also closes any sendfile dup via releaseConnState. + // cs.hijacked branch (detachClosed is tested first and + // would skip it); an off-thread-hijacked connState is + // never pooled. // - OnDisconnect: NOT fired. The conn did not disconnect, it // was handed to the application, and the sync hijack path // fires nothing either. Both directions are pinned by diff --git a/engine/epoll/review_v150_test.go b/engine/epoll/review_v150_test.go index e453ae34..96d7681e 100644 --- a/engine/epoll/review_v150_test.go +++ b/engine/epoll/review_v150_test.go @@ -328,15 +328,25 @@ func TestHijackDefersReleaseWhileAsyncGoroutineActive(t *testing.T) { l.drainDetachQueue() - // releaseConnState clears hijacked + zeroes fd. - if cs.hijacked.Load() { - t.Error("drainDetachQueue did not release hijacked cs (hijacked still set)") + // The hijack is settled on the loop — live set, connCount — and the + // connState is deliberately never pooled: entries made before or after + // the hijack may still name it (celeris#668). A further entry is a no-op. + if !cs.hijackSettled { + t.Error("drainDetachQueue did not settle the hijacked conn") } if len(l.liveConns) != 0 || l.connCount != 0 { t.Errorf("the hand-back left liveConns=%d connCount=%d, want 0 and 0", len(l.liveConns), l.connCount) } - if cs.fd != 0 { - t.Errorf("cs.fd = %d after release, want 0", cs.fd) + if cs.fd != local { + t.Errorf("cs.fd = %d after the hand-back, want %d: a hijacked connState must not be pooled", cs.fd, local) + } + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + l.drainDetachQueue() + if l.connCount != 0 { + t.Errorf("a second entry for the hijacked conn moved connCount to %d", l.connCount) } } diff --git a/engine/epoll/transplant.go b/engine/epoll/transplant.go index 9a8c939b..3be50b1c 100644 --- a/engine/epoll/transplant.go +++ b/engine/epoll/transplant.go @@ -214,7 +214,11 @@ func (l *Loop) flushedAtBoundary(cs *connState) bool { if !cs.h1State.AtRequestBoundary() { return false } - return !cs.dirty && !cs.epollOut && cs.writePos == 0 && cs.pendingBytes == 0 && + // relinkPending: the loop gave the conn up mid-handler and its dispatch + // goroutine owes it back (celeris#669). That hand-back is a queue entry + // naming cs, and a transplant returns cs to the pool; the conn is not at + // a boundary the loop has seen until the entry is drained. + return !cs.dirty && !cs.epollOut && !cs.relinkPending && cs.writePos == 0 && cs.pendingBytes == 0 && len(cs.writeBuf) == 0 && len(cs.bodyBuf) == 0 && cs.sendfile == nil } From 8e0e27bbf579ec4cc610b8c18d582ee593a0ae50 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 15:11:58 +0200 Subject: [PATCH 07/20] perf(epoll): skip the hijacked load for sync conns in the dirty pass (celeris#668) Only a conn with a detachMu is ever hijacked off-thread. --- engine/epoll/loop.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index f760baeb..c4d56bf3 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -2583,14 +2583,14 @@ func (l *Loop) flushDirty() { } mu.Lock() } - if cs.hijacked.Load() { + if mu != nil && cs.hijacked.Load() { // An async Hijack took the conn (celeris#668) and closed its // descriptor, whose number may already be someone else's: // writing the bytes still queued here would put them there. - // drainDetachQueue settles the rest. - if mu != nil { - mu.Unlock() - } + // drainDetachQueue settles the rest. (Only an async conn — + // one with a detachMu — is ever hijacked off-thread, so a + // sync conn skips the load.) + mu.Unlock() l.removeDirty(cs) cs = next continue From de7a2067a3ec344fa2084308efa046a2b45b49e0 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 15:12:57 +0200 Subject: [PATCH 08/20] test(epoll): two queue entries for a hijacked conn reach no pooled connState (celeris#668) --- engine/epoll/hijack_offthread_linux_test.go | 44 +++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index b101ed7c..9a6c93a1 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -632,3 +632,47 @@ func TestDirtyPassSkipsAHijackedConn(t *testing.T) { t.Error("the hijacked conn is still on the dirty list") } } + +// TestTwoQueueEntriesForAHijackedConnReachNoPooledConnState: a pipelined +// request whose predecessor's response is still in writeBuf when its handler +// hijacks makes runAsyncHandler enqueue cs twice on its way out — once for +// the partial flush, once for ErrHijacked. The first entry used to return cs +// to the pool, and the second then fell through every branch to markDirty on +// the pooled connState, leaving it on the loop's dirty list for the next +// connection that acquires it. +func TestTwoQueueEntriesForAHijackedConnReachNoPooledConnState(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + cs.writeBuf = append(cs.writeBuf[:0], "partial"...) + cs.pendingBytes = len(cs.writeBuf) + + cs.detachMu.Lock() + nc, err := l.hijackConn(local) + cs.detachMu.Unlock() + if err != nil { + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + + // runAsyncHandler's exit after ErrHijacked with writeBuf non-empty: the + // partial enqueue, then the processErr enqueue. + cs.asyncClosed.Store(true) + cs.asyncInMu.Lock() + cs.asyncRun = false + cs.asyncInMu.Unlock() + for range 2 { + l.detachQMu.Lock() + l.detachQueue = append(l.detachQueue, cs) + l.detachQPending.Store(1) + l.detachQMu.Unlock() + } + l.drainDetachQueue() + + if l.dirtyHead != nil || cs.dirty { + t.Fatalf("a queue entry for the hijacked conn put a connState on the dirty list after the first "+ + "entry had released it (dirtyHead=%p, cs.dirty=%v)", l.dirtyHead, cs.dirty) + } + if l.connCount != 0 { + t.Errorf("connCount = %d, want 0", l.connCount) + } +} From f11d5e1ab26a4c165f5821ac914d59d9b2c2c91e Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 15:31:23 +0200 Subject: [PATCH 09/20] test(epoll): take the released descriptor number for the pipe instead of skipping when pipe2 already took it (celeris#668) TestDirtyPassSkipsAHijackedConn skipped on every run: pipe2 takes the lowest free numbers, so it usually lands on the number the hijack just released, which the F_GETFD check read as 'in use'. A skip is absent, not a pass. --- engine/epoll/hijack_offthread_linux_test.go | 62 +++++++++++++++------ 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index 9a6c93a1..fda7a45c 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -600,29 +600,15 @@ func TestDirtyPassSkipsAHijackedConn(t *testing.T) { t.Cleanup(func() { _ = nc.Close() }) // The number is reissued to the WRITE end of a pipe, so a stray write - // lands where the test can read it. - var p [2]int - if err := unix.Pipe2(p[:], unix.O_CLOEXEC|unix.O_NONBLOCK); err != nil { - cs.detachMu.Unlock() - t.Fatalf("pipe2: %v", err) - } - t.Cleanup(func() { _ = unix.Close(p[0]) }) - if _, err := unix.FcntlInt(uintptr(local), unix.F_GETFD, 0); err == nil { - cs.detachMu.Unlock() - t.Skipf("fd %d in use again before the test could take it", local) - } - if err := unix.Dup3(p[1], local, unix.O_CLOEXEC); err != nil { - cs.detachMu.Unlock() - t.Fatalf("dup3: %v", err) - } - _ = unix.Close(p[1]) - t.Cleanup(func() { _ = unix.Close(local) }) + // lands where the test can read it. pipe2 takes the lowest free numbers, + // so it usually takes the released one itself. + rd := reissueToPipeWriteEnd(t, local) cs.detachMu.Unlock() // the handler returns l.flushDirty() // before the loop drains the hijack's notice buf := make([]byte, 64) - if n, err := unix.Read(p[0], buf); n > 0 { + if n, err := unix.Read(rd, buf); n > 0 { t.Fatalf("the dirty pass wrote %q to fd %d, which the hijack had released and another file now owns", buf[:n], local) } else if err != unix.EAGAIN { @@ -676,3 +662,43 @@ func TestTwoQueueEntriesForAHijackedConnReachNoPooledConnState(t *testing.T) { t.Errorf("connCount = %d, want 0", l.connCount) } } + +// reissueToPipeWriteEnd makes fd — a number the test's hijack just released — +// the write end of a new pipe, and returns the read end. pipe2 takes the +// lowest free numbers, so it normally lands on fd itself; either end may. +func reissueToPipeWriteEnd(t *testing.T, fd int) (readEnd int) { + t.Helper() + var p [2]int + if err := unix.Pipe2(p[:], unix.O_CLOEXEC|unix.O_NONBLOCK); err != nil { + t.Fatalf("pipe2: %v", err) + } + switch fd { + case p[1]: + // Already the write end. + case p[0]: + // The read end took it: keep a copy of the read end elsewhere, + // then put the write end on fd (dup3 closes the read end there). + rd, err := unix.FcntlInt(uintptr(p[0]), unix.F_DUPFD_CLOEXEC, 0) + if err != nil { + t.Fatalf("dup read end: %v", err) + } + if err := unix.Dup3(p[1], fd, unix.O_CLOEXEC); err != nil { + t.Fatalf("dup3 write end onto %d: %v", fd, err) + } + _ = unix.Close(p[1]) + p[0] = rd + default: + if _, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0); err == nil { + t.Fatalf("fd %d was reissued to something else before the test could take it", fd) + } + if err := unix.Dup3(p[1], fd, unix.O_CLOEXEC); err != nil { + t.Fatalf("dup3 write end onto %d: %v", fd, err) + } + _ = unix.Close(p[1]) + } + t.Cleanup(func() { + _ = unix.Close(p[0]) + _ = unix.Close(fd) + }) + return p[0] +} From e038a40827edf664705e6392a29af5eb28e6c693 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sat, 26 Sep 2026 15:34:10 +0200 Subject: [PATCH 10/20] test(epoll): pin the EPOLLRDHUP guard on a closing conn directly (celeris#669) The guard's only witness was the peer-close end-to-end test under -race, and its mutant survived a run of it: the unlocked read it prevents did not always meet the handler's write in the detector's history. The branch moves, unchanged, into Loop.onPeerHalfClose, and a unit test drives it with the close owed and the handler still writing: the 'pending' arm fails the mutant functionally, the 'race' arm under -race. --- .../epoll/async_handler_stall_linux_test.go | 44 +++++++++++++ engine/epoll/loop.go | 63 ++++++++++--------- 2 files changed, 79 insertions(+), 28 deletions(-) diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go index 616335cb..ffb7966e 100644 --- a/engine/epoll/async_handler_stall_linux_test.go +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -417,6 +417,50 @@ func TestAnOwedCloseIsNeverTransplanted(t *testing.T) { } } +// TestPeerHalfCloseLeavesAClosingConnToItsHandler: the FIN that drainRead's +// EOF branch turned into an owed close rides the same epoll event as +// EPOLLRDHUP, so onPeerHalfClose runs next, while the handler still holds +// detachMu and is writing its response. It must leave the conn alone: its +// csWritePending would read those buffers without the lock (a data race, +// the "race" arm under -race), and act on what it read (the "pending" arm). +func TestPeerHalfCloseLeavesAClosingConnToItsHandler(t *testing.T) { + for _, arm := range []string{"pending", "race"} { + t.Run(arm, func(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + release := holdAsHandler(t, cs, true) + if !returnsWhileHeld(t, release, l.checkTimeouts) { + t.Fatal("the reap waited on a running handler (celeris#669)") + } + if !cs.asyncClosed.Load() { + t.Fatal("setup: no close is owed") + } + if arm == "pending" { + // The handler has already queued part of its response. + cs.writeBuf = append(cs.writeBuf[:0], "response"...) + cs.pendingBytes = len(cs.writeBuf) + l.onPeerHalfClose(local) + } else { + done := make(chan struct{}) + go func() { + defer close(done) + l.onPeerHalfClose(local) + }() + // The handler writes its response; nothing orders this + // after the loop's look at the conn. + time.Sleep(20 * time.Millisecond) + cs.writeBuf = append(cs.writeBuf[:0], "response"...) + cs.pendingBytes = len(cs.writeBuf) + <-done + } + release() + if cs.peerClosed { + t.Error("onPeerHalfClose acted on a conn whose close is owed to its handler") + } + }) + } +} + // TestCloseStillWaitsForABoundedHolder is the negative control for the unit // arms. The dispatch goroutine is PARKED, so whoever holds detachMu is a // guarded writeFn in the middle of one write — a hold bounded by a syscall, diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index c4d56bf3..c2d7bc41 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -604,35 +604,9 @@ func (l *Loop) run(ctx context.Context) { } } - // EPOLLRDHUP: peer half-closed (sent FIN). drainRead's short-read - // fast path returns without a trailing EAGAIN read, so a FIN that - // rode the same readable edge as the request (client writes then - // immediately closes) leaves the EOF unread and — with EPOLLET — no - // further edge fires. Close here once the response has flushed; if a - // write is still pending (backpressure), defer via cs.peerClosed so - // the response is not truncated. Detached (WS/SSE) conns keep their - // middleware's close lifecycle, but they still have to LEARN the - // peer is gone — see notifyDetachedPeerClosed. - // - // A conn whose close is already under way (asyncClosed) is left - // alone. drainRead's EOF branch, on this same event, may have - // left the close to a dispatch goroutine that is still inside its - // handler (celeris#669); that goroutine is writing the response - // under detachMu, so csWritePending below would read its buffers - // unlocked, and closeConn would only repeat the hand-off. + // EPOLLRDHUP: peer half-closed (sent FIN). See onPeerHalfClose. if ev.Events&unix.EPOLLRDHUP != 0 { - if fd >= 0 && fd < len(l.conns) { - if cs := l.conns[fd]; cs != nil && !cs.detachClosed && !cs.asyncClosed.Load() { - switch { - case cs.h1State != nil && cs.h1State.Detached.Load(): - l.notifyDetachedPeerClosed(cs) - case csWritePending(cs): - cs.peerClosed = true - default: - l.closeConn(fd) - } - } - } + l.onPeerHalfClose(fd) } if ev.Events&(unix.EPOLLERR|unix.EPOLLHUP) != 0 { @@ -808,6 +782,39 @@ func (l *Loop) run(ctx context.Context) { } } +// onPeerHalfClose handles EPOLLRDHUP: the peer half-closed (sent FIN). +// drainRead's short-read fast path returns without a trailing EAGAIN read, +// so a FIN that rode the same readable edge as the request (client writes +// then immediately closes) leaves the EOF unread and — with EPOLLET — no +// further edge fires. Close here once the response has flushed; if a write +// is still pending (backpressure), defer via cs.peerClosed so the response is +// not truncated. Detached (WS/SSE) conns keep their middleware's close +// lifecycle, but they still have to LEARN the peer is gone — see +// notifyDetachedPeerClosed. Loop thread. +// +// A conn whose close is already under way (asyncClosed) is left alone. +// drainRead's EOF branch, on this same event, may have left the close to a +// dispatch goroutine that is still inside its handler (celeris#669); that +// goroutine is writing the response under detachMu, so csWritePending would +// read its buffers unlocked, and closeConn would only repeat the hand-off. +func (l *Loop) onPeerHalfClose(fd int) { + if fd < 0 || fd >= len(l.conns) { + return + } + cs := l.conns[fd] + if cs == nil || cs.detachClosed || cs.asyncClosed.Load() { + return + } + switch { + case cs.h1State != nil && cs.h1State.Detached.Load(): + l.notifyDetachedPeerClosed(cs) + case csWritePending(cs): + cs.peerClosed = true + default: + l.closeConn(fd) + } +} + // notifyDetachedPeerClosed reports an EPOLLRDHUP (peer half-close) to the // middleware that owns a detached WS/SSE connection. The engine does NOT close // here — after Detach the close lifecycle belongs to that middleware — it only From df2bcb76bedc15df2092a4f94d96c2e35adf4ff7 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:47:54 +0200 Subject: [PATCH 11/20] test(epoll): failing-first rig for a deferred transplant finished on an entry made before the quiesce (celeris#669) Review finding (round 1, major): once tryTransplant asks a parked dispatch goroutine to quiesce, drainDetachQueue's transplant branch finishes the hand-off on the FIRST entry naming the conn. An entry the goroutine made before its park (the remainder of a partial flush, or a relink hand-back) is one, so the connState was released while the goroutine was still waking to exit, and a later entry then acted on the released connState. Three arms: an earlier entry drained while the goroutine lives; the earlier entry and the exit in one batch; the review's interleaving, where the partial-flush entry clears relinkPending before the relink hand-back is queued. All three fail at e038a40. --- .../epoll/async_handler_stall_linux_test.go | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go index ffb7966e..9895eb6a 100644 --- a/engine/epoll/async_handler_stall_linux_test.go +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -349,6 +349,151 @@ func TestATransplantWaitsForARelink(t *testing.T) { } } +// TestADeferredTransplantFinishesOnlyAfterItsGoroutineExits: once tryTransplant +// has asked a parked dispatch goroutine to quiesce, EVERY detach-queue entry +// naming the conn reaches drainDetachQueue's transplant branch, not only the +// goroutine's exit. An entry the goroutine made before it parked is one: the +// remainder of its own partial flush, or a relink hand-back (celeris#669). +// Finishing the hand-off on such an entry released cs while the goroutine was +// still waking to exit, and a later entry then acted on the released +// connState. The hand-off must wait for the exit, happen exactly once, and +// leave nothing a later entry can act on. +func TestADeferredTransplantFinishesOnlyAfterItsGoroutineExits(t *testing.T) { + // deferTransplant runs setup, which leaves an entry for cs on the detach + // queue, then parks cs's dispatch goroutine (its state: parked with + // nothing buffered, the shape tryTransplant moves) and has tryTransplant + // ask it to quiesce. + deferTransplant := func(t *testing.T, setup func(*Loop, *connState)) (*hijackRaceRig, *countingTarget) { + t.Helper() + rig := hijackRaceConn(t) + l, cs := rig.l, rig.cs + l.async = true + cs.protocol = engine.HTTP1 + cs.detected = true + cs.lastActivity = time.Now().UnixNano() + setup(l, cs) + cs.asyncInMu.Lock() + cs.asyncRun, cs.asyncParked = true, true + cs.asyncInMu.Unlock() + target := &countingTarget{} + t.Cleanup(target.closeAll) + l.transplant.Store(&transplantState{target: target}) + l.tryTransplant(rig.local) + if !cs.transplantPending || l.detachQPending.Load() == 0 { + t.Fatalf("setup: no deferred transplant with an entry queued (transplantPending=%v queued=%d)", + cs.transplantPending, l.detachQPending.Load()) + } + return rig, target + } + // quiesceExit runs the REAL runAsyncHandler from the park: it sees the + // quiesce, exits and enqueues cs. Bounded, so a goroutine that parks again + // (a released connState has no quiesce set) fails the test, not hangs it. + quiesceExit := func(t *testing.T, l *Loop, cs *connState) { + t.Helper() + cs.asyncInMu.Lock() + cs.asyncParked = false + cs.asyncInMu.Unlock() + done := make(chan struct{}) + go func() { + defer close(done) + exitDispatch(l, cs) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + cs.asyncClosed.Store(true) + cs.asyncInMu.Lock() + cs.asyncCond.Broadcast() + cs.asyncInMu.Unlock() + <-done + t.Fatal("the dispatch goroutine parked again instead of exiting on its quiesce") + } + } + // handedOver asserts the end state once every entry has been drained. + handedOver := func(t *testing.T, rig *hijackRaceRig, target *countingTarget) { + t.Helper() + l, cs := rig.l, rig.cs + if n := target.count(); n != 1 { + t.Errorf("the conn was handed over %d times, want exactly once", n) + } + if cs.dirty || l.dirtyHead != nil { + t.Errorf("an entry drained after the hand-off put its connState on the dirty list "+ + "(dirty=%v head=%p): the pass would flush through a connState the loop let go", cs.dirty, l.dirtyHead) + } + // The loop cannot tell, at the finish, whether a later entry still + // names cs, so a released connState can be reissued under one. + if cs.detachMu == nil || cs.fd != rig.local { + t.Errorf("the hand-off returned its connState to the pool (detachMu=%p fd=%d) "+ + "while queue entries could still name it", cs.detachMu, cs.fd) + } + if l.transplantInFlight != 0 { + t.Errorf("transplantInFlight = %d after the hand-off, want 0", l.transplantInFlight) + } + } + // partialFlush is the entry the goroutine makes after its handler when its + // own flush is partial: enqueued once detachMu is released. + partialFlush := func(l *Loop, cs *connState) { l.enqueueDetach(cs) } + + t.Run("entry-drained-while-the-goroutine-lives", func(t *testing.T) { + rig, target := deferTransplant(t, partialFlush) + l, cs := rig.l, rig.cs + l.drainDetachQueue() + if n := target.count(); n != 0 || cs.detachMu == nil { + t.Fatalf("the hand-off finished on an entry made before the quiesce, with the goroutine "+ + "still alive (adopted=%d, connState released=%v): it wakes on a released connState", + n, cs.detachMu == nil) + } + quiesceExit(t, l, cs) + l.drainDetachQueue() + handedOver(t, rig, target) + }) + + t.Run("entry-and-exit-in-one-batch", func(t *testing.T) { + rig, target := deferTransplant(t, partialFlush) + // The goroutine exits before the loop swaps the queue: one batch + // holds the earlier entry, then the exit. + quiesceExit(t, rig.l, rig.cs) + rig.l.drainDetachQueue() + handedOver(t, rig, target) + }) + + // The review's interleaving: the partial-flush entry is drained before + // the goroutine reaches its loop top, which clears relinkPending while + // the relink hand-back is still to be queued. + t.Run("relink-hand-back-behind-a-partial-flush", func(t *testing.T) { + rig, target := deferTransplant(t, func(l *Loop, cs *connState) { + l.markDirty(cs) + release := holdAsHandler(t, cs, true) + if !returnsWhileHeld(t, release, l.flushDirty) { + t.Fatal("the dirty pass waited on a running handler (celeris#669)") + } + release() + partialFlush(l, cs) + l.drainDetachQueue() // back on the dirty list + l.flushDirty() // the remainder drains: off it again + // The goroutine's loop top (runAsyncHandler, under asyncInMu): + // the relink hand-back, then the park. + cs.asyncInMu.Lock() + owed := cs.relinkOwed + cs.relinkOwed = false + l.enqueueDetach(cs) + cs.asyncInMu.Unlock() + if !owed { + t.Fatal("setup: the dirty pass left no relink owed") + } + }) + l, cs := rig.l, rig.cs + l.drainDetachQueue() // the relink hand-back + if n := target.count(); n != 0 || cs.detachMu == nil { + t.Fatalf("the hand-off finished on the relink hand-back, with the goroutine still alive "+ + "(adopted=%d, connState released=%v)", n, cs.detachMu == nil) + } + quiesceExit(t, l, cs) + l.drainDetachQueue() + handedOver(t, rig, target) + }) +} + // TestPostDetachHandlerIsWaitedOut: after Detach the dispatch goroutine may // keep running — a handler that streams inline — but it never holds detachMu // across ProcessH1 again, so whoever holds the lock is a guarded writeFn in From ae354815cff7cd6d6740d74ef409d3c628ff4526 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 00:51:32 +0200 Subject: [PATCH 12/20] fix(epoll): finish a deferred transplant only after its dispatch goroutine exits, and never pool its connState (celeris#669) Once tryTransplant has asked a parked dispatch goroutine to quiesce, every detach-queue entry naming the conn reaches drainDetachQueue's transplant branch. An entry the goroutine made before its park (the remainder of its own partial flush, or a relink hand-back) finished the hand-off and pooled the connState while the goroutine was still waking to exit: it could then park again on a pooled connState (shutdown's asyncWG.Wait hangs), or a later entry marked a pooled connState dirty. The branch now does nothing while the goroutine lives (asyncRun, read under asyncInMu; the quiesce exit always enqueues), and finishTransplantHandoff no longer pools cs: the loop cannot tell whether a later entry still names it. transplanted makes every later entry a no-op. This makes relinkPending's early clear by a partial-flush entry harmless, so relinkPending now only keeps a conn the loop has not re-examined from being offered. Review nits in the same pass: armEpollOut registers EPOLLIN|EPOLLET|EPOLLOUT, so a conn's EPOLLOUT is edge-triggered and the comments that called it level-triggered are corrected (the driver's EPOLLOUT, registered without EPOLLET, is the level-triggered one); the suspend-gate note covers entries for a conn the loop has let go of; checkOffThreadHandBack's wording matches the notice-time settle; and hijackConn says why no sendfile dup can be left open off-thread. --- .../epoll/async_handler_stall_linux_test.go | 12 ++- engine/epoll/conn.go | 18 +++- engine/epoll/hijack_offthread_linux_test.go | 8 +- engine/epoll/loop.go | 95 ++++++++++++------- engine/epoll/transplant.go | 23 +++-- 5 files changed, 103 insertions(+), 53 deletions(-) diff --git a/engine/epoll/async_handler_stall_linux_test.go b/engine/epoll/async_handler_stall_linux_test.go index 9895eb6a..b793d644 100644 --- a/engine/epoll/async_handler_stall_linux_test.go +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -221,10 +221,11 @@ func TestDirtyPassDoesNotWaitForARunningAsyncHandler(t *testing.T) { } // TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler: a conn that hit write -// backpressure is on level-triggered EPOLLOUT; a pipelined request can start a -// handler before the socket drains. The resume must not park, and must drop -// the level-triggered interest, which would otherwise fire on every -// epoll_wait until the handler returns. +// backpressure is on EPOLLOUT; a pipelined request can start a handler before +// the socket drains. The resume must not park, and must drop the interest: +// EPOLLOUT is edge-triggered, this event spent its edge, and if the handler's +// own flush drains the socket no other comes. The goroutine's hand-back +// brings the conn back instead (TestAConnGivenUpMidHandlerIsHandedBack). func TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler(t *testing.T) { rig := hijackRaceConn(t) l, cs := rig.l, rig.cs @@ -241,7 +242,8 @@ func TestEPOLLOUTResumeDoesNotWaitForARunningAsyncHandler(t *testing.T) { stallWait) } if cs.epollOut { - t.Errorf("EPOLLOUT still armed: level-triggered, it fires on every epoll_wait until the handler returns") + t.Errorf("EPOLLOUT still armed after the resume gave the conn up: its edge is spent, and the " + + "goroutine's hand-back, not another edge, is what brings the conn back") } } diff --git a/engine/epoll/conn.go b/engine/epoll/conn.go index ecb9f99d..00244095 100644 --- a/engine/epoll/conn.go +++ b/engine/epoll/conn.go @@ -76,7 +76,7 @@ type connState struct { protocol engine.Protocol // 1 byte detected bool // 1 byte dirty bool // 1 byte: true when writeBuf has data to flush - epollOut bool // 1 byte: true while level-triggered EPOLLOUT is armed (write backpressure) + epollOut bool // 1 byte: true while EPOLLOUT is armed (write backpressure; edge-triggered, like EPOLLIN) _ [4]byte // padding to 8-byte alignment buf []byte // 24 bytes writeBuf []byte // 24 bytes: single append buffer for pending writes @@ -158,8 +158,14 @@ type connState struct { xferAsked atomic.Bool // transplantPending (#383, loop-thread-only) marks a conn detached for // transplant whose dispatch goroutine must drain+exit first; drainDetachQueue - // completes the handoff once it sees the enqueued cs with this set. + // completes the handoff at the first entry it drains after that exit. transplantPending bool + // transplanted (loop-thread-only) is set by finishTransplantHandoff once + // the fd is handed over. Such a connState is never returned to the pool — + // the goroutine's exit entry may still be queued behind the entry the + // hand-off finished on — so every later entry finds this set and does + // nothing (celeris#669). + transplanted bool // asyncPromoted: once an async-marked route is observed on this conn // while it ran inline on the event loop (per-handler async, celeris // #300), the conn is promoted (sticky) — every subsequent recv goes @@ -230,8 +236,11 @@ type connState struct { // hands cs back through the detach queue at its next park, and // drainDetachQueue puts it on the dirty list again. relinkPending // (loop-thread-only) is the loop's side of the same debt: while it is - // set the conn is not offered to a transplant, which would return cs to - // the pool under the queue entry the hand-back makes. + // set the conn is not offered to a transplant, because the loop has not + // yet seen it as the handler left it. Any entry that puts cs back on the + // dirty list clears it, the goroutine's own partial-flush entry included. + // A deferred hand-off does not rely on it for memory safety: that + // finishes only after the goroutine's exit and never pools cs. relinkOwed bool relinkPending bool @@ -317,6 +326,7 @@ func releaseConnState(cs *connState) { cs.asyncQuiesce.Store(false) cs.xferAsked.Store(false) cs.transplantPending = false + cs.transplanted = false cs.asyncPromoted = false cs.asyncDetachUnlocked = false cs.asyncDetachPending = false diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index fda7a45c..104bb944 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -171,8 +171,10 @@ func TestOffThreadHijackLeavesTheLiveSetToTheLoopDuringASweep(t *testing.T) { // checkOffThreadHandBack asserts the ownership rule on both sides of the // hand-back: the loop's live set and connCount still count the hijacked conn -// until its dispatch goroutine has exited, and drop it exactly once after. -// The public gauges move at the hijack itself. +// until the loop drains the notice hijackConn enqueued, and drop it exactly +// once then (the goroutine's exit is not waited for; see +// TestOffThreadHijackIsSettledBeforeItsGoroutineExits). The public gauges +// move at the hijack itself. func checkOffThreadHandBack(t *testing.T, l *Loop, cs *connState, bs []*connState) { t.Helper() if got := l.activeConns.Load(); got != 0 { @@ -184,7 +186,7 @@ func checkOffThreadHandBack(t *testing.T, l *Loop, cs *connState, bs []*connStat if cs.liveIdx < 0 || l.connCount != len(bs)+1 { t.Fatalf("celeris#668: hijackConn changed loop-thread-only state from the dispatch goroutine: "+ "liveIdx=%d connCount=%d, want the conn still in the live set and connCount %d until the "+ - "goroutine has exited", cs.liveIdx, l.connCount, len(bs)+1) + "loop drains the hijack's notice", cs.liveIdx, l.connCount, len(bs)+1) } handBack(l, cs) if l.connCount != len(bs) { diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index c2d7bc41..e7c6a726 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -594,8 +594,9 @@ func (l *Loop) run(ctx context.Context) { // EPOLLOUT: the socket became writable again for a conn that hit // write backpressure (armEpollOut). Flush the pending bytes and // disarm once drained. drainRead above may have closed the conn, - // so re-validate the slot. Level-triggered EPOLLOUT keeps firing - // while writable, so a partial flush simply resumes next edge. + // so re-validate the slot. EPOLLOUT is edge-triggered here (EPOLLET + // covers the whole mask), and a partial flush stops at EAGAIN, so + // the send buffer gaining room is the next edge that resumes it. if ev.Events&unix.EPOLLOUT != 0 { if fd >= 0 && fd < len(l.conns) { if cs := l.conns[fd]; cs != nil && cs.epollOut { @@ -754,10 +755,12 @@ func (l *Loop) run(ctx context.Context) { // forever, if none came (celeris#658). The flags are therefore // re-checked under wakeMu below, and AdoptConn kicks a parked loop // through wakeIfSuspended; see there for why the pair cannot lose a - // wakeup. (Every detach-queue enqueue still happens with the conn in - // the table, under transplantInFlight, or for a conn that is already - // closed, so the detach flag needs no kick — the re-check is for - // symmetry.) + // wakeup. (Every detach-queue enqueue that carries work happens with + // the conn still counted in connCount — an off-thread hijack's notice + // included — or under transplantInFlight. The rest name a conn the + // loop has already let go of: closed, hijack-settled or handed over, + // whose entry does nothing. So the detach flag needs no kick; the + // re-check is for symmetry.) if l.listenFD < 0 && l.connCount == 0 && l.acceptPaused.Load() && l.transplantInFlight == 0 && l.detachQPending.Load() == 0 && l.adoptQPending.Load() == 0 { @@ -1453,8 +1456,8 @@ func (l *Loop) drainRead(fd int, now int64) { l.disarmEpollOut(cs) } else { // Partial write — the kernel send buffer is full (write - // backpressure). Sync pendingBytes and arm level-triggered - // EPOLLOUT instead of busy-retrying via the dirty list, which + // backpressure). Sync pendingBytes and arm EPOLLOUT + // instead of busy-retrying via the dirty list, which // would spin epoll_wait(0)→write(EAGAIN) at 100% CPU. Truly- // detached WS/SSE conns keep the goroutine-driven dirty/ // detachQueue path (their writes flow through guarded writeFn @@ -1658,7 +1661,10 @@ func (l *Loop) hijackConn(fd int) (net.Conn, error) { // response) may still name it. Recycling cs would hand pooled-and-reissued // memory to all of them, so it is left to the garbage collector, as // closeConn leaves every conn a goroutine may still reference - // (celeris#668). The enqueue below is the loop's notice. + // (celeris#668). The enqueue below is the loop's notice. Not pooling + // leaves no sendfile dup open: only an async conn is hijacked off-thread, + // and initProtocol installs the sendfile hook in sync mode only, so + // cs.sendfile is always nil here. // // Inline, drainRead returns immediately on ErrHijacked without touching // cs again and nothing will enqueue cs, so release synchronously here — @@ -2430,19 +2436,39 @@ func (l *Loop) drainDetachQueue() { l.detachQPending.Store(0) l.detachQMu.Unlock() for _, cs := range l.detachQSpare { - // #383 transplant: the dispatch goroutine quiesced and exited; the fd was - // already removed from epoll by tryTransplant (at a flushed, clean - // boundary). Finish the hand-off to io_uring. Checked before EVERY - // other branch — including the already-closed guard below — so a - // quiescing conn is migrated, not dropped. The old ordering put - // detachClosed first, which would strand such a conn: no hand-off, - // no close, no hook, no counter, and the live gauge already + // #383 transplant: tryTransplant detached the fd from epoll (at a + // flushed, clean boundary) and asked the dispatch goroutine to + // quiesce. Finish the hand-off to io_uring once it has exited. + // Checked before EVERY other branch — including the already-closed + // guard below — so a quiescing conn is migrated, not dropped. The old + // ordering put detachClosed first, which would strand such a conn: no + // hand-off, no close, no hook, no counter, and the live gauge already // decremented (celeris#624). finishTransplantHandoff re-checks // detachClosed and counts the coincidence. + // + // Every entry naming cs reaches this branch, not only the + // goroutine's exit: one it made before it parked — the remainder of + // its own partial flush, or a relink hand-back (celeris#669) — may be + // drained after the quiesce was asked. Finishing on such an entry + // released cs while the goroutine was still waking to exit. So an + // entry drained while the goroutine lives does nothing (its exit, + // which always enqueues, finishes the hand-off), and the finish + // never pools cs (see finishTransplantHandoff): the loop cannot tell + // whether a later entry still names it. asyncRun is read under + // asyncInMu, which the exit clears it under. if cs.transplantPending { + cs.asyncInMu.Lock() + alive := cs.asyncRun + cs.asyncInMu.Unlock() + if alive { + continue + } l.finishTransplantHandoff(cs) continue } + if cs.transplanted { + continue // handed over already; this entry came after the finish + } if cs.detachClosed { continue } @@ -2556,9 +2582,10 @@ func (l *Loop) drainDetachQueue() { // actions tied to a completed flush — a deferred peer close (peerClosed), the // EPOLLOUT disarm — from being lost: a holder whose own flush completes has no // remainder to hand back, and peerClosed may be set after this call. -// relinkPending keeps tryTransplant off the conn meanwhile: the hand-back is -// a queue entry naming cs, and a transplant returns cs to the pool. Loop -// thread. +// relinkPending keeps tryTransplant off the conn until an entry has put it +// back on the dirty list, i.e. until the loop has seen it as the handler left +// it. (A queue entry naming a moved conn is drainDetachQueue's to make +// harmless, whichever entry clears relinkPending.) Loop thread. func (l *Loop) relink(cs *connState) { l.removeDirty(cs) if cs.epollOut { @@ -2570,7 +2597,7 @@ func (l *Loop) relink(cs *connState) { // flushDirty is the event loop's dirty-list pass, run once per iteration // after drainDetachQueue: flush every connection with bytes still queued, // close the ones whose flush failed, and hand a still-partial non-detached -// conn to level-triggered EPOLLOUT. Loop thread only. +// conn to EPOLLOUT. Loop thread only. func (l *Loop) flushDirty() { for cs := l.dirtyHead; cs != nil; { next := cs.dirtyNext @@ -2625,9 +2652,9 @@ func (l *Loop) flushDirty() { } } else { // Partial write: kernel send buffer full. Sync pendingBytes, - // then for a non-detached HTTP/H2 conn hand off to level- - // triggered EPOLLOUT (armEpollOut removes it from the dirty - // list) so the loop stops busy-retrying it. Truly-detached + // then for a non-detached HTTP/H2 conn hand off to EPOLLOUT + // (armEpollOut removes it from the dirty list) so the loop + // stops busy-retrying it. Truly-detached // WS/SSE conns stay on the dirty list — their writes are // goroutine-driven and re-signalled via the eventfd path. // `next` was captured above, so the removeDirty inside @@ -2663,13 +2690,15 @@ func (l *Loop) markDirty(cs *connState) { // (write(2) returned EAGAIN / a short count). Instead of re-flushing into a // guaranteed EAGAIN on every iteration — which made adaptiveTimeoutMs spin // at epoll_wait(0) → write(EAGAIN) at 100% CPU under backpressure — we add -// a level-triggered EPOLLOUT to the conn's interest set and DROP it from the +// EPOLLOUT to the conn's interest set and DROP it from the // dirty list. The worker then blocks in epoll_wait until the socket is // writable again, at which point handleWritable flushes and disarms. // -// EPOLLIN stays edge-triggered (EPOLLET applies only to EPOLLIN); EPOLLOUT -// is level-triggered so it keeps firing while the socket is writable and -// there is no missed-wakeup risk. Mirrors driver.go's flushDriverSendLocked. +// EPOLLET covers the whole event mask, so EPOLLOUT is edge-triggered here, +// like EPOLLIN (driver.go's flushDriverSendLocked registers no EPOLLET, so a +// driver conn's EPOLLOUT is level-triggered). No wakeup is lost: the MOD +// reports EPOLLOUT at once if the socket is already writable, and every flush +// stops at EAGAIN, after which the send buffer gaining room is a new edge. func (l *Loop) armEpollOut(cs *connState) { // Backpressure replaces the dirty-list retry; the two must not coexist // or adaptiveTimeoutMs would still return 0 and busy-poll. @@ -2689,7 +2718,7 @@ func (l *Loop) armEpollOut(cs *connState) { } } -// disarmEpollOut removes the level-triggered EPOLLOUT interest once a conn's +// disarmEpollOut removes the EPOLLOUT interest once a conn's // pending writes have fully drained, restoring the read-only edge-triggered // interest so an idle fd doesn't wake the loop on every writable signal. func (l *Loop) disarmEpollOut(cs *connState) { @@ -2711,10 +2740,12 @@ func (l *Loop) handleWritable(cs *connState) { if mu := cs.detachMu; mu != nil && !mu.TryLock() { if dispatchBusy(cs, &cs.relinkOwed) { // A pipelined request started a handler before the socket - // drained (celeris#669). Do not wait for it, and do not keep - // the level-triggered interest either, which would fire on - // every epoll_wait until the handler returns: give the conn - // up until its goroutine hands it back (see relink). + // drained (celeris#669). Do not wait for it, and do not leave + // the conn to EPOLLOUT's next edge either: this event spent the + // last one, and if the handler's own flush drains the socket no + // other may come, stranding the disarm and a deferred peer + // close. Give the conn up until its goroutine hands it back + // (see relink). l.relink(cs) return } diff --git a/engine/epoll/transplant.go b/engine/epoll/transplant.go index 3be50b1c..a06b1f1d 100644 --- a/engine/epoll/transplant.go +++ b/engine/epoll/transplant.go @@ -215,9 +215,8 @@ func (l *Loop) flushedAtBoundary(cs *connState) bool { return false } // relinkPending: the loop gave the conn up mid-handler and its dispatch - // goroutine owes it back (celeris#669). That hand-back is a queue entry - // naming cs, and a transplant returns cs to the pool; the conn is not at - // a boundary the loop has seen until the entry is drained. + // goroutine owes it back (celeris#669); the conn is not at a boundary the + // loop has seen until an entry puts it back on the dirty list. return !cs.dirty && !cs.epollOut && !cs.relinkPending && cs.writePos == 0 && cs.pendingBytes == 0 && len(cs.writeBuf) == 0 && len(cs.bodyBuf) == 0 && cs.sendfile == nil } @@ -259,10 +258,10 @@ func (l *Loop) detachForTransplant(fd int, cs *connState) { } // finishTransplantHandoff completes a deferred async transplant after the -// dispatch goroutine has exited and enqueued cs (#383). The fd was already -// removed from epoll by tryTransplant; here we capture the carry-over, release -// the connState, and hand the fd to the io_uring target. Runs on the loop thread -// (from drainDetachQueue). +// dispatch goroutine has exited (#383). The fd was already removed from epoll +// by tryTransplant; here we capture the carry-over, mark the connState handed +// over (it is never pooled), and hand the fd to the io_uring target. Runs on +// the loop thread (from drainDetachQueue). func (l *Loop) finishTransplantHandoff(cs *connState) { cs.transplantPending = false // The debt tracked for the standby suspend gate is settled here, on @@ -286,8 +285,14 @@ func (l *Loop) finishTransplantHandoff(cs *connState) { } fd := cs.fd carry := engine.Carryover{RemoteAddr: cs.remoteAddr} - l.dropAsk(cs) // celeris#657 P8: never pool a connState an ask still names - releaseConnState(cs) + l.dropAsk(cs) // celeris#657 P8: no ask may name it once it is not ours + // Never pooled (celeris#669): drainDetachQueue may finish on an entry + // the goroutine made before its exit, with the exit's own entry still to + // come, and cannot tell. A released connState would be reissued under + // that entry. transplanted makes every later entry a no-op, and the + // garbage collector takes cs once the queue lets go of it. The cost is + // one allocation per connection moved, i.e. per engine switch. + cs.transplanted = true ts := l.transplant.Load() if ts == nil { // The drain was stopped between the detach and here (the adaptive From cbf529fe8751fafafbd222f1153bb774a2fbecfe Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 15:29:48 +0200 Subject: [PATCH 13/20] test(epoll): pass deferAccept to createListenSocket in the off-thread hijack accept test #674 (49d2726) added a deferAccept parameter to createListenSocket and updated every caller then on main, passing true to keep the old behaviour (TCP_DEFER_ACCEPT was always set). This PR's TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack was written against the old signature, so after merging main the engine/epoll test package no longer compiled (GOOS=linux go vet: "not enough arguments in call to createListenSocket"). Pass true, as #674 did for its siblings. Checked: GOOS=linux go build ./... and go vet ./... on amd64 and arm64, rc=0 (both failed on the vet before this change). --- engine/epoll/hijack_offthread_linux_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index 104bb944..556a9002 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -441,7 +441,7 @@ func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { t.Fatalf("epoll_create1: %v", err) } t.Cleanup(func() { _ = unix.Close(epfd) }) - lfd, err := createListenSocket("127.0.0.1:0") + lfd, err := createListenSocket("127.0.0.1:0", true) if err != nil { t.Fatalf("listen socket: %v", err) } From c9b9b16d0e87c7c24858516c5416710f724b0787 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:09:15 +0200 Subject: [PATCH 14/20] test(epoll): the EPOLLOUT resume must neither flush nor re-arm a conn an async Hijack took (review of #698) --- engine/epoll/hijack_offthread_linux_test.go | 107 ++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index 556a9002..db24fe34 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -9,6 +9,9 @@ import ( "io" "net" "net/http" + "os" + "strconv" + "strings" "sync" "sync/atomic" "testing" @@ -621,6 +624,110 @@ func TestDirtyPassSkipsAHijackedConn(t *testing.T) { } } +// TestEPOLLOUTResumeSkipsAHijackedConn is TestDirtyPassSkipsAHijackedConn +// for the other site that flushes a backpressured conn: the EPOLLOUT resume +// (review of #698). The loop reads the conn from its slot for an EPOLLOUT +// event, and the conn's handler, which holds detachMu inside ProcessH1, +// hijacks it before handleWritable runs: the slot is cleared and the +// descriptor closed, and its number can be reissued at once. +// +// - handler returned: the handler has released detachMu, so the resume +// takes it. Flushing would write the conn's queued bytes to the number's +// new owner. +// - handler still running: the handler still holds detachMu, so the resume +// gives the conn up (relink), which disarms EPOLLOUT: an EPOLL_CTL_MOD by +// the number, in this loop's epoll set. The new owner here is a pipe's +// write end that a driver goroutine registered in that set (RegisterConn +// adds from the caller's goroutine), level-triggered for EPOLLOUT. The +// MOD would make it edge-triggered and drop its EPOLLOUT. +// +// Both call handleWritable directly with the conn, as the run loop does with +// the one it read from the slot. +func TestEPOLLOUTResumeSkipsAHijackedConn(t *testing.T) { + for _, handlerReturned := range []bool{true, false} { + name := "handler still running" + if handlerReturned { + name = "handler returned" + } + t.Run(name, func(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + cs.writeBuf = append(cs.writeBuf[:0], "pending"...) + cs.pendingBytes = len(cs.writeBuf) + l.armEpollOut(cs) + if !cs.epollOut { + t.Fatal("apparatus: EPOLLOUT could not be armed on the conn") + } + + cs.detachMu.Lock() + nc, err := l.hijackConn(local) + if err != nil { + cs.detachMu.Unlock() + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + rd := reissueToPipeWriteEnd(t, local) + if err := unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_ADD, local, &unix.EpollEvent{ + Events: unix.EPOLLOUT, + Fd: int32(local), + }); err != nil { + cs.detachMu.Unlock() + t.Fatalf("register the new owner of fd %d in the loop's epoll set: %v", local, err) + } + // As the kernel holds it: it adds EPOLLERR and EPOLLHUP. + driverEvents, ok := epollEventsOf(t, l.epollFD, local) + if !ok { + cs.detachMu.Unlock() + t.Fatalf("apparatus: fd %d is not in the loop's epoll set after the ADD", local) + } + + if handlerReturned { + cs.detachMu.Unlock() + l.handleWritable(cs) + } else { + l.handleWritable(cs) + cs.detachMu.Unlock() + } + + buf := make([]byte, 64) + if n, err := unix.Read(rd, buf); n > 0 { + t.Errorf("the EPOLLOUT resume wrote %q to fd %d, which the hijack had released and another file now owns", + buf[:n], local) + } else if err != unix.EAGAIN { + t.Errorf("read the pipe: %v", err) + } + if got, ok := epollEventsOf(t, l.epollFD, local); !ok { + t.Errorf("fd %d, the new owner, is no longer in the loop's epoll set", local) + } else if got != driverEvents { + t.Errorf("the EPOLLOUT resume changed the events of fd %d, which another file now owns, "+ + "from %#x to %#x", local, driverEvents, got) + } + }) + } +} + +// epollEventsOf reads the event mask epfd holds for fd from +// /proc/self/fdinfo, and whether epfd holds fd at all. +func epollEventsOf(t *testing.T, epfd, fd int) (uint32, bool) { + t.Helper() + b, err := os.ReadFile(fmt.Sprintf("/proc/self/fdinfo/%d", epfd)) + if err != nil { + t.Fatalf("read fdinfo of the epoll fd: %v", err) + } + for _, line := range strings.Split(string(b), "\n") { + f := strings.Fields(line) + if len(f) < 4 || f[0] != "tfd:" || f[2] != "events:" || f[1] != strconv.Itoa(fd) { + continue + } + ev, err := strconv.ParseUint(f[3], 16, 32) + if err != nil { + t.Fatalf("parse %q: %v", line, err) + } + return uint32(ev), true + } + return 0, false +} + // TestTwoQueueEntriesForAHijackedConnReachNoPooledConnState: a pipelined // request whose predecessor's response is still in writeBuf when its handler // hijacks makes runAsyncHandler enqueue cs twice on its way out — once for From f74b783a4cac5d1206c650bb45fa69cabbc3318f Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:10:27 +0200 Subject: [PATCH 15/20] fix(epoll): the EPOLLOUT resume skips a conn an async Hijack took, and a relink never disarms EPOLLOUT by a released number (review of #698) --- engine/epoll/loop.go | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index 2e13616d..657ebf1b 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -2680,7 +2680,18 @@ func (l *Loop) drainDetachQueue() { func (l *Loop) relink(cs *connState) { l.removeDirty(cs) if cs.epollOut { - l.disarmEpollOut(cs) + // The goroutine holding detachMu may be a handler that hijacks the + // conn (celeris#668). Its hijack stores hijacked, clears the slot + // under driverMu, and only then closes the descriptor, whose number + // another file, one a driver goroutine adds to this epoll set, can + // take at once. Read under driverMu, hijacked false means the + // descriptor is not closed yet, so the MOD cannot reach that file. + // A hijacked conn is out of the epoll set already. + l.driverMu.Lock() + if !cs.hijacked.Load() { + l.disarmEpollOut(cs) + } + l.driverMu.Unlock() } cs.relinkPending = true } @@ -2828,7 +2839,8 @@ func (l *Loop) disarmEpollOut(cs *connState) { // read-only interest. A still-partial flush leaves EPOLLOUT armed so the // next writable edge resumes. Returns false if the conn was closed. func (l *Loop) handleWritable(cs *connState) { - if mu := cs.detachMu; mu != nil && !mu.TryLock() { + mu := cs.detachMu + if mu != nil && !mu.TryLock() { if dispatchBusy(cs, &cs.relinkOwed) { // A pipelined request started a handler before the socket // drained (celeris#669). Do not wait for it, and do not leave @@ -2842,6 +2854,15 @@ func (l *Loop) handleWritable(cs *connState) { } mu.Lock() } + if mu != nil && cs.hijacked.Load() { + // The run loop read cs from its slot, and then the conn's handler + // hijacked it (celeris#668) and returned: the descriptor is closed + // and its number may already be another file's. Writing the queued + // bytes, or disarming EPOLLOUT by the number, would reach that file, + // as in flushDirty. drainDetachQueue settles the rest. + mu.Unlock() + return + } err := l.flushWrites(cs, true) drained := err == nil && !csWritePending(cs) if err == nil { @@ -2854,7 +2875,7 @@ func (l *Loop) handleWritable(cs *connState) { if err != nil && cs.h1State != nil && cs.h1State.OnError != nil { cs.h1State.OnError(err) } - if mu := cs.detachMu; mu != nil { + if mu != nil { mu.Unlock() } if err != nil { From 474beb66547a1126b5134cb86af6a4d176973d66 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:10:27 +0200 Subject: [PATCH 16/20] test(epoll): make the accept-after-hijack test's number reuse happen instead of skipping without it (review of #698) --- engine/epoll/hijack_offthread_linux_test.go | 52 ++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index db24fe34..ceedbecc 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -438,7 +438,54 @@ func TestAsyncHijackUnderAcceptChurnLeavesTheLoopsSuspendable(t *testing.T) { // The loop side below learns that the number is free from the kernel alone // (F_GETFD), exactly as accept4 does, so under -race the only thing that can // order the two slot writes is the lock. +// +// The reuse is the premise, so it is made to happen rather than hoped for +// (review of #698: the test used to skip without it, and CI counts no skip +// in this package). Every free number below the conn's is filled before the +// hijack, so the duplicate the hijack takes lands above it, and the number +// it releases is the lowest free one when accept4 runs. Something else in +// the process can still take it first; that attempt is repeated, and a test +// that never saw the reuse fails. func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { + const attempts = 3 + for i := 1; i <= attempts; i++ { + if acceptAfterHijackAttempt(t) { + return + } + t.Logf("attempt %d: the new conn did not get the released number, trying again", i) + } + t.Fatalf("the kernel gave the released number to the next accept in none of %d attempts: the reuse under "+ + "test never happened", attempts) +} + +// fillHolesBelow opens /dev/null until the number it gets is above fd, so +// every number below fd is taken and the next one the process allocates is +// above it. The fillers stay open until the test ends. +func fillHolesBelow(t *testing.T, fd int) { + t.Helper() + var fillers []int + t.Cleanup(func() { + for _, f := range fillers { + _ = unix.Close(f) + } + }) + for { + f, err := unix.Open("/dev/null", unix.O_RDONLY|unix.O_CLOEXEC, 0) + if err != nil { + t.Fatalf("open /dev/null: %v", err) + } + if f > fd { + _ = unix.Close(f) + return + } + fillers = append(fillers, f) + } +} + +// acceptAfterHijackAttempt runs the test once, and reports whether the new +// conn got the number the hijack released; the assertions run only then. +func acceptAfterHijackAttempt(t *testing.T) bool { + t.Helper() epfd, err := unix.EpollCreate1(unix.EPOLL_CLOEXEC) if err != nil { t.Fatalf("epoll_create1: %v", err) @@ -496,6 +543,7 @@ func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { cs.asyncRun = true // the conn's dispatch goroutine is alive: the hijack is off-thread cs.asyncInMu.Unlock() dial() // queued before the hijack frees fd, so the next accept4 takes fd + fillHolesBelow(t, fd) hijacked := make(chan net.Conn, 1) go func() { // the dispatch goroutine, inside the handler @@ -536,7 +584,8 @@ func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { } next := l.liveConns[1] if next.fd != fd { - t.Skipf("the kernel gave the new conn fd %d, not the released %d; the reuse under test did not happen", next.fd, fd) + t.Logf("the kernel gave the new conn fd %d, not the released %d", next.fd, fd) + return false } if l.conns[fd] != next { t.Fatalf("slot %d holds %p, want the new conn %p", fd, l.conns[fd], next) @@ -546,6 +595,7 @@ func TestAcceptOfANumberAHijackReleasedIsOrderedAfterTheHijack(t *testing.T) { if l.connCount != 1 { t.Errorf("connCount = %d after the hand-back, want 1", l.connCount) } + return true } // TestOffThreadHijackIsSettledBeforeItsGoroutineExits: a handler that hijacks From 29f100e7bc6e4715b4b24e111081ac1c6c2268a3 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:17:26 +0200 Subject: [PATCH 17/20] test(epoll): arming or disarming EPOLLOUT must not reach the number an async Hijack released (review of #698) --- engine/epoll/hijack_offthread_linux_test.go | 46 +++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/engine/epoll/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go index ceedbecc..f6889993 100644 --- a/engine/epoll/hijack_offthread_linux_test.go +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -756,6 +756,52 @@ func TestEPOLLOUTResumeSkipsAHijackedConn(t *testing.T) { } } +// TestEPOLLOUTArmAndDisarmLeaveAReleasedNumberAlone: the loop arms and +// disarms EPOLLOUT by the conn's number, and every site that does so for an +// async conn does it after releasing detachMu (drainRead, the dirty pass, the +// EPOLLOUT resume). The conn's handler can take detachMu in between and +// hijack the conn (celeris#668), releasing the number, which another file in +// this loop's epoll set, one a driver goroutine registered, can hold by the +// time the MOD runs. Neither MOD may reach it (review of #698). +func TestEPOLLOUTArmAndDisarmLeaveAReleasedNumberAlone(t *testing.T) { + rig := hijackRaceConn(t) + l, cs, local := rig.l, rig.cs, rig.local + cs.detachMu.Lock() + nc, err := l.hijackConn(local) + cs.detachMu.Unlock() + if err != nil { + t.Fatalf("hijackConn: %v", err) + } + t.Cleanup(func() { _ = nc.Close() }) + reissueToPipeWriteEnd(t, local) + if err := unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_ADD, local, &unix.EpollEvent{ + Events: unix.EPOLLOUT, + Fd: int32(local), + }); err != nil { + t.Fatalf("register the new owner of fd %d in the loop's epoll set: %v", local, err) + } + driverEvents, ok := epollEventsOf(t, l.epollFD, local) + if !ok { + t.Fatalf("apparatus: fd %d is not in the loop's epoll set after the ADD", local) + } + + l.armEpollOut(cs) + if got, _ := epollEventsOf(t, l.epollFD, local); got != driverEvents { + t.Errorf("armEpollOut on the hijacked conn changed the events of fd %d, which another file now owns, "+ + "from %#x to %#x", local, driverEvents, got) + } + if cs.dirty { + t.Error("armEpollOut put the hijacked conn on the dirty list") + } + + cs.epollOut = true + l.disarmEpollOut(cs) + if got, _ := epollEventsOf(t, l.epollFD, local); got != driverEvents { + t.Errorf("disarmEpollOut on the hijacked conn changed the events of fd %d, which another file now owns, "+ + "from %#x to %#x", local, driverEvents, got) + } +} + // epollEventsOf reads the event mask epfd holds for fd from // /proc/self/fdinfo, and whether epfd holds fd at all. func epollEventsOf(t *testing.T, epfd, fd int) (uint32, bool) { From 60fd47b0958442c53ef869a1d5abd153ca502f6d Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:17:55 +0200 Subject: [PATCH 18/20] fix(epoll): never arm or disarm EPOLLOUT by the number of a conn an async Hijack took (review of #698) --- engine/epoll/loop.go | 51 +++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index 657ebf1b..f565b8c6 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -2680,18 +2680,7 @@ func (l *Loop) drainDetachQueue() { func (l *Loop) relink(cs *connState) { l.removeDirty(cs) if cs.epollOut { - // The goroutine holding detachMu may be a handler that hijacks the - // conn (celeris#668). Its hijack stores hijacked, clears the slot - // under driverMu, and only then closes the descriptor, whose number - // another file, one a driver goroutine adds to this epoll set, can - // take at once. Read under driverMu, hijacked false means the - // descriptor is not closed yet, so the MOD cannot reach that file. - // A hijacked conn is out of the epoll set already. - l.driverMu.Lock() - if !cs.hijacked.Load() { - l.disarmEpollOut(cs) - } - l.driverMu.Unlock() + l.disarmEpollOut(cs) } cs.relinkPending = true } @@ -2808,10 +2797,11 @@ func (l *Loop) armEpollOut(cs *connState) { if cs.epollOut { return } - if err := unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &unix.EpollEvent{ - Events: unix.EPOLLIN | unix.EPOLLET | unix.EPOLLOUT | unix.EPOLLRDHUP, - Fd: int32(cs.fd), - }); err == nil { + issued, err := l.modEpollOut(cs, unix.EPOLLIN|unix.EPOLLET|unix.EPOLLOUT|unix.EPOLLRDHUP) + if !issued { + return // hijacked: drainDetachQueue settles the conn + } + if err == nil { cs.epollOut = true } else { // MOD failed (should not happen for a registered fd); fall back to @@ -2827,11 +2817,34 @@ func (l *Loop) disarmEpollOut(cs *connState) { if !cs.epollOut { return } - _ = unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &unix.EpollEvent{ - Events: unix.EPOLLIN | unix.EPOLLET | unix.EPOLLRDHUP, + _, _ = l.modEpollOut(cs, unix.EPOLLIN|unix.EPOLLET|unix.EPOLLRDHUP) + cs.epollOut = false +} + +// modEpollOut sets cs's interest in this loop's epoll set to events, by +// cs.fd, and reports whether it issued the MOD, and the MOD's error. +// +// It does not issue it for a conn an async Hijack has taken (celeris#668). +// Every site that arms or disarms EPOLLOUT for an async conn does so without +// detachMu, or after failing to take it, so the conn's handler can hijack it +// meanwhile. The hijack stores hijacked, clears the slot under driverMu, and +// only then closes the descriptor, whose number another file can take at +// once, one a driver goroutine adds to this epoll set among them. Read under +// driverMu, hijacked false means the descriptor is still the conn's until +// the MOD has run. A hijacked conn is out of the epoll set already. A sync +// conn is only ever hijacked inline, on this thread, so it skips the lock. +func (l *Loop) modEpollOut(cs *connState, events uint32) (issued bool, err error) { + if cs.detachMu != nil { + l.driverMu.RLock() + defer l.driverMu.RUnlock() + if cs.hijacked.Load() { + return false, nil + } + } + return true, unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &unix.EpollEvent{ + Events: events, Fd: int32(cs.fd), }) - cs.epollOut = false } // handleWritable resumes a backpressured conn on an EPOLLOUT event: flush From 0eb283faddfa79bde733ccad7eb948cc55db7fe8 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:44:00 +0200 Subject: [PATCH 19/20] perf(epoll): no defer around modEpollOut's lock, and a benchmark of the lock's cost on the send path (review of #698) --- engine/epoll/epollout_bench_linux_test.go | 56 +++++++++++++++++++++++ engine/epoll/loop.go | 23 ++++++---- 2 files changed, 69 insertions(+), 10 deletions(-) create mode 100644 engine/epoll/epollout_bench_linux_test.go diff --git a/engine/epoll/epollout_bench_linux_test.go b/engine/epoll/epollout_bench_linux_test.go new file mode 100644 index 00000000..bbef84d7 --- /dev/null +++ b/engine/epoll/epollout_bench_linux_test.go @@ -0,0 +1,56 @@ +//go:build linux + +package epoll + +import ( + "sync" + "testing" + + "golang.org/x/sys/unix" +) + +// BenchmarkEPOLLOUTArmDisarm measures one EPOLLOUT arm and disarm, the two +// EPOLL_CTL_MODs a backpressured flush costs the loop (armEpollOut when a +// flush stops short, disarmEpollOut once the socket drains). For an async +// conn, modEpollOut reads hijacked under driverMu.RLock before each MOD +// (celeris#668, review of #698); a sync conn takes no lock. The two +// sub-benchmarks differ by that lock alone. +func BenchmarkEPOLLOUTArmDisarm(b *testing.B) { + for _, async := range []bool{false, true} { + name := "sync" + if async { + name = "async" + } + b.Run(name, func(b *testing.B) { + epfd, err := unix.EpollCreate1(unix.EPOLL_CLOEXEC) + if err != nil { + b.Fatalf("epoll_create1: %v", err) + } + defer func() { _ = unix.Close(epfd) }() + pair, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM|unix.SOCK_NONBLOCK|unix.SOCK_CLOEXEC, 0) + if err != nil { + b.Fatalf("socketpair: %v", err) + } + defer func() { _ = unix.Close(pair[0]); _ = unix.Close(pair[1]) }() + if err := unix.EpollCtl(epfd, unix.EPOLL_CTL_ADD, pair[0], &unix.EpollEvent{ + Events: unix.EPOLLIN | unix.EPOLLET | unix.EPOLLRDHUP, + Fd: int32(pair[0]), + }); err != nil { + b.Fatalf("epoll_ctl ADD: %v", err) + } + l := &Loop{epollFD: epfd} + cs := &connState{fd: pair[0]} + if async { + cs.detachMu = &sync.Mutex{} + } + b.ReportAllocs() + for b.Loop() { + l.armEpollOut(cs) + l.disarmEpollOut(cs) + } + if cs.epollOut { + b.Fatal("EPOLLOUT still armed after the disarm") + } + }) + } +} diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index f565b8c6..5dbe4017 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -2833,18 +2833,21 @@ func (l *Loop) disarmEpollOut(cs *connState) { // driverMu, hijacked false means the descriptor is still the conn's until // the MOD has run. A hijacked conn is out of the epoll set already. A sync // conn is only ever hijacked inline, on this thread, so it skips the lock. +// +// The cost of the lock on this send-path call: BenchmarkEPOLLOUTArmDisarm. func (l *Loop) modEpollOut(cs *connState, events uint32) (issued bool, err error) { - if cs.detachMu != nil { - l.driverMu.RLock() - defer l.driverMu.RUnlock() - if cs.hijacked.Load() { - return false, nil - } + ev := unix.EpollEvent{Events: events, Fd: int32(cs.fd)} + if cs.detachMu == nil { + return true, unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &ev) } - return true, unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &unix.EpollEvent{ - Events: events, - Fd: int32(cs.fd), - }) + l.driverMu.RLock() + if cs.hijacked.Load() { + l.driverMu.RUnlock() + return false, nil + } + err = unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &ev) + l.driverMu.RUnlock() + return true, err } // handleWritable resumes a backpressured conn on an EPOLLOUT event: flush From 0dbd529176851dadb3524f8862bcc39ce4ade073 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 16:44:00 +0200 Subject: [PATCH 20/20] test(epoll): the hijack fixture fails instead of skipping, so CI cannot pass a test that never ran (review of #698) --- engine/epoll/hijack_closeconn_race_linux_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/engine/epoll/hijack_closeconn_race_linux_test.go b/engine/epoll/hijack_closeconn_race_linux_test.go index 77487107..083118e7 100644 --- a/engine/epoll/hijack_closeconn_race_linux_test.go +++ b/engine/epoll/hijack_closeconn_race_linux_test.go @@ -67,7 +67,9 @@ type hijackRaceRig struct { // hijackRaceConn builds an async-mode HTTP1 connection on a socketpair and // registers it with l exactly as acceptAll would: armed in epoll, in the conn -// table, in the live set, counted. h1State is non-nil and never Detached, +// table, in the live set, counted. It fails, never skips, when the +// setup does: every test built on it runs in CI without -v, where a skip +// would pass unseen (review of #698). h1State is non-nil and never Detached, // which is what selects closeConn's plainClose branch (SHUT_WR + Close) — // the path that closes the descriptor a second time. // @@ -87,27 +89,27 @@ func hijackRaceConn(t *testing.T) *hijackRaceRig { pair, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) if err != nil { - t.Skipf("socketpair unavailable: %v", err) + t.Fatalf("socketpair unavailable: %v", err) } local, peer := pair[0], pair[1] t.Cleanup(func() { _ = unix.Close(peer) }) if local >= connTableSize { _ = unix.Close(local) - t.Skipf("socketpair fd %d exceeds connTableSize %d", local, connTableSize) + t.Fatalf("socketpair fd %d exceeds connTableSize %d", local, connTableSize) } // Reads on peer are only ever done after a completed write on the other // end, so non-blocking cannot lose data — it only keeps a broken // expectation from hanging the test instead of failing it. if err := unix.SetNonblock(peer, true); err != nil { _ = unix.Close(local) - t.Skipf("set peer non-blocking: %v", err) + t.Fatalf("set peer non-blocking: %v", err) } if err := unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_ADD, local, &unix.EpollEvent{ Events: unix.EPOLLIN | unix.EPOLLET | unix.EPOLLRDHUP, Fd: int32(local), }); err != nil { _ = unix.Close(local) - t.Skipf("epoll_ctl ADD fd %d: %v", local, err) + t.Fatalf("epoll_ctl ADD fd %d: %v", local, err) } cs := &connState{fd: local, liveIdx: -1}