Skip to content

io_uring, epoll: a panic on the async dispatch goroutine leaves cs.detachMu locked, and closeConn then parks the whole worker on it #791

Description

@FumingPower3925

Summary

When an async dispatch goroutine's conn.ProcessH1 panics, runAsyncHandler recovers the panic but does not release cs.detachMu, which it took before the call. The recover then hands the conn to the worker for teardown. The worker's closeConn calls cs.detachMu.Lock(), and that call never returns. The worker (io_uring) or loop (epoll) thread is locked to its OS thread, so it stops serving every connection it owns. Listen does not return after its context is cancelled either (the tests wait 3 s, but nothing is left that could release the lock).

The same thing happens on both engines. The recover() was added by #240 as the last-resort safety net, and on this path it turns one connection's panic into a dead worker.

Found by code reading by the #715 test author. Confirmed here on origin/main 0cf0c52.

Mechanism (file:line at 0cf0c52)

io_uring

  1. engine/iouring/worker.go:4366-4370: runAsyncHandler takes cs.detachMu (if !cs.asyncDetachUnlocked { cs.detachMu.Lock() }) and calls conn.ProcessH1 at :4386. It releases the lock at :4379, :4420 and :4513. Each of those sites comes after a normal return from ProcessH1.
  2. worker.go:4282-4304: the deferred recover() sets asyncClosed, clears asyncRun, appends cs to detachQueue and signals the wake fd. It never releases cs.detachMu.
  3. worker.go:4782-4783: drainDetachQueue sees asyncClosed and calls w.closeConn(cs.fd). closeConn at worker.go:3662 calls cs.detachMu.Lock() and blocks forever.

epoll

  1. engine/epoll/loop.go:2343-2347: the same lock, taken before ProcessH1 at :2364. It is released at :2353, :2386 and :2467.
  2. loop.go:2252-2270: the same recover. It does not release the lock, and its cs.endDispatch() clears asyncRun.
  3. loop.go:2594-2595: drainDetachQueue calls l.closeConn. In closeConn, TryLock fails at :3175. dispatchBusy (loop.go:1665) then reports "not busy", because the recover already cleared asyncRun. So closeConn falls through to a blocking cs.detachMu.Lock() at loop.go:3179. The epoll: a timeout reap parks the whole loop thread on cs.detachMu for as long as an async handler runs (the epoll twin of the closed celeris#593) #669 escape (leave the close to a running goroutine) does not apply, because no goroutine is left.

Goroutine dump, taken 300 ms after the panicking requests (io_uring, 2 workers, arm64, -race). Both workers are parked here:

goroutine 44 [sync.Mutex.Lock, locked to thread]:
sync.(*Mutex).Lock(...)
github.com/goceleris/celeris/engine/iouring.(*Worker).closeConn(0xc00017c588, 0xd)
	/src/engine/iouring/worker.go:3662 +0xe4
github.com/goceleris/celeris/engine/iouring.(*Worker).drainDetachQueue(0xc00017c588)
	/src/engine/iouring/worker.go:4783 +0x208
github.com/goceleris/celeris/engine/iouring.(*Worker).run(0xc00017c588, {...})
	/src/engine/iouring/worker.go:1359 +0x20a0

epoll dump. Both loops are parked here:

goroutine 56 [sync.Mutex.Lock, locked to thread]:
sync.(*Mutex).Lock(...)
github.com/goceleris/celeris/engine/epoll.(*Loop).closeConn(0xc00023a588, 0x10)
	/src/engine/epoll/loop.go:3179 +0x164
github.com/goceleris/celeris/engine/epoll.(*Loop).drainDetachQueue(0xc00023a588)
	/src/engine/epoll/loop.go:2595 +0x4e4
github.com/goceleris/celeris/engine/epoll.(*Loop).run(0xc00023a588, {...})
	/src/engine/epoll/loop.go:666 +0x1360

runtime.Goexit inside ProcessH1 has the same root cause. recover() returns nil, so the deferred function does nothing, and cs.detachMu stays locked with asyncRun still true:

  • io_uring: the peer's FIN reaches handleRecv, which locks cs.detachMu at worker.go:2686-2687 and blocks. The worker wedges in the same way.
  • epoll: dispatchBusy reports busy, because asyncRun is still set, so closeConn leaves the close to a goroutine that no longer exists. The loop keeps serving, but those connections are never closed, and Listen does not return after cancel.

Reproduction

Scratch tests, not for merge: branch test/celeris-panic-detachmu-repro (1e09040, based on 0cf0c52), files engine/{iouring,epoll}/zz_panic_detachmu_linux_test.go. The engine refuses Workers: 1 (">= 2 if set"), so each test starts a 2-worker engine with AsyncHandlers: true and a stream.Handler. /boom is the only async route (RouteAsync).

Each test then does the following:

  1. Sends one warm-up /ok.
  2. Sends /boom 12 times, on fresh connections, so that every worker gets one.
  3. Takes a goroutine dump.
  4. Sends 16 /ok requests, each on a fresh connection with a 1 s deadline.
  5. Cancels the engine's context and waits 3 s for Listen to return.

Each arm ran in its own process, in golang:1.27 under Docker (linuxkit 7.0.12), with --security-opt seccomp=unconfined and -race.

arm (engine level) io_uring arm64 io_uring arm64, 8 MiB memlock (1 worker) epoll arm64 epoll amd64
AsyncPanic: /boom panics FAIL: 0/16 answered, both workers in closeConn:3662, Listen not stopped FAIL: 0/16, same frame FAIL: 0/16, both loops in closeConn:3179, not stopped FAIL: 0/16, same frame
AsyncError (control): /boom returns an error. The teardown route is the same (asyncClosed → detachQueue → closeConn), with the lock released PASS 16/16 (500 x12) PASS 16/16 PASS 16/16 PASS 16/16
AsyncGoexit: /boom calls runtime.Goexit FAIL: 0/16, both workers in handleRecv:2687 FAIL: 0/16, same 16/16 answered, 12 conns never closed, Listen not stopped same as arm64
SyncPanic: /boom runs inline on the worker (child process) the process dies (panic: zz handler panic, exit 2) same same same

The Docker amd64 container on this laptop is emulated and has no io_uring, so the same four arms also ran on native GitHub-hosted x86 and arm64 runners. These used probatorium celeris-stress runs 36365045110 (memlock 8 MiB, the celeris CI unit shape, 1 io_uring worker) and 36365050936 (memlock unlimited, 2 workers), both with -race. All four arch × memlock cells gave the same results:

  • AsyncPanic: 0/16 answered and not stopped. The io_uring workers are in closeConn and the epoll loops are in closeConn.
  • AsyncError: 16/16 answered and stopped.
  • AsyncGoexit: io_uring 0/16, in handleRecv. epoll 16/16 but not stopped.
  • SyncPanic: the child dies with the panic.

That workflow runs a package's tests in one process. So AsyncError's --- FAIL in those runs comes only from the earlier arm's still-wedged workers showing up in the same process's goroutine dump. Its own counts are 16/16 answered and stopped. No DATA RACE appeared in any run, local or native.

The sync path is a different matter. The engines have no recover on the inline path, so a raw stream.Handler that panics kills the process. The engines have never recovered there; for celeris.Server, routerAdapter covers it (the sync row of the next table). It is not this bug.

Through the public API (celeris.Server, 2 workers, /boom marked .Async() so that it really runs on the dispatch goroutine):

arm (celeris.Server) io_uring epoll
.Async() route panics PASS: 500 x12, 16/16 answered PASS, same
sync route panics (AsyncHandlers: false) PASS: 500 x12, 16/16 PASS, same
.Async() route calls runtime.Goexit FAIL: 0/16, both workers in handleRecv:2687, not stopped 16/16 answered, the /boom conns never closed, not stopped

The std engine (net/http recovers per connection, and no engine lock is held around the handler) passes every arm (panic, error, Goexit) on arm64 and amd64.

Impact

  • The whole worker (io_uring) or loop (epoll) stops, not just the one connection. It stops accepting, reading and writing for every connection it owns, and Listen/shutdown hangs. Every worker that receives one such panic stops. On the 2-worker test engine, the 12 panicking requests stopped all serving (0/16 answered).
  • Reachability through celeris.Server: an ordinary handler panic is recovered earlier, by routerAdapter.recoverAndRelease (handler.go:283, inside HandleStream). The table above shows it: 500, and the server survives. So the engine's recover is reached only by a panic that escapes routerAdapter, which covers the following cases:
  • runtime.Goexit from an .Async() handler, for example t.FailNow/t.SkipNow in a test's handler, does reach this through the public API, and it wedges io_uring workers.
  • The adaptive engine runs these two engines, so it inherits the bug.

Related issues (not duplicates)

#704 and #750 (io_uring) and #669 (epoll, fixed by #698) are about a live handler that holds cs.detachMu while it runs. In this bug the holder has already exited.

The #669 escape on epoll (TryLock, then leave the close to the dispatch goroutine) does not help here, as the tables above show:

  • After a panic: dispatchBusy reports "not busy", and the blocking Lock parks the loop forever.
  • After a Goexit: dispatchBusy reports "busy", and the close is left to a goroutine that no longer exists. The conn leaks and shutdown hangs.

A #704 fix of the same shape on io_uring would inherit both outcomes. The lock has to be released by the goroutine that took it, on its way out.

Fix direction

Release cs.detachMu on every abnormal exit of the dispatch goroutine, before the conn is handed to the worker. The two variants below were tried as scratch patches (not proposed as the fix):

variant AsyncPanic AsyncGoexit AsyncError control sync Server .Async() Goexit
R: unlock in the recover() branch only PASS (both engines) still FAIL (io_uring wedge; epoll leak and no stop) PASS unchanged still FAIL
D: a deferred release that runs whether recover() returned a value or not, plus the same teardown for a Goexit PASS (both) PASS (both) PASS unchanged PASS (both)

Both variants track whether this goroutine holds the lock (held, set after the Lock and cleared before each Unlock). They release it only if held && !cs.asyncDetachUnlocked. The reason is that OnDetach may already have released the lock inside ProcessH1 (#273). An unconditional Unlock there would be a fatal "unlock of unlocked mutex" (cf. #309).

Suggested shape: a deferred release in runAsyncHandler (both engines) that covers panic and runtime.Goexit. A Goexit should also get the asyncClosed + enqueue teardown, so the conn is closed instead of leaked. Add a regression test per engine: the AsyncPanic and AsyncGoexit arms above, with AsyncError as the control.

Variant D as tried (scratch, against 0cf0c52; the Goexit teardown reuses the panic branch)
diff --git a/engine/epoll/loop.go b/engine/epoll/loop.go
index 7b239f0..fd34d53 100644
--- a/engine/epoll/loop.go
+++ b/engine/epoll/loop.go
@@ -2249,8 +2249,15 @@ func (l *Loop) runAsyncHandler(cs *connState) {
 	// recover for the sync path; async dispatch needs symmetric
 	// protection because the panic would otherwise unwind here,
 	// outside any router code. See #240.
+	held := false // SCRATCH fix-direction check: this goroutine holds cs.detachMu
 	defer func() {
-		if r := recover(); r != nil {
+		r := recover()
+		abnormal := r != nil || (held && !cs.asyncDetachUnlocked)
+		if held && !cs.asyncDetachUnlocked {
+			held = false
+			cs.detachMu.Unlock()
+		}
+		if abnormal {
 			if l.logger != nil {
 				l.logger.Error("async handler panicked",
 					"panic", r,
@@ -2344,12 +2351,14 @@ func (l *Loop) runAsyncHandler(cs *connState) {
 		if !cs.asyncDetachUnlocked {
 			cs.detachMu.Lock()
 			acquiredDetachMu = true
+			held = true
 		}
 		// Re-check asyncClosed under detachMu when we acquired it;
 		// closeConn sets asyncClosed BEFORE tearing down cs.h1State.
 		// Mirrors the iouring fix.
 		if cs.asyncClosed.Load() {
 			if acquiredDetachMu {
+				held = false
 				cs.detachMu.Unlock()
 			}
 			// Nor does this one; see the loop-top exit.
@@ -2383,6 +2392,7 @@ func (l *Loop) runAsyncHandler(cs *connState) {
 					promoteErr = err
 				}
 			}
+			held = false
 			cs.detachMu.Unlock()
 			if promoteErr != nil {
 				cs.asyncClosed.Store(true)
@@ -2464,6 +2474,7 @@ func (l *Loop) runAsyncHandler(cs *connState) {
 			}
 		}
 		partial := flushErr == nil && (cs.writePos < len(cs.writeBuf) || len(cs.bodyBuf) > 0)
+		held = false
 		cs.detachMu.Unlock()
 
 		if partial {
diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go
index 157c13c..1c499f5 100644
--- a/engine/iouring/worker.go
+++ b/engine/iouring/worker.go
@@ -4279,8 +4279,15 @@ func (w *Worker) canRevertToInline(cs *connState) bool {
 
 func (w *Worker) runAsyncHandler(cs *connState) {
 	defer w.asyncWG.Done()
+	held := false // SCRATCH fix-direction check: this goroutine holds cs.detachMu
 	defer func() {
-		if r := recover(); r != nil {
+		r := recover()
+		abnormal := r != nil || (held && !cs.asyncDetachUnlocked)
+		if held && !cs.asyncDetachUnlocked {
+			held = false
+			cs.detachMu.Unlock()
+		}
+		if abnormal {
 			if w.logger != nil {
 				w.logger.Error("async handler panicked",
 					"panic", r,
@@ -4367,6 +4374,7 @@ func (w *Worker) runAsyncHandler(cs *connState) {
 		if !cs.asyncDetachUnlocked {
 			cs.detachMu.Lock()
 			acquiredDetachMu = true
+			held = true
 		}
 		// Re-check asyncClosed under detachMu (when we acquired it).
 		// closeConn sets asyncClosed BEFORE taking detachMu to run
@@ -4376,6 +4384,7 @@ func (w *Worker) runAsyncHandler(cs *connState) {
 		// don't call ProcessH1 on a closed state.
 		if cs.asyncClosed.Load() {
 			if acquiredDetachMu {
+				held = false
 				cs.detachMu.Unlock()
 			}
 			cs.asyncInMu.Lock()
@@ -4417,6 +4426,7 @@ func (w *Worker) runAsyncHandler(cs *connState) {
 					promoteErr = werr
 				}
 			}
+			held = false
 			cs.detachMu.Unlock()
 			if promoteErr != nil {
 				// Fatal — route through the asyncClosed teardown so the
@@ -4510,6 +4520,7 @@ func (w *Worker) runAsyncHandler(cs *connState) {
 				cs.writeBuf = cs.writeBuf[:0]
 			}
 		}
+		held = false
 		cs.detachMu.Unlock()
 
 		if processErr != nil {

Variant R is the same diff with abnormal := r != nil and the release guarded by r != nil.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/engineEngine interface or implementationbugSomething isn't workingengine/epollEpoll engine specificsengine/iouringio_uring engine specificsplatform/linuxLinux-specific (io_uring, epoll)

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions