-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(terminal): wait for PTY exit on Windows before recreating terminal #1184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
6777570
1067842
7eaffb2
1d28716
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,43 @@ import type { SupportedTerminal } from '../../shared/types/settings'; | |
|
|
||
| // Windows shell paths are now imported from the platform module via getWindowsShellPaths() | ||
|
|
||
| /** | ||
| * Track pending exit promises for terminals being destroyed. | ||
| * Used to wait for PTY process exit on Windows where termination is async. | ||
| */ | ||
| const pendingExitPromises = new Map<string, { | ||
| resolve: () => void; | ||
| timeoutId: NodeJS.Timeout; | ||
| }>(); | ||
|
|
||
| /** | ||
| * Default timeouts for waiting for PTY exit (in milliseconds). | ||
| * Windows needs longer timeout due to slower process termination. | ||
| */ | ||
| const PTY_EXIT_TIMEOUT_WINDOWS = 2000; | ||
| const PTY_EXIT_TIMEOUT_UNIX = 500; | ||
|
|
||
| /** | ||
| * Wait for a PTY process to exit. | ||
| * Returns a promise that resolves when the PTY's onExit event fires. | ||
| * Has a timeout fallback in case the exit event never fires. | ||
| */ | ||
| export function waitForPtyExit(terminalId: string, timeoutMs?: number): Promise<void> { | ||
| const timeout = timeoutMs ?? (isWindows() ? PTY_EXIT_TIMEOUT_WINDOWS : PTY_EXIT_TIMEOUT_UNIX); | ||
|
|
||
| return new Promise<void>((resolve) => { | ||
| // Set up timeout fallback | ||
| const timeoutId = setTimeout(() => { | ||
| debugLog('[PtyManager] PTY exit timeout for terminal:', terminalId); | ||
| pendingExitPromises.delete(terminalId); | ||
| resolve(); | ||
| }, timeout); | ||
|
|
||
| // Store the promise resolver | ||
| pendingExitPromises.set(terminalId, { resolve, timeoutId }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concurrent destroy with same ID causes wrong promise resolutionMedium Severity The Additional Locations (1) |
||
| }); | ||
| } | ||
|
Comment on lines
+40
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this function is called multiple times for the same export function waitForPtyExit(terminalId: string, timeoutMs?: number): Promise<void> {
const timeout = timeoutMs ?? (isWindows() ? PTY_EXIT_TIMEOUT_WINDOWS : PTY_EXIT_TIMEOUT_UNIX);
// If we are already waiting for this terminal, clear the old timeout to prevent leaks.
const existing = pendingExitPromises.get(terminalId);
if (existing) {
debugLog('[PtyManager] PTY exit already being awaited, overwriting for terminal:', terminalId);
clearTimeout(existing.timeoutId);
}
return new Promise<void>((resolve) => {
// Set up timeout fallback
const timeoutId = setTimeout(() => {
debugLog('[PtyManager] PTY exit timeout for terminal:', terminalId);
pendingExitPromises.delete(terminalId);
resolve();
}, timeout);
// Store the promise resolver
pendingExitPromises.set(terminalId, { resolve, timeoutId });
});
} |
||
|
|
||
| /** | ||
| * Get the Windows shell executable based on preferred terminal setting | ||
| */ | ||
|
|
@@ -115,6 +152,14 @@ export function setupPtyHandlers( | |
| ptyProcess.onExit(({ exitCode }) => { | ||
| debugLog('[PtyManager] Terminal exited:', id, 'code:', exitCode); | ||
|
|
||
| // Resolve any pending exit promise FIRST (before other cleanup) | ||
| const pendingExit = pendingExitPromises.get(id); | ||
| if (pendingExit) { | ||
| clearTimeout(pendingExit.timeoutId); | ||
| pendingExitPromises.delete(id); | ||
| pendingExit.resolve(); | ||
| } | ||
|
|
||
| const win = getWindow(); | ||
| if (win) { | ||
| win.webContents.send(IPC_CHANNELS.TERMINAL_EXIT, id, exitCode); | ||
|
|
@@ -123,7 +168,10 @@ export function setupPtyHandlers( | |
| // Call custom exit handler | ||
| onExitCallback(terminal); | ||
|
|
||
| terminals.delete(id); | ||
| // Only delete if still in map (destroyTerminal may have already removed it) | ||
| if (terminals.has(id)) { | ||
| terminals.delete(id); | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
| } | ||
|
|
||
|
|
@@ -228,9 +276,17 @@ export function resizePty(terminal: TerminalProcess, cols: number, rows: number) | |
| } | ||
|
|
||
| /** | ||
| * Kill a PTY process | ||
| * Kill a PTY process. | ||
| * @param terminal The terminal process to kill | ||
| * @param waitForExit If true, returns a promise that resolves when the PTY exits. | ||
| * Used on Windows where PTY termination is async. | ||
| */ | ||
| export function killPty(terminal: TerminalProcess): void { | ||
| export function killPty(terminal: TerminalProcess, waitForExit?: boolean): Promise<void> | void { | ||
| if (waitForExit) { | ||
| const exitPromise = waitForPtyExit(terminal.id); | ||
| terminal.pty.kill(); | ||
| return exitPromise; | ||
| } | ||
| terminal.pty.kill(); | ||
| } | ||
|
|
||
|
|
||
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.