Data race: Connection.SetLogger writes c.logger while receive() reads it
- Dominant language
- Go
- Stars
- 230
- Forks
- 37
- PR merge metrics
- No merged PRs in 30d
Description
`Connection.SetLogger` writes `c.logger` with no synchronization:
```go
// connection.go:105
func (c *Connection) SetLogger(l *slog.Logger) { c.logger = l }
// connection.go:107
func (c *Connection) loggerOrDefault() *slog.Logger {
if c.logger != nil {
return c.logger
}
return slog.Default()
}
```
`NewConnection` starts `receive()`, `sendCancelRequests()` and `processNotifications()` before returning (connection.go:97-99), and those goroutines read the field through `loggerOrDefault()`. Since `SetLogger` can only be called *after* the constructor returns, the documented way to install a logger always races with a connection that is already running — any diagnostic logged in that window (a parse error, "connection closed", …) is the read side.
### Reproduction
```go
func TestSetLoggerRace(t *testing.T) {
logger := slog.New(slog.NewTextHandler(io.Discard, nil))
for i := 0; i < 200; i++ {
_, c2aW := io.Pipe()
a2cR, a2cW := io.Pipe()
conn := acp.NewClientSideConnection(&testClient{}, c2aW, a2cR)
go func() {
_, _ = a2cW.Write([]byte("not json\n")) // makes receive() log a parse error
_ = a2cW.Close()
}()
conn.SetLogger(logger)
_ = c2aW.Close()
}
}
```
`go test -race` reports it on the first iteration:
```
WARNING: DATA RACE
Read at 0x00c00042c3a0 by goroutine 26:
acp.(*Connection).loggerOrDefault() connection.go:108
acp.(*Connection).receive() connection.go:364
acp.NewConnection.gowrap2() connection.go:98
Previous write at 0x00c00042c3a0 by goroutine 20:
acp.(*Connection).SetLogger() connection.go:105
acp.(*ClientSideConnection).SetLogger() client.go:27
```
Seen on v0.6.4-0.20260227160919-584abe6abe22; the same code is present on current main.
In our test suite this showed up as an intermittent `-race` failure under parallel load. We worked around it by not calling `SetLogger` at all and routing SDK diagnostics through `slog.Default()` instead, but that gives up per-connection loggers.
### Suggested fix
Store the logger in an `atomic.Pointer[slog.Logger]`:
```go
type Connection struct {
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()
}
```
An optional logger argument (or functional option) on `NewConnection` / `NewClientSideConnection` / `NewAgentSideConnection` would also close the window, and would let a logger be in place for anything the constructor's goroutines log first. Happy to send a PR for either shape.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in connection.go at Connection, SetLogger, loggerOrDefault, and the constructor goroutines identified in the issue; check the forwarding SetLogger methods in client.go. Run the supplied reproduction with go test -race. Done means concurrent logger updates no longer report a race, while per-connection diagnostics still use the configured logger.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 84/100