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/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 new file mode 100644 index 00000000..b793d644 --- /dev/null +++ b/engine/epoll/async_handler_stall_linux_test.go @@ -0,0 +1,875 @@ +//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 owes the conn back +// (relink); TestAConnGivenUpMidHandlerIsHandedBack follows it home. +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") + } + 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 +// 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 + 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 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") + } +} + +// 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) + 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 || !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) + } + 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 deferred peer close was lost: slot=%p hooks=%d", l.conns[local], rig.disconnects.Load()) + } + }) + } +} + +// 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) + } +} + +// 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 +// 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 +// 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()) + } +} + +// 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, +// 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/conn.go b/engine/epoll/conn.go index b98f1b8b..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 @@ -195,19 +201,65 @@ 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 // 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) - hijacked bool + // 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. + // + // 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 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, 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 + + // 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{ @@ -274,11 +326,17 @@ 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 cs.liveIdx = -1 - cs.hijacked = false + 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/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/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/hijack_closeconn_race_linux_test.go b/engine/epoll/hijack_closeconn_race_linux_test.go index 8b069ed3..083118e7 100644 --- a/engine/epoll/hijack_closeconn_race_linux_test.go +++ b/engine/epoll/hijack_closeconn_race_linux_test.go @@ -26,16 +26,34 @@ 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 // 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. @@ -49,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. // @@ -69,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} @@ -237,6 +257,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 +299,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 @@ -302,9 +326,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. @@ -315,7 +339,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() @@ -324,11 +349,11 @@ func TestCloseConnDoesNotRecloseConnHijackedWhileWaitingOnDetachMu(t *testing.T) l.detachQPending.Store(1) l.detachQMu.Unlock() l.drainDetachQueue() - if cs.hijacked { - t.Error("drainDetachQueue skipped the hijacked pool release (detachClosed short-circuit)") + if !cs.hijackSettled || cs.liveIdx != -1 { + t.Errorf("drainDetachQueue did not settle the hijack (settled=%v liveIdx=%d)", cs.hijackSettled, cs.liveIdx) } - 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) } } @@ -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.hijackSettled { + t.Error("the hand-back did not settle the hijack") } } // 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") } @@ -542,8 +594,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) } }) @@ -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.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) + } + 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/hijack_offthread_linux_test.go b/engine/epoll/hijack_offthread_linux_test.go new file mode 100644 index 00000000..f6889993 --- /dev/null +++ b/engine/epoll/hijack_offthread_linux_test.go @@ -0,0 +1,909 @@ +//go:build linux + +package epoll + +import ( + "bufio" + "context" + "fmt" + "io" + "net" + "net/http" + "os" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "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" +) + +// 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 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 { + 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 "+ + "loop drains the hijack's notice", 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.hijackSettled { + t.Error("the hand-back did not settle the hijack") + } +} + +// 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)) + } + } +} + +// 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. +// +// 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) + } + t.Cleanup(func() { _ = unix.Close(epfd) }) + lfd, err := createListenSocket("127.0.0.1:0", true) + 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 + fillHolesBelow(t, 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.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) + } + handBack(l, cs) + assertLiveSet(t, l, next) + if l.connCount != 1 { + t.Errorf("connCount = %d after the hand-back, want 1", l.connCount) + } + return true +} + +// 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. 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(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 { + t.Fatalf("read the pipe: %v", err) + } + if cs.dirty { + t.Error("the hijacked conn is still on the dirty list") + } +} + +// 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) + } + }) + } +} + +// 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) { + 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 +// 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) + } +} + +// 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] +} diff --git a/engine/epoll/livecs_linux_test.go b/engine/epoll/livecs_linux_test.go new file mode 100644 index 00000000..a75bca9f --- /dev/null +++ b/engine/epoll/livecs_linux_test.go @@ -0,0 +1,9 @@ +//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 { + return l.liveConns[i] +} diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go index 203ac6c4..7b239f06 100644 --- a/engine/epoll/loop.go +++ b/engine/epoll/loop.go @@ -86,12 +86,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 @@ -313,7 +323,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, @@ -592,8 +602,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 { @@ -602,28 +613,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. + // 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 { - 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 { @@ -683,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. @@ -816,10 +763,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 { @@ -844,6 +793,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 @@ -990,7 +972,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 { @@ -1166,7 +1156,10 @@ func (l *Loop) lingerTimeoutMs(ms int) int { // 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 { @@ -1211,33 +1204,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 } @@ -1574,8 +1547,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 @@ -1627,6 +1600,81 @@ 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 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, nil) { + 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'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 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 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. +// +// 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. +// +// 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.asyncDetachUnlocked + if busy && owe != nil { + *owe = 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 @@ -1637,38 +1685,57 @@ 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 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) - 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) @@ -1676,32 +1743,26 @@ 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, 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 — 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. 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. // - // 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 + // 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 { + 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 @@ -1946,7 +2007,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 @@ -2193,20 +2261,25 @@ 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 // 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 @@ -2228,8 +2301,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 @@ -2237,13 +2316,9 @@ 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) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) return } // Double-buffer swap: hand asyncInBuf to the goroutine, reuse @@ -2277,9 +2352,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) @@ -2309,20 +2388,16 @@ 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) } - 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 @@ -2340,13 +2415,9 @@ 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) - 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 @@ -2396,11 +2467,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 { @@ -2415,18 +2482,42 @@ 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) - l.detachQPending.Store(1) - l.detachQMu.Unlock() - l.wakeFD.Signal() + l.enqueueDetach(cs) return } } } +// 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 + // 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 +} + +// 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 @@ -2436,38 +2527,65 @@ 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 } // 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 { - // 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) + // 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() { + 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 @@ -2485,6 +2603,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 } @@ -2529,6 +2648,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 @@ -2539,6 +2661,108 @@ 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 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 { + 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 +// conn to EPOLLOUT. Loop thread only. +func (l *Loop) flushDirty() { + for cs := l.dirtyHead; cs != nil; { + next := cs.dirtyNext + 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, + // 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 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. (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 + } + 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 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 @@ -2557,13 +2781,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. @@ -2571,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 @@ -2583,28 +2810,75 @@ 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) { if !cs.epollOut { return } - _ = unix.EpollCtl(l.epollFD, unix.EPOLL_CTL_MOD, cs.fd, &unix.EpollEvent{ - Events: unix.EPOLLIN | unix.EPOLLET | unix.EPOLLRDHUP, - Fd: int32(cs.fd), - }) + _, _ = 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. +// +// The cost of the lock on this send-path call: BenchmarkEPOLLOUTArmDisarm. +func (l *Loop) modEpollOut(cs *connState, events uint32) (issued bool, err error) { + 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) + } + 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 // the pending bytes and, if fully drained, disarm EPOLLOUT and restore the // 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 := 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 + // 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 + } 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 { @@ -2617,7 +2891,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 { @@ -2665,7 +2939,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() @@ -2681,17 +2955,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 @@ -2767,11 +3044,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 @@ -2874,27 +3155,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 dispatchBusy(cs, &cs.closeOwed) { + 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 @@ -2925,14 +3226,17 @@ 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, 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 @@ -2941,6 +3245,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 — @@ -3078,10 +3393,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 { @@ -3139,11 +3453,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 @@ -3151,6 +3468,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 d09fcef2..eba853b8 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. @@ -325,12 +328,25 @@ func TestHijackDefersReleaseWhileAsyncGoroutineActive(t *testing.T) { l.drainDetachQueue() - // releaseConnState clears hijacked + zeroes fd. - if cs.hijacked { - 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 cs.fd != 0 { - t.Errorf("cs.fd = %d after release, want 0", cs.fd) + 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 != 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) } } @@ -358,7 +374,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 +396,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..a06b1f1d 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 @@ -207,7 +214,10 @@ 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); 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 } @@ -248,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 @@ -275,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 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{},