Fix potential data race in Walk #5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a potential data race in the radix tree's Walk functionality by introducing a two-phase approach that allows walking tree keys without necessarily reading their values. The key innovation is the new WalkHandler interface with separate Check and Handle methods, enabling concurrent goroutines to safely walk the tree without race conditions.
Key Changes:
- Introduced
WalkHandlerinterface withCheck(key)andHandle(key, value)methods to separate key inspection from value access - Added
WalkSkipflag andShouldSkip()method to allow selective value reading - Made
WalkFnimplement theWalkHandlerinterface for backward compatibility - Refactored all Walk methods to use
WalkHandlerinstead ofWalkFndirectly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| radix.go | Core implementation: added WalkHandler interface, WalkSkip flag, and refactored Walk/WalkPrefix/WalkPath/recursiveWalk to use two-phase Check/Handle approach |
| radix_test.go | Comprehensive test additions including Skip tests, Stop in check/handle tests, parallel walk test for data race verification, and benchmark additions for key-only walks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // WalkHandler is the handler passed to Walk functions. | ||
| // When split into two steps, this allows us to walk the | ||
| // keys withut reading the values. |
There was a problem hiding this comment.
Typo in comment: "withut" should be "without"
| // keys withut reading the values. | |
| // keys without reading the values. |
| c.Assert(collect(r), qt.DeepEquals, []int{0, 1, 20, 3, 40, 5, 60, 7, 80, 9}) | ||
| }) | ||
|
|
||
| c.Run("Skip some ", func(c *qt.C) { |
There was a problem hiding this comment.
Trailing space in test name: "Skip some " should be "Skip some" (without trailing space)
| c.Run("Skip some ", func(c *qt.C) { | |
| c.Run("Skip some", func(c *qt.C) { |
The construct in armon#3 was added to allow setting values while walking but also allow other goroutines to walk the tree concurrently. But that premiss was flawed, as any walk would read the leaf values, hence causing a potential data race. This changes makes the Walk take an interface with an option al pre Check method that takes only the key, and a Handle method that takes both key and value. This allows the caller to skip reading the value if not needed.
1 participant
The construct in #3 was added to allow setting values while walking but also allow other goroutines to walk the tree concurrently.
But that premiss was flawed, as any walk would read the leaf values, hence causing a potential data race.
This changes makes the Walk take an interface with an option al pre Check method that takes only the key, and a Handle method that takes both key and value.
This allows the caller to skip reading the value if not needed.