Added support for remote debugging.

[giacoman]

I have added support for remote debugging. I did this by adding remotePath, port, and host properties to configuration. Also, mode property must be set to debug_remote. Could not find graceful way to shut down remote dlv process, so I just issue a halt command and the developer must Ctrl-C on the remote dlv process. Hopefully, you can merge this request in. Nice to be able to debug in vscode! Thanks.

[msftclas]

Hi @giacoman, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[msftclas]

@giacoman, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[lukehoban]

Here and elsewhere whitespace appears to be off. The project uses tabs. Could you make sure whitespace and formatting match?

[giacoman]

Fixed.

[lukehoban]

Thanks @giacoman - this looks great. A handful of code review comments.

Could you rebase on current master?

[giacoman]

I will go through all the recommendations and update the code. Maybe the weekend before I can get to it.

[lukehoban]

Looks great. Really good to add this support - opens up a number of new scenarios.

Only one remaining question:

I removed the commented out code. I think the other code should remain though. I shut down remote dlv process by hitting Ctrl-C. If I don't issue halt command, the remote dlv process will not shut down gracefully. It will hang trying to delete the debug binary and you will have to hit Ctrl-X and process will be left open. With halt command, hitting Ctrl-C will shut down process smoothly.

At least in the Docker-based setup I've been testing this with (see https://github.com/lukehoban/webapp-go/tree/debugging), what has made the most sense is to send a restart command to the remote dlv server when you stop debugging in VS Code. That puts the dlv server back in the state closest to what it was presumably in before attaching the VS Code debugger. And it allows restarting the debug session in VS Code. If you send a halt, it seems you then have to recycle the debug server on the remote machine every time you want to start a new debug session.

Regarding the ctrl-c issue, that sounds like a bug in Delve. I can't think of a reason why the headless mode dlv server should not shutdown gracefully on ctrl-c.

Thoughts?

[giacoman]

How did you issue restart command? Did you test running remote dlv and have it shutdown gracefully after hitting Ctrl-C while doing restart instead of halt? What I am seeing is that if I don't issue halt command, calling Ctrl-C will hang trying to delete 'Debug'. I played around with issuing restart command, but may have not been doing it right.

[lukehoban]

I tested restart just by changing from halt to restart in the Delve#close implementation in your code.

I didn't have any problem with restarting the docker container - but I'm not sure what signal that sends to the process - maybe different than ctrl-c. I didn't have need in the Docker approach to manually kill/restart the debug server.

That said, I think it would be good to raise the issue about being about to ctrl-c the debug server process as a Delve bug. It seems like something that should be fixable there.

FWIW - Here's the patch I have currently on top of your branch:

diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts
index e73e63a..ceea72e 100644
--- a/src/debugAdapter/goDebug.ts
+++ b/src/debugAdapter/goDebug.ts
@@ -234,7 +234,7 @@ class Delve {
            });
            this.debugProcess.on('close', function(code) {
                // TODO: Report `dlv` crash to user.
-               console.error('Process exiting with code: ' + code);
+               logError('Process exiting with code: ' + code);
            });
            this.debugProcess.on('error', function(err) {
                reject(err);
@@ -264,14 +264,15 @@ class Delve {
    }

    close() {
-       if (this.debugProcess === null) {
-           this.call<DebuggerState>('Command', [{ name: 'halt' }], (err, state) => {
+       if (!this.debugProcess) {
+           this.call<DebuggerState>('Command', [{ name: 'restart' }], (err, state) => {
                if (err) {
-                   console.error('Failed to halt.');
+                   logError('Failed to restart.');
                }
            });
+       } else {
+           this.debugProcess.kill();
        }
-       else this.debugProcess.kill();
    }
 }

@@ -359,15 +360,17 @@ class GoDebugSession extends DebugSession {
        log('ConfigurationDoneRequest');
    }

-   protected toDebuggerPath(path): string {
-       if (this.delve.remotePath.length === 0)
+   protected toDebuggerPath(path: string): string {
+       if (this.delve.remotePath.length === 0) {
            return this.convertClientPathToDebugger(path);
+       }
        return path.replace(this.delve.program, this.delve.remotePath).split(this.localPathSeparator).join(this.remotePathSeparator);
    }

-   protected toLocalPath(path): string {
-       if (this.delve.remotePath.length === 0)
+   protected toLocalPath(path: string): string {
+       if (this.delve.remotePath.length === 0) {
            return this.convertDebuggerPathToClient(path);
+       }
        return path.replace(this.delve.remotePath, this.delve.program).split(this.remotePathSeparator).join(this.localPathSeparator);
    }

@@ -385,9 +388,11 @@ class GoDebugSession extends DebugSession {
        })).then(() => {
            log('All cleared');
            return Promise.all(args.lines.map(line => {
-               if (this.delve.remotePath.length === 0)
+               if (this.delve.remotePath.length === 0) {
                    log('Creating on: ' + file + ':' + line);
-               else log('Creating on: ' + file + ' (' + remoteFile + ') :' + line);
+               } else {
+                   log('Creating on: ' + file + ' (' + remoteFile + ') :' + line);
+               }
                return this.delve.callPromise<DebugBreakpoint>('CreateBreakpoint', [{ file: remoteFile, line }]).catch(err => {
                    log('Error on CreateBreakpoint');
                    return null;
@@ -450,7 +455,6 @@ class GoDebugSession extends DebugSession {
                    new Source(
                        basename(location.file),
                        this.toLocalPath(location.file)
-                       // this.convertDebuggerPathToClient(location.file)
                    ),
                    location.line,
                    0

[giacoman]

Hmmm. Looking at the delve code, I don't see a 'Restart' option for 'Command'. Is it possible you are sending non-existent command and nothing was executed? If so, I would suggest still going with the halt option. For me, when I am done debugging, I would want the process to stop on the remote server so I could rebuild and run normally.

I was calling Restart differently. Something like:
this.call('Restart', [null, null], (err, ret) => {
if (err) {
console.error('Failed to restart.');
}
});

If I pass command that is not there, Delve will complain. It didn't complain for me, but it also didn't seem to restart or work for me.

I also tried what you had done and it didn't work either.

[lukehoban]

You are right - I was not correctly restarting in my tests earlier.

I just tried a few things, and I am now seeing the behaviour I expect if I use:

this.call<DebuggerState>('Command', [{ name: 'halt' }], (err, state) => {
    if (err) return logError('Failed to restart.');
    this.call<DebuggerState>('Restart', [], (err, state) => {
        if (err) return logError('Failed to restart.');
    });
});

Does that work in your usecase as well? I believe it should leave the debug server in a state where you can still ctrl-c, but will ensure also ensure that Play/Stop/Play/Stop/etc. in VS Code will work.

[giacoman]

I tried the code where you issue halt and follow it with restart. I didn't see anything different to just issuing halt. I am not able to restart debugger without relaunching remote delve process. One thing I want to make sure on. In the debugger controls in vscode, there is restart and stop button. I have been hitting the stop button. I did test using restart button and it does not successfully restart.

[lukehoban]

I just tried again, and without the call to Restart, I'm not able to reliably restart the vscode debugger. With the call to Restart in there, I can restart the vscode debugger reliably.

By "restart the vscode debugger", I mean any of:

• While the process is running, hit Stop, then hit Play again.
• While the process is paused in the debugger, hit Stop, then hit Play again.
• While the process is running, hit Restart.
• While the process is paused in the debugger, hit Restart.

This did require sending a halt followed by a Restart to Delve.

My particular test case at least is https://github.com/lukehoban/webapp-go/tree/debugging, with a breakpoint in loadPage, and a container built and launched per https://github.com/lukehoban/webapp-go/blob/debugging/.vscode/launch.json#L15.

[giacoman]

Wonder if we could be using different version of Delve. On my remote vm, I am using:
Delve Debugger
Version: 0.11.0-alpha

Calling restart doesn't affect me, so I am good with leaving it in.

[lukehoban]

Great - this is merged into master now. I'll push an update to the extension in the next day or so with these changes.

Thanks @giacoman for the work on this!