abstractinterpret: Fix data race on shared binding_states#840
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #840 +/- ##
==========================================
+ Coverage 84.05% 84.10% +0.05%
==========================================
Files 12 12
Lines 3160 3170 +10
==========================================
+ Hits 2656 2666 +10
Misses 504 504 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aviatesk
left a comment
There was a problem hiding this comment.
Thanks for fixing the issue! That was very helpful. Also, sorry for taking so long to review it.
That map is mutated during inference whenever a method assigns to a global (e.g. in
global_assignment_rt_exct)
This isn’t exactly correct.
Since v0.11, report_package starts its analysis from the method signatures collected by Revise, so it generally shouldn’t infer top-level :const expressions.
That said, report_file still takes this hacky code path for practical reasons.
`report_package` analyzes method signatures concurrently via `Threads.@spawn` (aviatesk#785). Each task gets its own analyzer with refreshed local caches, but every task still shares a single `binding_states::IdDict` held in `AnalyzerState`. That map is mutated during inference whenever a method assigns to a global (e.g. in `global_assignment_rt_exct`), so concurrent `setindex!`/rehash corrupts it. It typically surfaces later as: UndefRefError: access to undefined reference from the `unique!` over the collected reports. The shared write itself dates back to aviatesk#698, but it was benign while `report_package` was single-threaded; aviatesk#785 made the analysis concurrent without isolating or guarding it. Binding partitions are a Julia 1.12 feature, so this affects both 1.12 and 1.13. Wrap `binding_states` in an `AbstractBindings` type that pairs the `IdDict` with a `ReentrantLock` and forwards `getindex`, `setindex!`, `haskey`, and `get` through it. The wrapper is also made lockable so the read-modify-write in `const_assignment_rt_exct` can run under one `@lock` and stay atomic. This keeps the shared-and-reused design intact (no per-task isolation, parallelism preserved) rather than giving each task a private map. Accesses are infrequent (global assignments and partition lookups) and the lock is uncontended in the common case, so the overhead is negligible. `test_typeinfer` and `test_virtualprocess` pass. I used Claude Opus 4.8 while implementing this.
report_packageanalyzes method signatures concurrently viaThreads.@spawn(#785). Each task gets its own analyzer with refreshed local caches, but every task still shares a singlebinding_states::IdDictheld inAnalyzerState. That map is mutated during inference whenever a method assigns to a global (e.g. inglobal_assignment_rt_exct), so concurrentsetindex!/rehash corrupts it. It typically surfaces later as:from the
unique!over the collected reports.The shared write itself dates back to #698, but it was benign while
report_packagewas single-threaded; #785 made the analysis concurrent without isolating or guarding it. Binding partitions are a Julia 1.12 feature, so this affects both 1.12 and 1.13.Wrap
binding_statesin anAbstractBindingstype that pairs theIdDictwith aReentrantLockand forwardsgetindex,setindex!,haskey, andgetthrough it. The wrapper is also made lockable so the read-modify-write inconst_assignment_rt_exctcan run under one@lockand stay atomic. This keeps the shared-and-reused design intact (no per-task isolation, parallelism preserved) rather than giving each task a private map.Accesses are infrequent (global assignments and partition lookups) and the lock is uncontended in the common case, so the overhead is negligible.
test_typeinferandtest_virtualprocesspass.I used Claude Opus 4.8 while implementing this.