Repository navigation
Fix NPE and cross-workspace corruption when running the same statistic concurrently - #3309
Merged
Merged
Conversation
…c concurrently Every StatisticsUI implementation is a singleton shared by all workspaces and stashes the running Statistics instance in a field between setup() and unsetup(). Running the same statistic (e.g. Statistical Inference) in two workspaces at the same time let one workspace's completion null out or overwrite the field while the other was still reading it, causing an NPE in getValue() (or silently attributing the wrong workspace's values). Serialize execution per statistics class in StatisticsControllerUIImpl: a second concurrent run of the same statistics class is now rejected with a warning dialog instead of racing, while different statistics classes and concurrent runs in general remain fully parallel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… SPI A code review of the previous commit found the semaphore approach still left every statistic with a settings panel exposed to the same race (the lock never covered StatisticsFrontEnd's pre-execute setup()/unsetup() calls), and for statistics without one it turned the crash into silent data corruption instead of fixing it. Fix the actual root cause instead: StatisticsUI.getValue() had to read a Statistics instance stashed in a field by setup(), which is unsafe since every StatisticsUI implementation is a singleton shared by all workspaces. Add StatisticsUI.getValue(Statistics) as a default method (delegating to the deprecated getValue() for any implementation that hasn't migrated, so third-party StatisticsUI plugins keep compiling and working unchanged) and migrate every first-party implementation plus their only caller (StatisticsModelUIImpl.addResult) to it. The result no longer depends on any shared mutable state, so concurrent runs of the same statistic across workspaces just work, with no locking or rejection needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Description
Running the same statistic (e.g. Statistical Inference) in two workspaces at the same time throws:
Root cause: every
StatisticsUIimplementation is registered as a@ServiceProvidersingleton shared by all workspaces.setup(statistics)stashes the runningStatisticsinstance in an instance field,getValue()reads it,unsetup()nulls it. Two workspaces running the same statistics class concurrently race on that shared field: whichever finishes first callsunsetup()and nulls it out from under the other, which crashes ongetValue()(or, without the crash, could silently read/report the wrong workspace's value). Most of the ~16StatisticsUIimplementations follow this same pattern, so the bug isn't specific to Statistical Inference.An earlier version of this PR serialized runs per statistics class with a
Semaphore. A review caught that this didn't actually close the bug:StatisticsFrontEnd's pre-executionsetup()/unsetup()calls around a statistic's settings panel (e.g. Modularity) happen outside that lock, so the same race — and for statistics with no settings panel, a redundantsetup()call ahead of the lock check turned the crash into guaranteed silent data corruption instead of fixing it.This version fixes the actual root cause instead:
StatisticsUI.getValue()is deprecated and a newgetValue(Statistics)is added as a default method (falling back to the deprecatedgetValue()for any implementation that hasn't migrated, so third-partyStatisticsUIplugins keep compiling and working unchanged, just without the fix). Every first-party implementation (16 classes) and their one caller (StatisticsModelUIImpl.addResult) are migrated to read the result from the passed-inStatisticsinstance directly instead of a shared field. With no more shared mutable state involved, concurrent runs of the same statistic across workspaces just work — no locking, no rejection dialog, no behavior change for the happy path.Checklist
Added tests?
Added to documentation?
🤖 Generated with Claude Code