debug: fix updating breakpoints while debuggee is running #2128
Conversation
xiphon
force-pushed
the
fix-breakpoints
branch
5 times, most recently
from
9edd5b0
to
003a3e5
Compare
|
@xiphon can you please rebase on top of master? |
xiphon
force-pushed
the
fix-breakpoints
branch
2 times, most recently
from
a5c534e
to
504b8cd
Compare
|
Rebased |
| @@ -1073,17 +1091,22 @@ class GoDebugSession extends LoggingDebugSession { | |||
| this.threads.delete(id); | |||
| }); | |||
|
|
|||
| if (this.skipStopEventOnce) { | |||
| this.skipStopEventOnce = false; | |||
| return; | |||
There was a problem hiding this comment.
Its not clear to me as to why we need to do this...
There was a problem hiding this comment.
To silently skip the pause that we introduce just to set the breakpoint and continue right after.
xiphon
force-pushed
the
fix-breakpoints
branch
from
504b8cd
to
a9e52a9
Compare
src/debugAdapter/goDebug.ts
Outdated
| return this.continue(); | ||
| }, err => { | ||
| logError(err); | ||
| return this.sendErrorResponse(response, 2009, 'Failed to continue delve after setting the breakpoint: "{e}"', { e: err.toString() }); |
There was a problem hiding this comment.
Since this error handler is for the continue operation, I would move it up like so:
this.delve.callPromise('Command', [{ name: 'halt' }]).then(() => {
return this.setBreakPoints(response, args).then(() => {
return this.continue().then(null, err => {
logError(err);
this.sendErrorResponse(response, 2009, 'Failed to continue delve after setting the breakpoint: "{e}"', { e: err.toString() });
})
}, null);
}, err => {
logError(err)
this.sendErrorResponse(response, 2008, 'Failed to halt delve before attempting to set breakpoint: "{e}"', { e: err.toString() });
});
There was a problem hiding this comment.
Actually, on second thought, this.continue() doesnt return a promise.
Also, inside this.continue(), if the call to delve failed, it is logged and the error doesnt bubble up here.
xiphon
force-pushed
the
fix-breakpoints
branch
from
a9e52a9
to
ba96f6c
Compare
|
@xiphon I have pushed a commit to handle the error from continue(). Please see 45e6b64 and 4226264 In the normal scenario where |
|
this needs to be rebased against pull/2126 |
|
Fixes #2002 |
Right
Updated the description Will update the PR to finally fix all the edge cases:
|
|
Updated |
|
@xiphon If the continue after the halting of delve fails, right now we are just logging the error. Shouldnt we send the error response too? |
|
If we reached |
Issue
Can't set/remove breakpoints while debuggee is running
Details
Implemented stop-breakpoint-start approach to resolve the issue as described in #978 (comment).
Take a look at #978 for details, @muravjov's comments contain detailed description what's going wrong in that case.
Fixes #2002