Skip to content

fix: EventBucket.Wait() can return before a scheduled flush runs - #177

Open
samyfodil wants to merge 1 commit into
dylibso:mainfrom
samyfodil:fix-bucket-waitgroup-add-race
Open

samyfodil wants to merge 1 commit into
dylibso:mainfrom
samyfodil:fix-bucket-waitgroup-add-race

Conversation

@samyfodil

Copy link
Copy Markdown

What

scheduleFlush calls b.wg.Add(1) from inside the goroutine it spawns, instead of before starting it:

go func() {
    defer b.wg.Done()
    // ... flush logic
}()
b.wg.Add(1) // too late — Wait() may already be past wg.Wait()

sync.WaitGroup's contract requires Add to happen-before Wait observes the counter. Since Add(1) here runs from inside the new goroutine, there's a window where the goroutine hasn't been scheduled yet, the counter is still 0, and a concurrent call to Wait() returns immediately — before the flush has even been registered, let alone run.

Fix

Move wg.Add(1) to the caller side, before go func() { ... }().

Tests

Added TestBucketWaitBlocksUntilFlush, which fails consistently on the original code (Wait() returns having observed 0 flushes) and passes on the fix. Verified: fails 10/10 with -race -count=10 against the original bucket.go, passes 20/20 against the fix.

Also fixed a separate, pre-existing race in the existing TestBucket: it read TestFlusher.NumFlushes/FlushedEvents and bucket.bucket without synchronization while the flush goroutine concurrently wrote them. Added a mutex to TestFlusher and locked the reads. Unrelated to the wg.Add bug itself, just something -race surfaced while working in this file.

go build ./..., go vet ./..., and go test -race ./... are all clean on this branch.

Related to #176, which is a separate, deeper race in AdapterBase.Stop() found in the same file — this PR doesn't touch or fix that one.

scheduleFlush called b.wg.Add(1) from inside the goroutine it spawns
instead of before starting it. Add must happen-before Wait observes
the counter; calling it from within the goroutine means a concurrent
Wait() can return while wg's internal counter is still zero and the
flush hasn't even been registered yet, let alone run. In practice this
lets Wait() return before the scheduled flush has actually flushed,
which callers rely on it not doing.

Move wg.Add(1) to the caller side of the goroutine so the registration
happens-before any Wait() call can observe it.

Also fixes a separate, pre-existing data race in the existing
TestBucket test: it read TestFlusher.NumFlushes/FlushedEvents and
bucket.bucket without synchronization while the flush goroutine
concurrently writes them. Added a mutex to TestFlusher and locked
reads.

Adds TestBucketWaitBlocksUntilFlush, a deterministic test proving the
happens-before ordering: fails consistently on the original code
(Wait() returns with 0 flushes observed) and passes on the fix.

Signed-off-by: samyfodil <[email protected]>
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.

1 participant