fix(safego): eliminate WithRestartTimeout data race by capturing restartTimeout at spawn - #1465
Open
kotwal-itpro wants to merge 1 commit into
Open
fix(safego): eliminate WithRestartTimeout data race by capturing restartTimeout at spawn#1465kotwal-itpro wants to merge 1 commit into
kotwal-itpro wants to merge 1 commit into
Conversation
…artTimeout at spawn
`Execution.WithRestartTimeout` mutates `exec.restartTimeout` from the
caller's goroutine, while the spawned goroutine's panic-recovery `defer`
reads that field on restart. When a caller uses the documented
`RunWithRestart(f).WithRestartTimeout(t)` builder pattern (as safego's
own TestHandlePanicAndRestart does), the goroutine is already running
by the time the setter fires — any panic in the interim races the write.
`go test -race ./bulker/jitsubase/safego/` reproduces it deterministically:
WARNING: DATA RACE
Read at 0x... by goroutine N:
safego.(*Execution).run.func1.1() safego.go:56
Previous write at 0x... by goroutine M:
safego.(*Execution).WithRestartTimeout() safego.go:68
safego.RunWithRestart / Run are called in 20+ production paths across
bulker/, ingest/, kafkabase/, sync-controller/, bulkerapp/,
eventslog/, and jitsubase/, so the primitive is load-bearing. No
current production caller happens to chain WithRestartTimeout, so the
race does not fire in shipped code, but the pattern is publicly
exported and documented — any future caller trips it.
Fix (backward compatible):
1. New constructor RunWithRestartTimeout(f, timeout) sets the timeout
before the goroutine spawns, eliminating the need for the mutating
builder pattern for the common case.
2. run() captures restartTimeout into a function-local at spawn time,
so a subsequent mutation via the deprecated setter (or any future
mutation of the shared Execution) cannot race the goroutine's
recovery handler.
3. WithRestartTimeout is retained for source-level backward
compatibility and marked with a Deprecated doc comment pointing
callers at the race-free constructor.
Also updates TestHandlePanicAndRestart to use the new constructor and
an atomic.Int32 counter (the previous plain-int shared counter was
itself flagged by -race). The test now passes -race -count=5.
Verified:
- `go test -race -count=5 ./bulker/jitsubase/safego/` — 5/5 pass
- `go build ./...` clean on jitsubase, kafkabase, bulkerlib,
bulkerapp, ingest, sync-controller
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Execution.WithRestartTimeoutmutatesexec.restartTimeoutfrom the caller's goroutine, while the spawned goroutine's panic-recoverydeferreads that same field on restart. When a caller uses the documentedRunWithRestart(f).WithRestartTimeout(t)builder pattern (as safego's ownTestHandlePanicAndRestartdid), the goroutine is already running by the time the setter fires — any panic in the interim races the write.go test -race ./bulker/jitsubase/safego/reproduces it deterministically on currentnewjitsu:safego.RunWithRestart/safego.Runare called in 20+ production paths acrossbulker/,ingest/,kafkabase/,sync-controller/,bulkerapp/,eventslog/, andjitsubase/, so the primitive is load-bearing. No current production caller happens to chainWithRestartTimeout, so the race does not fire in shipped code — but the pattern is publicly exported and documented, so any future caller (or a copy-paste from the existing test) trips it.Fix (backward compatible)
RunWithRestartTimeout(f func(), timeout time.Duration) *Execution— sets the timeout before the goroutine spawns, eliminating the need for the mutating builder pattern in the common case.run()capturesrestartTimeoutinto a function-local at spawn time, so a subsequent mutation via the deprecated setter (or any future mutation of the sharedExecution) cannot race the goroutine's recovery handler.WithRestartTimeoutretained for source-level backward compatibility and marked with aDeprecated:doc comment pointing callers at the race-free constructor.Test
Updates
TestHandlePanicAndRestartto use the new constructor and anatomic.Int32counter (the previous plain-int shared counter was itself flagged by-race).Verified
go test -race -count=5 ./bulker/jitsubase/safego/— 5/5 pass (was consistently failing before)go build ./...clean onjitsubase,kafkabase,bulkerlib,bulkerapp,ingest,sync-controllerRun,RunWithRestart, andWithRestartTimeoutall keep their signatures; newRunWithRestartTimeoutis additive