Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Polish remote debug experience #2685

Merged
merged 7 commits into from
Sep 6, 2019
Merged

Polish remote debug experience #2685

merged 7 commits into from
Sep 6, 2019

Conversation

quoctruong
Copy link
Contributor

@quoctruong quoctruong commented Aug 6, 2019

Fix a bug where setting breakpoint fails if the remote program is started through dlv with --continue switch (#2690).
Fix a bug where disconnecting (after attaching to) the remote program terminates it (#2592).
Fix a bug where setting breakpoint will fail if a breakpoint already exists (if dlv is started through multi client and another client sets the breakpoint).

@msftclas
Copy link

msftclas commented Aug 6, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @quoctruong

While @jhendrixMSFT and I review and test the changes in this PR, can you create a separate PR that has only changes related to the clean up that you have done here regarding types and interfaces?

I'd prefer to have them merged separately

@@ -781,6 +776,11 @@ class GoDebugSession extends LoggingDebugSession {

protected disconnectRequest(response: DebugProtocol.DisconnectResponse, args: DebugProtocol.DisconnectArguments): void {
log('DisconnectRequest');
// For remote process, we have to issue a continue request
// before disconnecting.
if (this.delve.request === 'attach' && !this.delve.debugProcess) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request can be launch and the user could be using the old of way doing remove debugging.

I would suggest adding a property isLocalDebugging on this.delve so that we can use that directly instead of checks like this

@quoctruong
Copy link
Contributor Author

@ramya-rao-a PTAL

@jhendrixMSFT
Copy link
Member

@quoctruong when running dlv with --continue, if I set a breakpoint after remote-attach I don't see it being set. However if the breakpoint is set then I remote-attach it is set. Could you please take a look?

@quoctruong
Copy link
Contributor Author

@jhendrixMSFT Thanks for the catch! I put out a fix for the issue. Please give it another try. It should work now.

@jhendrixMSFT
Copy link
Member

OK that fixed it however there is still some squirrely behavior.

  1. attach to remote process
  2. set breakpoint
  3. after breakpoint is hit, resume and remove breakpoint
  4. detach from process
  5. reattach to process
  6. set breakpoint, notice it won't get hit

It's possible that this was a pre-existing issue, hard to tell since I can't detach/re-attach without these changes. But since this change enables the detach/re-attach scenario I feel it's worth fixing so that one can set breakpoints on re-attach.

@quoctruong
Copy link
Contributor Author

@jhendrixMSFT I believe this should fix it. I tried detaching and setting breakpoints a few times. Thanks for the catch!

@ramya-rao-a ramya-rao-a merged commit 12e064c into microsoft:master Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants