fix(v2/httpserver): synchronize Addr/ProbeAddr with Start writes
Summary
httpserver.Server.Start writes the bound listener fields (s.ln, s.probeLn) at v2/httpserver/server.go:234,241 without any synchronisation against concurrent reads through the public Addr() / ProbeAddr() accessors. The accessors are documented for cross-goroutine use ("Use this after starting with Addr :0 to discover the bound port"), so callers reasonably expect them to be race-safe — yet go test -race flags two distinct races whenever a caller spawns go app.Run(ctx) and then polls the accessors to discover the bound ephemeral ports:
- Interface-field assignment race. The two-word write of the
net.Listeneriface header tos.probeLnraces with the nil-check read inProbeAddr()(server.go:217). - TCPListener byte-read race.
(*net.TCPListener).Addr()reads bytes that are still being written bynet.ListenConfig.ListeninStart.
Sample race trace (matches the trace in the AR work item):
WARNING: DATA RACE
Write at 0x00c0003b2270 by goroutine 11:
...httpserver.(*Server).Start() server.go:234
Previous read at 0x00c0003b2270 by goroutine 12:
...httpserver.(*Server).Addr() server.go:207Fix
Guard the two listener fields with a sync.Mutex:
Startnow publishes both listeners under the lock after bothListencalls succeed. Readers therefore never observe a half-initialised state (e.g. the app listener bound but the probe stillnil) either.Addr()andProbeAddr()take the same lock around the nil-check and theAddr()call.
The lock is uncontended in production (one writer in Start, occasional post-Start readers from operators/tests) and adds no measurable overhead. http.Server keeps its own listener reference internally, so Shutdown is unaffected.
This matches the upstream-resolution sketch in the work item.
Test plan
-
TestServer_Addr_ProbeAddr_RaceFreeadded — pollsAddr()/ProbeAddr()from a separate goroutine whileStartruns. Reproduces both races under-raceagainst master (verified) and passes after the fix. -
go test -race ./...(full v2 module) passes. -
./scripts/test.shpasses. -
./scripts/golangci-lint.shreports 0 issues for both modules.
Downstream unblock
Surfaced by gitlab-org/ops/artifact-registry#100 (closed), whose S01 Step 6 lifecycle integration tests currently skip under -race pending this fix. Once this lands in a v2 patch release, AR can bump its go.mod pin and drop the testing.RaceEnabled skip in cmd/artifact-registry/main_test.go.
Related
- gitlab-org/labkit
v2/httpserver/server.go:213-232— the previously unsynchronised accessors - gitlab-org/labkit
v2/httpserver/server.go:234-262— the writer inStart