Added resume and suspend actions #1971

JPinkney · 2018-05-25T21:55:46Z

Signed-off-by: jpinkney josh.pinkney@mail.utoronto.ca

This PR adds the resume and suspend actions for DAP.

tolusha · 2018-05-29T07:08:44Z

packages/debug/src/browser/debug-command.ts


 export namespace DebugSessionContextMenu {
    export const STOP = [...DEBUG_SESSION_CONTEXT_MENU, '1_stop'];
+    export const RESUME_ALL_THREADS = [...DEBUG_SESSION_CONTEXT_MENU, '3_resume_all_threads'];


These actions should be represented by buttons.

Where should the buttons be displayed? Still here? Or just registered to the top debug menu? I figured the context menu should be a good place to have these because it covers the entire session.

Top debug menu will be ok for now.

tolusha · 2018-05-29T07:09:26Z

packages/debug/src/browser/debug-command.ts

+
+    export const SUSPEND_ALL_THREADS = {
+        id: 'debug.thread.suspend.all',
+        label: 'Suspend all threads'


Just Suspend

tolusha · 2018-05-29T07:09:39Z

packages/debug/src/browser/debug-command.ts

+
+    export const RESUME_ALL_THREADS = {
+        id: 'debug.thread.resume.all',
+        label: 'Resume all threads'


Just Resume

tolusha · 2018-05-29T07:10:20Z

packages/debug/src/browser/debug-command.ts

@@ -60,6 +90,8 @@ export class DebugCommandHandlers implements MenuContribution, CommandContributi
    protected readonly debugSessionManager: DebugSessionManager;
    @inject(DebugConfigurationManager)
    protected readonly debugConfigurationManager: DebugConfigurationManager;
+    @inject(DebugThreadSelectionService)
+    protected readonly selectionService: DebugThreadSelectionService;


We have to get rid of DebugThreadSelectionService

tolusha · 2018-05-29T07:13:21Z

packages/debug/src/browser/debug-command.ts

+            execute: () => {
+                const debugSession = this.debugSessionManager.getActiveDebugSession();
+                if (debugSession) {
+                    debugSession.threads().then(response => {


I would move these code to DebugSession in function suspend(threadId?: number)
The same for resume command.;

tolusha · 2018-05-29T07:13:50Z

packages/debug/src/browser/debug-command.ts

+                    debugSession.threads().then(response => {
+                        const threads: DebugProtocol.Thread[] = response.body.threads;
+                        // tslint:disable-next-line:forin
+                        for (const thread in threads) {


It is better to use
for (const thread of threads)

tolusha · 2018-06-01T08:39:32Z

packages/debug/src/browser/debug-session.ts

+                return Promise.all(requests);
+            });
+        }
+        this.emit('changeStatus', threadId, false);


StoppedEvent is fired when thread is stopped. You don't have emit changeStatus event.

Do you mean I should resolve the promise and then emit the changeStatus event or that I should emit changeStatus event here?

Don't emit any events here.
Just rely on DebugState https://github.com/theia-ide/theia/blob/vscode-debug-protocol/packages/debug/src/browser/debug-session.ts#L27

Let me know if you need something else.

tolusha · 2018-06-05T13:02:48Z

packages/debug/src/browser/debug-session.ts

+                const requests = [];
+                for (const thread of threads) {
+                    const currentThreadID = thread.id;
+                    const pauseResponse = this.proceedRequest("pause", { currentThreadID });


currentThreadID -> threadId: thread.id

What happened if thread already stopped?

tolusha · 2018-06-05T13:05:55Z

packages/debug/src/browser/debug-session.ts

+                const requests = [];
+                for (const thread of threads) {
+                    const currentThreadID = thread.id;
+                    const continueResponse = this.proceedRequest("continue", { currentThreadID }) as Promise<DebugProtocol.ContinueResponse>;


currentThreadID -> threadId: thread.id

tolusha · 2018-06-05T13:08:46Z

packages/debug/src/browser/view/debug-threads-widget.ts

@@ -23,10 +24,13 @@ import { injectable, inject } from "inversify";
 export class DebugThreadsWidget extends VirtualWidget {
    private _threads: DebugProtocol.Thread[] = [];
    private _threadId?: number;
+    private _threadStatus: Map<number, boolean> = new Map();


Get rid of _threadStatus field. Rely on debugSession.debugSessionState

My only problems is that inside of the onContinuedEvent it assumes that to remove the thread you need to have the event but the debug adapter protocol suggests that the continued event is optional after a continue:

/** Event message for 'continued' event type. The event indicates that the execution of the debuggee has continued. Please note: a debug adapter is not expected to send this event in response to a request that implies that execution continues, e.g. 'launch' or 'continue'. It is only necessary to send a ContinuedEvent if there was no previous request that implied this. */

If the onContinuedEvent state is not triggered then we won't accurately know the state of all the threads

I see.
Let's emit continued event ourselves after continue response. In this case DebugSessionState will be consistent.

const event: DebugProtocol.ContinuedEvent = { type: 'event', seq: -1, event: 'continued', body: { threadId, allThreadsContinued: false } }; this.proceedEvent(event);

tolusha · 2018-06-05T13:09:56Z

packages/debug/src/browser/view/debug-threads-widget.ts

+                    }
+                }
+            },
+            isEnabled: () => !this.debugSession.debugSessionState.allThreadsContinued,


Should be enabled if at least one thread is stopped

tolusha · 2018-06-05T13:10:25Z

packages/debug/src/browser/view/debug-threads-widget.ts

+                    }
+                }
+            },
+            isEnabled: () => !this.debugSession.debugSessionState.allThreadsStopped,


Should be enabled if at least one thread is run

tolusha · 2018-06-06T06:27:44Z

packages/debug/src/browser/debug-session.ts

        return this.proceedRequest("pause", { threadId });
    }

+    resume(threadId?: number): Promise<DebugProtocol.ContinueResponse> | Promise<DebugProtocol.ContinueResponse[]> {


It is better not to duplicate the code.
Let's do it in a simpler way:

resume(threadId?: number): Promise<DebugProtocol.ContinueResponse | DebugProtocol.ContinueResponse[]> { if (threadId) { return this.doResume(threadId); } else { return this.threads().then(response => Promise.all(response.body.threads.map(thread => this.doResume(thread.id)))); } } private doResume(threadId: number): Promise<DebugProtocol.ContinueResponse> { return this.proceedRequest("continue", { threadId: threadId }).then(response => { const event: DebugProtocol.ContinuedEvent = { type: 'event', seq: -1, event: 'continued', body: { threadId: threadId, allThreadsContinued: false } }; this.proceedEvent(event); return response as DebugProtocol.ContinueResponse; }); }

tolusha · 2018-06-06T06:53:59Z

packages/debug/src/browser/debug-session.ts

@@ -124,10 +125,56 @@ export class DebugSessionImpl extends EventEmitter implements DebugSession {
        return this.proceedRequest("threads");
    }

-    pause(threadId: number): Promise<DebugProtocol.PauseResponse> {
+    pause(threadId?: number): Promise<DebugProtocol.PauseResponse> | Promise<DebugProtocol.PauseResponse[]> {


Let's do it in a simple way

pause(threadId?: number): Promise<DebugProtocol.PauseResponse> | Promise<DebugProtocol.PauseResponse[]> { if (threadId) { return this.doPause(threadId); } else { return this.threads().then(response => Promise.all(response.body.threads.map(thread => this.doPause(thread.id)))); } } doPause(threadId: number): Promise<DebugProtocol.PauseResponse> { return this.proceedRequest("pause", { threadId }); }

Signed-off-by: jpinkney <josh.pinkney@mail.utoronto.ca>

tolusha requested changes May 29, 2018

View reviewed changes

tolusha reviewed Jun 1, 2018

View reviewed changes

tolusha force-pushed the vscode-debug-protocol branch from faf8a36 to 0eff22d Compare June 4, 2018 11:56

JPinkney force-pushed the resume_and_suspend branch from 3734923 to 33fa65d Compare June 4, 2018 19:39

tolusha requested changes Jun 5, 2018

View reviewed changes

tolusha assigned JPinkney Jun 5, 2018

tolusha added the dap debug adapter protocol label Jun 5, 2018

tsmaeder mentioned this pull request Jun 5, 2018

Suspend/Resume Actions eclipse-che/che#9424

Closed

tolusha reviewed Jun 6, 2018

View reviewed changes

JPinkney force-pushed the resume_and_suspend branch from 4d0d992 to 9e02925 Compare June 6, 2018 13:48

Added resume and suspend actions

606b052

Signed-off-by: jpinkney <josh.pinkney@mail.utoronto.ca>

JPinkney force-pushed the resume_and_suspend branch from 9e02925 to 606b052 Compare June 6, 2018 13:51

tolusha approved these changes Jun 6, 2018

View reviewed changes

tolusha merged commit 2c8f5d3 into eclipse-theia:vscode-debug-protocol Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added resume and suspend actions #1971

Added resume and suspend actions #1971

JPinkney commented May 25, 2018

tolusha May 29, 2018

JPinkney May 31, 2018

tolusha Jun 1, 2018

tolusha May 29, 2018

tolusha May 29, 2018

tolusha May 29, 2018

tolusha May 29, 2018

tolusha May 29, 2018

tolusha Jun 1, 2018

JPinkney Jun 4, 2018

tolusha Jun 4, 2018

tolusha Jun 5, 2018

tolusha Jun 5, 2018

tolusha Jun 5, 2018

tolusha Jun 5, 2018

JPinkney Jun 5, 2018 •

edited

Loading

tolusha Jun 5, 2018 •

edited

Loading

tolusha Jun 5, 2018

tolusha Jun 5, 2018

tolusha Jun 6, 2018

tolusha Jun 6, 2018

Added resume and suspend actions #1971

Added resume and suspend actions #1971

Conversation

JPinkney commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPinkney Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

tolusha Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPinkney Jun 5, 2018 •

edited

Loading

tolusha Jun 5, 2018 •

edited

Loading