From 37af0fa80870be63b528c6a36681c4ed39ca3451 Mon Sep 17 00:00:00 2001 From: Max Sherman Date: Tue, 23 Aug 2016 12:18:17 -0700 Subject: [PATCH] Debugger Eval #2 -- Refactor WES logic out of the Bridge Summary: Prior to this diff, the Bridge handled a lot of evaluation logic. This is sub-optimal for two reasons: 1. It overloads the Bridge, whose purpose is to just be a forwarding layer to/from chrome's model/the backend. 2. It makes following the lifetime of an evaluation request complicated, because you have to understand how the WES and Bridge interoperate. This diff pulls the logic out of the Bridge entirely, and puts it into the WES. When the Bridge gets evaluation responses from the backend, it uses flux actions to communicate this with the WES. Reviewed By: yinghuitan Differential Revision: D3699243 fbshipit-source-id: 10bd479e46dc58dced9c384888476cdda416dbf8 --- pkg/nuclide-debugger/lib/Bridge.js | 146 ++----------- pkg/nuclide-debugger/lib/Constants.js | 2 + pkg/nuclide-debugger/lib/DebuggerActions.js | 22 ++ .../lib/WatchExpressionStore.js | 192 +++++++++++++----- pkg/nuclide-debugger/lib/types.js | 13 ++ 5 files changed, 194 insertions(+), 181 deletions(-) diff --git a/pkg/nuclide-debugger/lib/Bridge.js b/pkg/nuclide-debugger/lib/Bridge.js index 8474b95e97..32fc144d69 100644 --- a/pkg/nuclide-debugger/lib/Bridge.js +++ b/pkg/nuclide-debugger/lib/Bridge.js @@ -12,33 +12,18 @@ import type DebuggerModel from './DebuggerModel'; import type { Callstack, - EvaluationResult, + EvalCommand, ExpansionResult, - ObjectGroup, NuclideThreadData, BreakpointUserChangeArgType, IPCBreakpoint, + ExpressionResult, + GetPropertiesResult, } from './types'; -type ExpressionResult = ChromeProtocolResponse & { - expression: string, -}; - -type GetPropertiesResult = ChromeProtocolResponse & { - objectId: string, -}; - -type ChromeProtocolResponse = { - result: ?EvaluationResult | ?GetPropertiesResult, - error: ?Object, -}; - import {CompositeDisposable, Disposable} from 'atom'; -import {getLogger} from '../../nuclide-logging'; import nuclideUri from '../../commons-node/nuclideUri'; -import {Deferred} from '../../commons-node/promise'; import {DebuggerMode} from './DebuggerStore'; -import {normalizeRemoteObjectValue} from './normalizeRemoteObjectValue'; const INJECTED_CSS = [ /* Force the inspector to scroll vertically on Atom ≥ 1.4.0 */ @@ -82,22 +67,15 @@ class Bridge { _cleanupDisposables: CompositeDisposable; _webview: ?WebviewElement; _suppressBreakpointSync: boolean; - // Tracks requests for expression evaluation, keyed by the expression body. - _expressionsInFlight: Map>; - _getPropertiesRequestsInFlight: Map>; - _evaluationId: number; constructor(debuggerModel: DebuggerModel) { this._debuggerModel = debuggerModel; this._cleanupDisposables = new CompositeDisposable(); this._webview = null; this._suppressBreakpointSync = false; - this._evaluationId = 0; this._disposables = new CompositeDisposable( debuggerModel.getBreakpointStore().onUserChange(this._handleUserBreakpointChange.bind(this)), ); - this._expressionsInFlight = new Map(); - this._getPropertiesRequestsInFlight = new Map(); } setWebviewElement(webview: WebviewElement) { @@ -149,26 +127,6 @@ class Bridge { } } - getProperties(objectId: string): Promise { - return this._sendEvaluationCommand( - this._getPropertiesRequestsInFlight, - 'getProperties', - objectId, - ); - } - - evaluateWatchExpression(expression: string): Promise { - return this._evaluateOnSelectedCallFrame(expression, 'watch-group'); - } - - evaluateConsoleExpression(expression: string): Promise { - if (this._debuggerModel.getStore().getDebuggerMode() === 'paused') { - return this._evaluateOnSelectedCallFrame(expression, 'console'); - } else { - return this._runtimeEvaluate(expression); - } - } - triggerAction(actionId: string): void { if (this._webview) { this._webview.send( @@ -219,79 +177,22 @@ class Bridge { } } - async _evaluateOnSelectedCallFrame( - expression: string, - objectGroup: ObjectGroup, - ): Promise { - const result = await this._sendEvaluationCommand( - this._expressionsInFlight, - 'evaluateOnSelectedCallFrame', - expression, - objectGroup, - ); - if (result == null) { - // TODO: It would be nice to expose a better error from the backend here. - return { - type: 'text', - value: `Failed to evaluate: ${expression}`, - }; - } else { - return result; - } - } - - async _runtimeEvaluate(expression: string): Promise { - const result = await this._sendEvaluationCommand( - this._expressionsInFlight, - 'runtimeEvaluate', - expression, - ); - if (result == null) { - // TODO: It would be nice to expose a better error from the backend here. - return { - type: 'text', - value: `Failed to evaluate: ${expression}`, - }; - } else { - return result; - } - } - - async _sendEvaluationCommand( - pending: Map>, - command: string, + sendEvaluationCommand( + command: EvalCommand, + evalId: number, ...args: Array - ): Promise { - const webview = this._webview; - if (webview == null) { - return null; - } - const deferred = new Deferred(); - const evalId = this._evaluationId; - ++this._evaluationId; - pending.set(evalId, deferred); - webview.send('command', command, evalId, ...args); - let result = null; - try { - result = await deferred.promise; - } catch (e) { - getLogger().warn(`${command}: Error getting result.`, e); + ): void { + if (this._webview != null) { + this._webview.send('command', command, evalId, ...args); } - pending.delete(evalId); - return result; } - _handleExpressionEvaluationResponse(response: ExpressionResult): void { - response.result = normalizeRemoteObjectValue(response.result); - this._handleResponseForPendingRequest(response.id, this._expressionsInFlight, response); + _handleExpressionEvaluationResponse(response: ExpressionResult & {id: number}): void { + this._debuggerModel.getActions().receiveExpressionEvaluationResponse(response.id, response); } - _handleGetPropertiesResponse(response: GetPropertiesResult): void { - this._handleResponseForPendingRequest( - response.id, - this._getPropertiesRequestsInFlight, - response, - ); + _handleGetPropertiesResponse(response: GetPropertiesResult & {id: number}): void { + this._debuggerModel.getActions().receiveGetPropertiesResponse(response.id, response); } _handleCallstackUpdate(callstack: Callstack): void { @@ -302,27 +203,6 @@ class Bridge { this._debuggerModel.getActions().updateLocals(locals); } - _handleResponseForPendingRequest( - id: number, - pending: Map>, - response: ChromeProtocolResponse, - ): void { - const { - result, - error, - } = response; - const deferred = pending.get(id); - if (deferred == null) { - // Nobody is listening for the result of this expression. - return; - } - if (error != null) { - deferred.reject(error); - } else { - deferred.resolve(((result : any) : T)); - } - } - _handleIpcMessage(stdEvent: Event): void { // addEventListener expects its callback to take an Event. I'm not sure how to reconcile it with // the type that is expected here. diff --git a/pkg/nuclide-debugger/lib/Constants.js b/pkg/nuclide-debugger/lib/Constants.js index 0903ff804d..c110fb0a0c 100644 --- a/pkg/nuclide-debugger/lib/Constants.js +++ b/pkg/nuclide-debugger/lib/Constants.js @@ -43,6 +43,8 @@ const Actions = Object.freeze({ UPDATE_THREADS: 'UPDATE_THREADS', NOTIFY_THREAD_SWITCH: 'NOTIFY_THREAD_SWITCH', TOGGLE_SINGLE_THREAD_STEPPING: 'TOGGLE_SINGLE_THREAD_STEPPING', + RECEIVED_EXPRESSION_EVALUATION_RESPONSE: 'RECEIVED_EXPRESSION_EVALUATION_RESPONSE', + RECEIVED_GET_PROPERTIES_RESPONSE: 'RECEIVED_GET_PROPERTIES_RESPONSE', }); module.exports = { diff --git a/pkg/nuclide-debugger/lib/DebuggerActions.js b/pkg/nuclide-debugger/lib/DebuggerActions.js index 06e6f97978..26b47a3c68 100644 --- a/pkg/nuclide-debugger/lib/DebuggerActions.js +++ b/pkg/nuclide-debugger/lib/DebuggerActions.js @@ -24,6 +24,8 @@ import type { Callstack, DebuggerModeType, ExpansionResult, + ExpressionResult, + GetPropertiesResult, NuclideThreadData, } from './types'; @@ -464,6 +466,26 @@ class DebuggerActions { }); } + receiveExpressionEvaluationResponse(id: number, response: ExpressionResult): void { + this._dispatcher.dispatch({ + actionType: Constants.Actions.RECEIVED_EXPRESSION_EVALUATION_RESPONSE, + data: { + id, + response, + }, + }); + } + + receiveGetPropertiesResponse(id: number, response: GetPropertiesResult): void { + this._dispatcher.dispatch({ + actionType: Constants.Actions.RECEIVED_GET_PROPERTIES_RESPONSE, + data: { + id, + response, + }, + }); + } + _handleDebugModeStart(): void { // Open the console window if it's not already opened. atom.commands.dispatch( diff --git a/pkg/nuclide-debugger/lib/WatchExpressionStore.js b/pkg/nuclide-debugger/lib/WatchExpressionStore.js index 53c72eb503..0902464761 100644 --- a/pkg/nuclide-debugger/lib/WatchExpressionStore.js +++ b/pkg/nuclide-debugger/lib/WatchExpressionStore.js @@ -11,8 +11,11 @@ import type Bridge from './Bridge'; import type { + ChromeProtocolResponse, + EvalCommand, EvaluationResult, ExpansionResult, + ObjectGroup, } from './types'; import type Dispatcher from 'flux'; @@ -25,6 +28,9 @@ import {Actions} from './Constants'; import {BehaviorSubject, Observable} from 'rxjs'; import invariant from 'assert'; import UniversalDisposable from '../../commons-node/UniversalDisposable'; +import {Deferred} from '../../commons-node/promise'; +import {getLogger} from '../../nuclide-logging'; +import {normalizeRemoteObjectValue} from './normalizeRemoteObjectValue'; type Expression = string; @@ -33,11 +39,17 @@ export class WatchExpressionStore { _disposables: CompositeDisposable; _watchExpressions: Map>; _previousEvaluationSubscriptions: CompositeDisposable; + _evaluationId: number; + _isPaused: boolean; + _evaluationRequestsInFlight: Map>; constructor(dispatcher: Dispatcher, bridge: Bridge) { + this._evaluationId = 0; + this._isPaused = false; this._bridge = bridge; this._disposables = new CompositeDisposable(); this._watchExpressions = new Map(); + this._evaluationRequestsInFlight = new Map(); this._disposables.add(new Disposable(() => this._watchExpressions.clear())); // `this._previousEvaluationSubscriptions` can change at any time and are a distinct subset of // `this._disposables`. @@ -53,52 +65,59 @@ export class WatchExpressionStore { _handlePayload(payload: Object) { switch (payload.actionType) { - case Actions.CLEAR_INTERFACE: + case Actions.CLEAR_INTERFACE: { this._clearEvaluationValues(); break; - case Actions.DEBUGGER_MODE_CHANGE: + } + case Actions.DEBUGGER_MODE_CHANGE: { + this._isPaused = false; if (payload.data === DebuggerMode.PAUSED) { + this._isPaused = true; this._triggerReevaluation(); } else if (payload.data === DebuggerMode.STOPPED) { this._cancelRequestsToBridge(); this._clearEvaluationValues(); } break; - default: + } + case Actions.RECEIVED_GET_PROPERTIES_RESPONSE: { + const {id, response} = payload.data; + this._handleResponseForPendingRequest(id, response); + break; + } + case Actions.RECEIVED_EXPRESSION_EVALUATION_RESPONSE: { + const {id, response} = payload.data; + response.result = normalizeRemoteObjectValue(response.result); + this._handleResponseForPendingRequest(id, response); + break; + } + default: { return; + } } } - _requestActionFromBridge( - subject: BehaviorSubject, - callback: () => Promise, - ): IDisposable { - return new UniversalDisposable( - Observable - .fromPromise(callback()) - .merge(Observable.never()) - .subscribe(subject), - ); + _triggerReevaluation(): void { + this._cancelRequestsToBridge(); + for (const [expression, subject] of this._watchExpressions) { + if (subject.observers == null || subject.observers.length === 0) { + // Nobody is watching this expression anymore. + this._watchExpressions.delete(expression); + continue; + } + this._requestExpressionEvaluation(expression, subject, false /* no REPL support */); + } } - _requestExpressionEvaluation( - expression: Expression, - subject: BehaviorSubject, - supportRepl: boolean, - ): void { - const evaluationDisposable = this._requestActionFromBridge( - subject, - () => ( - supportRepl - ? this._bridge.evaluateConsoleExpression(expression) - : this._bridge.evaluateWatchExpression(expression) - ), - ); - // Non-REPL environments will want to record these requests so they can be canceled on - // re-evaluation, e.g. in the case of stepping. REPL environments should let them complete so - // we can have e.g. a history of evaluations in the console. - if (!supportRepl) { - this._previousEvaluationSubscriptions.add(evaluationDisposable); + _cancelRequestsToBridge(): void { + this._previousEvaluationSubscriptions.dispose(); + this._previousEvaluationSubscriptions = new CompositeDisposable(); + } + + // Resets all values to N/A, for examples when the debugger resumes or stops. + _clearEvaluationValues(): void { + for (const subject of this._watchExpressions.values()) { + subject.next(null); } } @@ -107,7 +126,11 @@ export class WatchExpressionStore { * Resources are automatically cleaned up once all subscribers of an expression have unsubscribed. */ getProperties(objectId: string): Observable { - return Observable.fromPromise(this._bridge.getProperties(objectId)); + const getPropertiesPromise: Promise = this._sendEvaluationCommand( + 'getProperties', + objectId, + ); + return Observable.fromPromise(getPropertiesPromise); } evaluateConsoleExpression(expression: Expression): Observable { @@ -142,27 +165,100 @@ export class WatchExpressionStore { return subject.asObservable(); } - _triggerReevaluation(): void { - this._cancelRequestsToBridge(); - for (const [expression, subject] of this._watchExpressions) { - if (subject.observers == null || subject.observers.length === 0) { - // Nobody is watching this expression anymore. - this._watchExpressions.delete(expression); - continue; - } - this._requestExpressionEvaluation(expression, subject, false /* no REPL support */); + _requestExpressionEvaluation( + expression: Expression, + subject: BehaviorSubject, + supportRepl: boolean, + ): void { + let evaluationPromise; + if (supportRepl) { + evaluationPromise = this._isPaused + ? this._evaluateOnSelectedCallFrame(expression, 'console') + : this._runtimeEvaluate(expression); + } else { + evaluationPromise = this._evaluateOnSelectedCallFrame(expression, 'watch-group'); + } + + const evaluationDisposable = new UniversalDisposable( + Observable + .fromPromise(evaluationPromise) + .merge(Observable.never()) // So that we do not unsubscribe `subject` when disposed. + .subscribe(subject), + ); + + // Non-REPL environments will want to record these requests so they can be canceled on + // re-evaluation, e.g. in the case of stepping. REPL environments should let them complete so + // we can have e.g. a history of evaluations in the console. + if (!supportRepl) { + this._previousEvaluationSubscriptions.add(evaluationDisposable); + } else { + this._disposables.add(evaluationDisposable); } } - _cancelRequestsToBridge(): void { - this._previousEvaluationSubscriptions.dispose(); - this._previousEvaluationSubscriptions = new CompositeDisposable(); + async _evaluateOnSelectedCallFrame( + expression: string, + objectGroup: ObjectGroup, + ): Promise { + const result: ?EvaluationResult = await this._sendEvaluationCommand( + 'evaluateOnSelectedCallFrame', + expression, + objectGroup, + ); + if (result == null) { + // TODO: It would be nice to expose a better error from the backend here. + return { + type: 'text', + value: `Failed to evaluate: ${expression}`, + }; + } else { + return result; + } } - // Resets all values to N/A, for examples when the debugger resumes or stops. - _clearEvaluationValues(): void { - for (const subject of this._watchExpressions.values()) { - subject.next(null); + async _runtimeEvaluate(expression: string): Promise { + const result: ?EvaluationResult = await this._sendEvaluationCommand( + 'runtimeEvaluate', + expression, + ); + if (result == null) { + // TODO: It would be nice to expose a better error from the backend here. + return { + type: 'text', + value: `Failed to evaluate: ${expression}`, + }; + } else { + return result; + } + } + + async _sendEvaluationCommand(command: EvalCommand, ...args: Array): Promise { + const deferred = new Deferred(); + const evalId = this._evaluationId; + ++this._evaluationId; + this._evaluationRequestsInFlight.set(evalId, deferred); + this._bridge.sendEvaluationCommand(command, evalId, ...args); + let result = null; + try { + result = await deferred.promise; + } catch (e) { + getLogger().warn(`${command}: Error getting result.`, e); + } + this._evaluationRequestsInFlight.delete(evalId); + return result; + } + + _handleResponseForPendingRequest(id: number, response: ChromeProtocolResponse): void { + const {result, error} = response; + const deferred = this._evaluationRequestsInFlight.get(id); + if (deferred == null) { + // Nobody is listening for the result of this expression. + return; + } + if (error != null) { + deferred.reject(error); + } else { + deferred.resolve(result); } } diff --git a/pkg/nuclide-debugger/lib/types.js b/pkg/nuclide-debugger/lib/types.js index 5ec74d0089..52813f5eb0 100644 --- a/pkg/nuclide-debugger/lib/types.js +++ b/pkg/nuclide-debugger/lib/types.js @@ -33,6 +33,19 @@ export type EvaluatedExpression = { value: Observable, }; export type EvaluatedExpressionList = Array; +export type EvalCommand = 'runtimeEvaluate' | 'evaluateOnSelectedCallFrame' | 'getProperties'; +export type ExpressionResult = ChromeProtocolResponse & { + expression: string, +}; + +export type GetPropertiesResult = ChromeProtocolResponse & { + objectId: string, +}; + +export type ChromeProtocolResponse = { + result: ?EvaluationResult | ?GetPropertiesResult, + error: ?Object, +}; /* Breakpoints */ export type FileLineBreakpoint = {