Fix race condition in CleanupAddon.subscribeRenderingChanged#3725
Draft
vogella wants to merge 2 commits intoeclipse-platform:masterfrom
Draft
Fix race condition in CleanupAddon.subscribeRenderingChanged#3725vogella wants to merge 2 commits intoeclipse-platform:masterfrom
vogella wants to merge 2 commits intoeclipse-platform:masterfrom
Conversation
Contributor
Test Results 3 024 files + 9 3 024 suites +9 2h 5m 40s ⏱️ + 1m 27s For more details on these failures, see this check. Results for commit d5fa691. ± Comparison against base commit e6d3cbf. ♻️ This comment has been updated with latest results. |
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. |
vogella
force-pushed
the
fix-cleanup-addon-race-condition
branch
from
aa9e3b7 to
0b45f54
Compare
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 <noreply@anthropic.com>
vogella
force-pushed
the
fix-cleanup-addon-race-condition
branch
from
0b45f54 to
9b6c852
Compare
Contributor
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
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
2 participants
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