Skip to content

Fix data race on Connection logger field - #59

Open
dimetron wants to merge 1 commit into
coder:mainfrom
dimetron:fix/atomic-logger
Open

Fix data race on Connection logger field#59
dimetron wants to merge 1 commit into
coder:mainfrom
dimetron:fix/atomic-logger

Conversation

@dimetron

@dimetron dimetron commented Sep 1, 2026

Copy link
Copy Markdown

Fixes #57.

The race

NewConnection spawns receive and processNotifications in its constructor, so a caller can only reach SetLogger after those goroutines are already running. SetLogger wrote the logger field with no synchronization while they read it through loggerOrDefault, so installing a logger races the connection's own logging.

Under go test -race this aborts every test that connects an ACP subprocess — which is how it surfaced downstream (pi-go pins a patched copy of v0.13.5 behind a go.mod replace today; this PR is that patch, upstreamed).

The change

logger becomes an atomic.Pointer[slog.Logger]:

logger atomic.Pointer[slog.Logger]

func (c *Connection) SetLogger(l *slog.Logger) { c.logger.Store(l) }

func (c *Connection) loggerOrDefault() *slog.Logger {
    if l := c.logger.Load(); l != nil {
        return l
    }
    return slog.Default()
}

A single lock-free atomic load covers the hot loggerOrDefault read path: no mutex contention, and no lock-reentrancy risk against the connection's other mutexes. The pointer is replaced wholesale rather than mutated, so an atomic is sufficient — a sync.RWMutex would buy nothing here.

No API change: SetLogger's signature and semantics are unchanged, and sync/atomic was already imported.

A constructor-level WithLogger(...) option would also close the hole and would let the very first log line carry the caller's logger, but that is an API addition; this keeps the fix to the minimum and leaves that option open.

Test

TestConnectionSetLogger_ConcurrentWithReceive feeds the connection unparseable lines — which drive receive into loggerOrDefault — while calling SetLogger concurrently. It fails under -race without the change:

WARNING: DATA RACE
--- FAIL: TestConnectionSetLogger_ConcurrentWithReceive (0.01s)

and passes with it.

Verified

  • go test -race ./... — pass
  • go build ./example/... — pass
  • gofmt -l . — clean

NewConnection spawns the receive and processNotifications goroutines in its
constructor, so a caller can only reach SetLogger after those goroutines are
already running. SetLogger wrote the logger field with no synchronization
while they read it through loggerOrDefault, so any call to SetLogger raced
the connection's own logging. Under `go test -race` this aborts every test
that connects an ACP subprocess.

Store the logger in an atomic.Pointer instead. A single lock-free atomic load
covers the hot loggerOrDefault read path with no mutex contention, and no
lock-reentrancy risk against the connection's other mutexes; the pointer is
replaced wholesale, so an atomic is sufficient.

Add a regression test that feeds the connection unparseable input (which makes
receive log) while calling SetLogger concurrently. It fails under -race
without this change and passes with it.

Verified: go test -race ./... and go build ./example/... pass.

Fixes coder#57

Signed-off-by: Dmytro Rashko <dmitriy.rashko@amdocs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data race: SetLogger writes logger field unsynchronized; NewConnection already started goroutines

1 participant