Fix race condition in CleanupAddon.subscribeRenderingChanged #3725
+10
−17
Conversation
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
Contributor
Contributor
Author
|
AFAICS this is a save change (famous last words) and should finally fix our forever flaky cleanup race condition revealed in the cleanup test. |
Contributor
Author
|
I suggest to wait for the beginn of the next release developement for this one. |
The test ensureCleanUpAddonCleansUp was failing intermittently because CleanupAddon deferred container cleanup via Display.asyncExec(), creating a race condition where event loop pumping during part activation could interfere with async cleanup tasks. Root cause analysis: - When hidePart() is called on parts with REMOVE_ON_HIDE_TAG, it triggers synchronous EMF model change events via UIEventPublisher - Event delivery is fully synchronous: EventBroker.send() -> EventAdmin.sendEvent() -> Display.syncExec() -> handler execution - The subscribeRenderingChanged handler detected visCount==0 but deferred cleanup via asyncExec (line 352) - Meanwhile, activate() calls during hidePart() pumped the event loop (via focusGui/SWT focus handling), potentially consuming queued asyncExecs before they could observe the final state - This created timing-dependent behavior where cleanup could be skipped Fix: - Remove asyncExec wrapper from subscribeRenderingChanged's visCount==0 path - Since event delivery is synchronous, visCount already reflects the actual state at event time - no need to defer and re-check - Container is now hidden immediately when last renderable child becomes non-renderable, before any event loop pumping can interfere - Update test to use simple spinEventLoop + assertFalse instead of 30-second waitForCondition, as cleanup is now synchronous This fixes the flaky test that failed ~1-3% of the time even with the 30-second timeout. The subscribeTopicChildren handler still uses asyncExec for children removal/sash unwrapping, but toBeRendered cleanup is now deterministic. Fixes eclipse-platform#3581 Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
aa9e3b7 to
0b45f54
Compare
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.
The test ensureCleanUpAddonCleansUp was failing intermittently because CleanupAddon deferred container cleanup via Display.asyncExec(), creating a race condition where event loop pumping during part activation could interfere with async cleanup tasks.
Root cause analysis:
EventAdmin.sendEvent() -> Display.syncExec() -> handler execution
Fix:
This fixes the flaky test that failed ~1-3% of the time even with the 30-second timeout. The subscribeTopicChildren handler still uses asyncExec for children removal/sash unwrapping, but toBeRendered cleanup is now deterministic.
Fixes #3581