Skip to content

Commit

Permalink
fix: possible bad state when resuming multiple times with a slow client
Browse files Browse the repository at this point in the history 
Some requests can make the client application run slow. If the user
tries to resume multiple times when paused and "stuck", it's very easy
to get into a bad state.

With this change, coalesce multiple requests of the same type together
so that clicking resume while stalled doesn't tear the state.

I couldn't really get a consistent test case for this since it's
timing based :/

Found while investigating #1719
  • Loading branch information
@connor4312
connor4312 committed Jun 9, 2023
1 parent 2fbe263 commit f3ac417
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 47 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he
- fix: breakpoints not hitting early on in nested sourcemapped programs ([#1704](https://github.com/microsoft/vscode-js-debug/issues/1704))
- fix: increase smart step backout threshold for better stepping ([#1700](https://github.com/microsoft/vscode-js-debug/issues/1700))
- fix: Blazor sources sometimes being missing ([dotnet/runtime#86754](https://github.com/dotnet/runtime/issues/86754))
- fix: possible bad state when resuming multiple times with a slow client

## v1.78 (April 2024)

Expand Down
132 changes: 85 additions & 47 deletions src/adapter/threads.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { HrTime } from '../common/hrnow';
import { ILogger, LogTag } from '../common/logging';
import { isInstanceOf, truthy } from '../common/objUtils';
import { Base1Position } from '../common/positions';
import { delay, getDeferred, IDeferred } from '../common/promiseUtil';
import { IDeferred, delay, getDeferred } from '../common/promiseUtil';
import { IRenameProvider } from '../common/sourceMaps/renameProvider';
import * as sourceUtils from '../common/sourceUtils';
import { StackTraceParser } from '../common/stackTraceParser';
Expand All @@ -33,16 +33,16 @@ import { CustomBreakpointId, customBreakpoints } from './customBreakpoints';
import { IEvaluator } from './evaluator';
import { IExceptionPauseService } from './exceptionPauseService';
import * as objectPreview from './objectPreview';
import { getContextForType, PreviewContextType } from './objectPreview/contexts';
import { PreviewContextType, getContextForType } from './objectPreview/contexts';
import { SmartStepper } from './smartStepping';
import {
base1To0,
IPreferredUiLocation,
ISourceWithMap,
IUiLocation,
rawToUiOffset,
Source,
SourceContainer,
base1To0,
rawToUiOffset,
} from './sources';
import { StackFrame, StackTrace } from './stackTrace';
import {
Expand Down Expand Up @@ -164,6 +164,33 @@ interface IInstrumentationPauseAuxData {
sourceMapURL: string;
}

/**
* Queue used to avoid getting into a bad state if the user runs multiple
* state-changing commands concurrently. Some things can cause V8 to block
* and take a while responding to requests. Mutliple requests of the same
* operation type get coalesced, while other operation types are
* run sequentially.
*/
class StateQueue {
private queue?: { operation: string; result: Promise<unknown> };

public enqueue<T>(operation: string, fn: () => Promise<T>) {
if (!this.queue || this.queue.operation !== operation) {
const promise = this.queue?.result.then(fn, fn) ?? fn();
const queued = (this.queue = {
operation,
result: promise.finally(() => {
if (this.queue === queued) {
this.queue = undefined;
}
}),
});
}

return this.queue.result as Promise<T>;
}
}

export class Thread implements IVariableStoreLocationProvider {
private static _lastThreadId = 0;
public readonly id: number;
Expand All @@ -181,6 +208,7 @@ export class Thread implements IVariableStoreLocationProvider {
private _sourceMapDisabler?: SourceMapDisabler;
private _expectedPauseReason?: ExpectedPauseReason;
private _excludedCallers: readonly Dap.ExcludedCaller[] = [];
private readonly stateQueue = new StateQueue();
private readonly _onPausedEmitter = new EventEmitter<IPausedDetails>();
private readonly _dap: DeferredContainer<Dap.Api>;
private disposed = false;
Expand Down Expand Up @@ -252,66 +280,76 @@ export class Thread implements IVariableStoreLocationProvider {
}
}

public async resume(): Promise<Dap.ContinueResult | Dap.Error> {
this._sourceContainer.clearDisabledSourceMaps();
if (!(await this._cdp.Debugger.resume({}))) {
// We don't report the failure if the target wasn't paused. VS relies on this behavior.
if (this._pausedDetails !== undefined) {
return errors.createSilentError(l10n.t('Unable to resume'));
public resume(): Promise<Dap.ContinueResult | Dap.Error> {
return this.stateQueue.enqueue('resume', async () => {
this._sourceContainer.clearDisabledSourceMaps();
if (!(await this._cdp.Debugger.resume({}))) {
// We don't report the failure if the target wasn't paused. VS relies on this behavior.
if (this._pausedDetails !== undefined) {
return errors.createSilentError(l10n.t('Unable to resume'));
}
}
}
return { allThreadsContinued: false };
return { allThreadsContinued: false };
});
}

public async pause(): Promise<Dap.PauseResult | Dap.Error> {
this._expectedPauseReason = { reason: 'pause' };
public pause(): Promise<Dap.PauseResult | Dap.Error> {
return this.stateQueue.enqueue('pause', async () => {
this._expectedPauseReason = { reason: 'pause' };

if (await this._cdp.Debugger.pause({})) {
return {};
}
if (await this._cdp.Debugger.pause({})) {
return {};
}

return errors.createSilentError(l10n.t('Unable to pause'));
return errors.createSilentError(l10n.t('Unable to pause'));
});
}

async stepOver(): Promise<Dap.NextResult | Dap.Error> {
this._expectedPauseReason = { reason: 'step', direction: StepDirection.Over };
if (await this._cdp.Debugger.stepOver({})) {
return {};
}
public stepOver(): Promise<Dap.NextResult | Dap.Error> {
return this.stateQueue.enqueue('stepOver', async () => {
this._expectedPauseReason = { reason: 'step', direction: StepDirection.Over };
if (await this._cdp.Debugger.stepOver({})) {
return {};
}

return errors.createSilentError(l10n.t('Unable to step next'));
return errors.createSilentError(l10n.t('Unable to step next'));
});
}

async stepInto(targetId?: number): Promise<Dap.StepInResult | Dap.Error> {
this._waitingForStepIn = { lastDetails: this._pausedDetails };
this._expectedPauseReason = { reason: 'step', direction: StepDirection.In };
public stepInto(targetId?: number): Promise<Dap.StepInResult | Dap.Error> {
return this.stateQueue.enqueue('stepInto', async () => {
this._waitingForStepIn = { lastDetails: this._pausedDetails };
this._expectedPauseReason = { reason: 'step', direction: StepDirection.In };

const stepInTarget = this._pausedDetails?.stepInTargets?.[targetId as number];
if (stepInTarget) {
const breakpoint = await this._cdp.Debugger.setBreakpoint({
location: stepInTarget.breakLocation,
});
this._waitingForStepIn.intoTargetBreakpoint = breakpoint?.breakpointId;
if (await this._cdp.Debugger.resume({})) {
return {};
}
} else {
if (await this._cdp.Debugger.stepInto({ breakOnAsyncCall: true })) {
return {};
const stepInTarget = this._pausedDetails?.stepInTargets?.[targetId as number];
if (stepInTarget) {
const breakpoint = await this._cdp.Debugger.setBreakpoint({
location: stepInTarget.breakLocation,
});
this._waitingForStepIn.intoTargetBreakpoint = breakpoint?.breakpointId;
if (await this._cdp.Debugger.resume({})) {
return {};
}
} else {
if (await this._cdp.Debugger.stepInto({ breakOnAsyncCall: true })) {
return {};
}
}
}

return errors.createSilentError(l10n.t('Unable to step in'));
return errors.createSilentError(l10n.t('Unable to step in'));
});
}

async stepOut(): Promise<Dap.StepOutResult | Dap.Error> {
this._expectedPauseReason = { reason: 'step', direction: StepDirection.Out };
public stepOut(): Promise<Dap.StepOutResult | Dap.Error> {
return this.stateQueue.enqueue('stepOut', async () => {
this._expectedPauseReason = { reason: 'step', direction: StepDirection.Out };

if (await this._cdp.Debugger.stepOut({})) {
return {};
}
if (await this._cdp.Debugger.stepOut({})) {
return {};
}

return errors.createSilentError(l10n.t('Unable to step out'));
return errors.createSilentError(l10n.t('Unable to step out'));
});
}

_stackFrameNotFoundError(): Dap.Error {
Expand Down

0 comments on commit f3ac417

Please sign in to comment.