Skip to content

fix(server): close conns channel only after accept loops exit - #3755

Open
m-u-xyz wants to merge 1 commit into
dolthub:mainfrom
m-u-xyz:fix/listener-close-race
Open

fix(server): close conns channel only after accept loops exit#3755
m-u-xyz wants to merge 1 commit into
dolthub:mainfrom
m-u-xyz:fix/listener-close-race

Conversation

@m-u-xyz

@m-u-xyz m-u-xyz commented Sep 2, 2026

Copy link
Copy Markdown

Problem

Listener.Close raced with its own accept loops, causing intermittent
send on closed channel panics and -race failures.

The accept loops send accepted connections into l.conns via a select
that also watches l.shutdown:

select {
case <-l.shutdown:
    conn.Close()
    return nil
case l.conns <- connRes{conn, err}:
}

Go's select picks randomly among ready cases. Close did:

l.once.Do(func() {
    close(l.shutdown)
    close(l.conns)   // <-- while a loop may still be mid-send
})
return l.eg.Wait()

So a loop that had just accepted a connection could win the conns <-
arm and send into a channel Close was closing concurrently →
runtime.closechan vs runtime.chansend race (panics or trips the
detector). This showed up downstream in dolt's sql-server tests as a
flaky race on server/listener.go:101 vs :149 during rapid
start/stop.

Fix

Split the single sync.Once into shutdownOnce and connsOnce so
Close can sequence the two channel closings around eg.Wait():

  1. close(shutdown) — loops observe it and return
  2. close the underlying net.Listeners — unblocks Accept(), loops exit
  3. eg.Wait()all senders are now guaranteed gone
  4. close(conns) — safe; no sender remains

Accept()'s contract is preserved: it still unblocks with
net.ErrClosed because conns is still closed, just after the loops
drain rather than before.

Reproduction / test

server/listener_race_test.go adds a deterministic repro that:

  • starts a Listener but withholds the Accept() consumer so the
    loop's conns <- send stalls inside the select,
  • dials several real connections so the loop has a conn to send,
  • calls Close while that send is in flight,
  • repeats 50×.

On the original code this fails under -race:

WARNING: DATA RACE
--- FAIL: TestListenerCloseNoRace
runtime.closechan  (server/listener.go:149)
runtime.chansend   (server/listener.go:101)

On the patched code it passes (-race -count=5 clean). Covers both the
TCP and unix accept loops (they share conns) plus the post-Close
Acceptnet.ErrClosed contract.

Checklist

  • go build ./...
  • go vet ./server/
  • go test -race -count=5 -run TestListenerClose ./server/
  • Existing server package tests still pass
  • Test fails on the unfixed code (proven via stash/revert)

Listener.Close raced with its own accept loops: it closed the conns
channel (inside the sync.Once, before eg.Wait) while the loops could
still be selected to send into it. Go's select chooses randomly among
ready cases, so a loop that had just accepted a connection could pick
the `conns <-` arm over the `<-shutdown` arm and send into a channel
being closed — panicking (`send on closed channel`) or tripping the
race detector under -race.

This manifested in downstream dolt's sql-server tests as a flaky
`runtime.closechan` vs `runtime.chansend` race on server/listener.go
during rapid start/stop.

Fix: split the single sync.Once into shutdownOnce and connsOnce so
Close can sequence the two closings around eg.Wait():

  1. close shutdown (loops observe it and return)
  2. close the underlying net.Listeners (unblocks Accept -> loops exit)
  3. eg.Wait() — all senders are now guaranteed gone
  4. close conns — safe, no sender remains

Accept()'s contract is preserved: it still unblocks with net.ErrClosed
because conns is still closed (just later, after the loops drain).

Adds listener_race_test.go with a deterministic reproduction that
fails (WARNING: DATA RACE) on the original code and passes on the fix,
covering both the TCP and unix accept loops plus the post-Close Accept
contract.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants