Don't use display thread for scheduling console update tasks#2461
Open
iloveeclipse wants to merge 8 commits intoeclipse-platform:masterfrom
Open
Don't use display thread for scheduling console update tasks#2461iloveeclipse wants to merge 8 commits intoeclipse-platform:masterfrom
iloveeclipse wants to merge 8 commits intoeclipse-platform:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a deadlock issue (#2460) that occurred when terminating launches. The deadlock happened because the UI thread would wait for a lock on ProcessConsole to update the console name, while console code was blocked waiting for a QueueProcessingJob to execute on the same UI thread. The fix introduces a separate ScheduledExecutorService to handle console name updates asynchronously, avoiding direct calls from the UI thread to ProcessConsole.resetName().
Changes:
- Replaced UI thread timer-based scheduling (
Display.timerExec) with a dedicatedScheduledExecutorServicefor console name updates - Added proper executor cleanup in the
dispose()method - Introduced tracking of pending name update tasks to avoid duplicate scheduling
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/views/console/ProcessConsole.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/views/console/ProcessConsole.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/views/console/ProcessConsole.java
Outdated
Show resolved
Hide resolved
Member
Author
|
@trancexpress : if you can reproduce, please try this PR. Beside this, I wonder if this task that updates console every second must be always executed, even if console is hidden behind other one. This is definitely a resource waste. Start ~6 processes with Console and enjoy that every 100 ms some task is executed updating some of the consoles. This makes no sense. So probably the code has to be rewritten to avoid parallel updates of hidden console pages. |
Contributor
|
I wasn't able to reproduce the deadlock when I tried. Likely it needs stepping with breakpoints, I've not checked how to do that. |
Contributor
iloveeclipse
force-pushed
the
issue_2460
branch
from
16c60c8 to
36cf40b
Compare
This was
linked to
issues
Feb 25, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
debug/org.eclipse.ui.console/src/org/eclipse/ui/console/IConsole.java:127
- The documentation contains a grammatical error: "is not more visible" should be "is no longer visible".
* this console is not more visible to user.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleView.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleView.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleManager.java
Show resolved
Hide resolved
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleManager.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/views/console/ProcessConsole.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/views/console/ProcessConsole.java
Show resolved
Hide resolved
- Extracted common code from ShowConsoleViewJob to AbstractConsoleJob - Renamed RepaintJob to RedrawJob, switched to use AbstractConsoleJob - Extracted anonymous UI job to WarnAboutContentChangedJob, based on AbstractConsoleJob - fWarnQueued is not needed anymore for WarnAboutContentChangedJob - This above fixes the race condition where multiple consoles changes could have triggered the warnOfContentChange(), but the job only scheduled for the first one - Moved final field inits to constructor - Fixed potential NP in RedrawJob See eclipse-platform#2477
`page.findView(IConsoleConstants.ID_CONSOLE_VIEW)` API doesn't return console views opened in same page with secondary id's (== any Console views opened next to the first one in same page). So all calls to `ConsoleManager.refresh(IConsole)` were not working for such views. In SDK, this would only affect `TextConsole.addHyperlink()` code and is visible if opening Java Stack Trace view as secondary view - it will not show hyperlinks. Worse, since view id's are persisted across sessions, such views would not work properly as long as they present in the perspective. For the fix, make use of views registered in ConsoleManager and iterate over them, similar to ShowConsoleViewJob code. See eclipse-platform#2477
Original code had multiple issues: it didn't notified about console content changes in all opened windows, it didn't notified for the console changes in all secondary console views but it notified even if the console with changes was visible. As noticed already before, `page.findView(IConsoleConstants.ID_CONSOLE_VIEW)` API doesn't return console views opened in same page with secondary id's (== any Console views opened next to the first one in same page). So WarnAboutContentChangedJob only supported Console views without secondary ids and only in the currently active window. - Notify console changes in all windows, not only in the active one - Notify also for secondary consoles - Don't notify about console changes if changed console page is visible (== top of the stack) in the current Console view and Console view is also visible. Fixes eclipse-platform#2477
- Moved final field inits to constructor - Get rid of unneeded or duplicated code, obsoleted comments - Use instanceof variables - Fix spelling mistakes - Changed all access to fActiveConsole via get/set methods
- Moved final field inits to constructor - Added trivial toString() implementation to the AbstractConsole
- Moved final field inits to constructor - Removed useless javadocs - Use instanceof variables where possible - Added "disposed" field and checks before each asyncExec() for that - Refactored computeName() to readable code -- split into multiple logical parts -- made things static which are static -- don't do anything if view is already disposed
This fixes regression from 7e1f60c, where main thread (which owns lock on UI operations) waits for the lock on ProcessConsole to update console name but never gets it because console code is blocked internally waiting for a QueueProcessingJob being executed on UI thread. The solution is to avoid direct calls from UI to ProcessConsole.resetName(). Fixes eclipse-platform#2460
- Added new API to allow console pages knew whether they are visible or not. - Use new IConsole API to schedule console updates only if console page is visible (top level) in the console view. This prevents multiple opened consoles in same console view run update tasks every second, even if no one can see the updated console name. Fixes eclipse-platform#2478
iloveeclipse
force-pushed
the
issue_2460
branch
from
36cf40b to
3d746e1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/views/console/ProcessConsole.java
Show resolved
Hide resolved
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
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.
This fixes regression from 7e1f60c, where main thread (which owns lock on UI operations) waits for the lock on
ProcessConsoleto update console name but never gets it because console code is blocked internally waiting for aQueueProcessingJobbeing executed on UI thread.The solution is to avoid direct calls from UI to
ProcessConsole.resetName().Fixes #2460