Replace @debounce with disposable RunOnceScheduler in TerminalResizeDebouncer#318177
Draft
bryanchen-d wants to merge 1 commit into
Draft
Replace @debounce with disposable RunOnceScheduler in TerminalResizeDebouncer#318177bryanchen-d wants to merge 1 commit into
bryanchen-d wants to merge 1 commit into
Conversation
…ebouncer The @debounce decorator schedules a bare setTimeout stored as an instance field; the timer is not registered with the disposable store, so the callback can fire after the terminal/xterm is disposed. PR #314795 worked around this with an isDisposed early-return inside _debounceResizeX, but each new debounced callsite would need the same defensive guard. Replace the decorator with a RunOnceScheduler registered on the disposable store. The pending timer is now cancelled deterministically when the TerminalResizeDebouncer is disposed, so no post-dispose callbacks can run and no per-method guard is required. Also cancel the pending scheduler in the immediate-resize branch and in flush() so a queued debounced X-resize cannot overwrite a fresh immediate resize. Refs #314795.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the @debounce(100)-based horizontal resize debounce in TerminalResizeDebouncer with a RunOnceScheduler that’s owned by the class’ disposable store, ensuring any pending timer is deterministically cancelled on dispose and avoiding post-dispose resize callbacks into torn-down xterm state.
Changes:
- Replaces the
@debounce(100)decorator with a_debounceResizeXScheduler: RunOnceSchedulerregistered viathis._register(...). - Updates call sites to schedule/cancel the scheduler (including cancelling in the immediate-resize path and in
flush()). - Removes the
debounceimport and theisDisposedguard inside the previously-decorated method.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/terminalResizeDebouncer.ts | Moves horizontal resize debouncing to a disposable RunOnceScheduler and cancels pending work during flush/immediate resize to avoid post-dispose callbacks. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
Comment on lines
85
to
+87
| this._resizeYCallback(rows); | ||
| this._latestX = cols; | ||
| this._debounceResizeX(cols); | ||
| this._debounceResizeXScheduler.schedule(); |
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.
What
Replace the
@debounce(100)decorator on_debounceResizeXwith aRunOnceSchedulerowned by the disposable store.Why
The
@debouncedecorator schedules a baresetTimeoutand stashes the handle as an instance field ($debounce$_debounceResizeX). The timer is not registered with anyDisposableStore, sosuper.dispose()does not cancel it. The callback can fire after the terminal/xterm has been torn down and crash inside xterm.js'RenderService.get dimensions()— telemetry bucketsaaa283a2,4826565b,1e83a096.PR #314795 worked around this with an
if (this._store.isDisposed) return;guard inside_debounceResizeX. That closes the buckets but:_resizethree weeks later for the same class of race).DisposableStore(RunOnceScheduler,ThrottledDelayer,disposableTimeout, …) and are cancelled deterministically at dispose.As Daniel noted on #314795:
Changes
terminalResizeDebouncer.ts:@debounce(100) _debounceResizeX(cols)with aRunOnceSchedulerregistered viathis._register(...). The scheduler readsthis._latestXlazily (already updated before each schedule).this._debounceResizeX(cols)withthis._debounceResizeXScheduler.schedule().flush()so a queued X-debounce cannot fire after a fresh full resize.import { debounce }and the per-callsiteisDisposedguard inside the debounced method.After this change, disposing the
TerminalResizeDebouncercancels the pending timer through the normal disposable chain — no post-dispose callback can run, so no guard is needed.How to test
Ctrl+Shift+W) a terminal while resizing the window.terminal.integrated.fontFamilyimmediately after killing a terminal.exitin a long-running shell while the panel is mid-resize.Cannot read properties of undefined (reading 'dimensions')in DevTools console.4826565b(the_debounceResizeXpath) should stop firing.Risk
RunOnceScheduler.schedule()cancels and re-schedules a 100ms timer just like the decorator did, and reads the latestcolsfrom_latestXwhich the caller updates before calling.flush()and on immediate resize, so we cannot land a stale X-resize after a full resize — this was implicit before becauseclearTimeoutdid the same on the decorator's stored handle.Refs #314795, #303546.