Fix concurrent map access on task index map in flow tasks#209
Open
damilolaedwards wants to merge 1 commit into
Open
Fix concurrent map access on task index map in flow tasks#209damilolaedwards wants to merge 1 commit into
damilolaedwards wants to merge 1 commit into
Conversation
The concurrent and matrix flow tasks wrote the task index map inside the child spawn loop while each child's watcher goroutine read it. That is a concurrent map read and write, which the Go runtime turns into a fatal error that cannot be recovered, taking down the whole process and every running test. Populate the index map fully before spawning any child goroutine, since the mapping is known up front and never changes after that. Add race regression tests that drive the executors with many children and fail under -race without the fix.
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
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
The concurrent (
run_tasks_concurrent) and matrix (run_task_matrix) flow tasks wrote the task index map inside the child spawn loop, while each child's watcher goroutine read the same map. That is a concurrent map read and write.The Go runtime detects this and raises a fatal error ("concurrent map read and map write"), which is not a panic and cannot be recovered by the scheduler's per-task recover. When it triggers, the whole assertoor process dies, aborting every test that is currently running, not just the offending one.
The read happens in
watchChildTask(via the debug log of the task index), and the watcher goroutines are started fromExecuteTaskwhile the spawn loop is still writing entries for later children, so a fast or skipped child can hit the read before the map is fully populated.Fix
Populate the task index map fully in a dedicated pass before spawning any child goroutine. The mapping is known up front and never changes after that, so there is no reason to write it concurrently with the readers.
Tests
Each package gets a race regression test that drives the real
Executewith many children and a stub scheduler whoseExecuteTaskstarts the watcher goroutine exactly as the real scheduler does. The tests pass under-racewith the fix and fail without it.go build ./...,go vet, andgo test -raceon both packages pass.