Skip to content

Destroy the terminal process off the UI thread - #2997

Merged
vogella merged 1 commit into
eclipse-platform:masterfrom
vogella:terminal-destroy-process-async
Oct 13, 2026
Merged

vogella merged 1 commit into
eclipse-platform:masterfrom
vogella:terminal-destroy-process-async

Conversation

@vogella

@vogella vogella commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Closing a terminal tab, or the workbench with terminals open, froze the UI because ProcessConnector called the CDT Spawner's destroy() on the UI thread. That method sends SIGTERM and waits up to a second before sending SIGKILL, and an interactive shell ignores SIGTERM, so every open terminal added a full second of freeze. Spawner processes are now destroyed on a background thread, keeping the existing platform-specific order relative to closing the streams, while other processes are still destroyed synchronously. When the bundle stops, it waits up to two seconds for pending destroys, so the JVM does not exit before the shells are killed.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Pending destruction can be abandoned during JVM shutdown, and cross-platform process cleanup needs verification.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Moves terminal process destruction off the calling thread to reduce UI freezes when closing tabs or the workbench.

Changes:

  • Adds a daemon-thread helper for process destruction.
  • Schedules destruction before stream cleanup on non-Windows platforms and afterward on Windows.
File Description
terminal/​bundles/​org.eclipse.terminal.connector.process/​src/​org/​eclipse/​terminal/​connector/​process/​ProcessConnector.java Moves process destruction into a background thread.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

    54 files  ±0      54 suites  ±0   1h 1m 7s ⏱️ ±0s
 4 862 tests ±0   4 840 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 477 runs  ±0  12 323 ✅ ±0  154 💤 ±0  0 ❌ ±0 

Results for commit 2bff605. ± Comparison against base commit 1965d89.

♻️ This comment has been updated with latest results.

@vogella
vogella force-pushed the terminal-destroy-process-async branch from 377e835 to 541d90b Compare October 8, 2026 12:14
Spawner.destroy() sends SIGTERM and then waits up to a second before
killing the process. An interactive shell ignores SIGTERM, so closing a
terminal tab blocked the UI thread for a full second, and closing the
workbench with several terminals open froze it for that long per tab.

Spawner processes are now destroyed on a daemon thread, other processes
still synchronously. The bundle's stop() waits up to two seconds for
pending destroys so the JVM does not exit before the shells are killed.

Assisted-by: multiple AI agents and layers of automated tooling 🤖
@vogella
vogella force-pushed the terminal-destroy-process-async branch from 541d90b to 2bff605 Compare October 11, 2026 13:48
@vogella
vogella requested a balanced review from Copilot October 11, 2026 18:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation addresses the UI blocking while retaining synchronous handling for other process types and bounded shutdown cleanup.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@vogella
vogella merged commit 73411d1 into eclipse-platform:master Oct 13, 2026
18 checks passed
@vogella
vogella deleted the terminal-destroy-process-async branch October 13, 2026 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants