service/dap: refine teardown logic #2414

polinasok · 2021-04-04T06:27:15Z

Updates #1515

polinasok · 2021-04-04T06:36:03Z

service/dap/server.go

+}
+
+func (s *Server) stopDebugSession(killProcess bool) {
+	_, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)


Why doesn't rpc server halt in Stop()?

delve/service/rpccommon/server.go

Lines 91 to 97 in c223ef6

func (s *ServerImpl) Stop() error {

if s.config.AcceptMulti {

close(s.stopChan)

s.listener.Close()

}

kill := s.config.Debugger.AttachPid == 0

return s.debugger.Detach(kill)

?
On a related note, the terminology collision here is unfortunate: https://github.com/go-delve/delve/blob/master/Documentation/api/ClientHowto.md#gracefully-ending-the-debug-session
From the RPC client point of view, it appears that

Detach = detach debugger from the process (maybe kill it) + close client connection

Disconnect = close client connection
But the client how-to uses a phrase "disconnecting a running program" as a case where Detach is called. And then we have DAP where disconnect request is sent to "disconnect from the debuggee and to terminate the debug adapter". Sigh.

Wild guess - isn't it because it's the rpc client who is supposed to call Halt (or Disconnect) command before the server stops. Not sure about the case where the debugging stops because client unexpectedly crashes or gets disconnected.

That's precisely the case I was thinking about. The client takes care of Halt before Detach. But if there is an error reading rpc messages from client, the server will go into shutdown mode and call Stop() without halting.

I don't think there's a particular reason, halting would probably be better. And the client howto should say detach where it says ""disconnecting a running program".

polinasok · 2021-04-05T08:02:58Z

Test failure is unrelated:
=== RUN Test1Issue419
Start
common_test.go:18: failed assertion at integration1_test.go:1023: Continue() - thread 2700 does not exist
--- FAIL: Test1Issue419 (1.75s)

polinasok · 2021-04-05T17:37:05Z

cc @hyangah

service/dap/server.go

hyangah · 2021-04-05T20:33:19Z

service/dap/server.go

+}
+
+func (s *Server) stopDebugSession(killProcess bool) {
+	_, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)


Wild guess - isn't it because it's the rpc client who is supposed to call Halt (or Disconnect) command before the server stops. Not sure about the case where the debugging stops because client unexpectedly crashes or gets disconnected.

service/dap/server.go

polinasok

PTAL

service/dap/server.go

polinasok · 2021-04-05T23:28:04Z

Failing tests are unrelated:
TestIssue419github.com/go-delve/delve/pkg/proc
TestAttachDetachgithub.com/go-delve/delve/pkg/proc

hyangah

Thanks!
I don't know enough to review the Halt + Detach logic so still need review from delve maintainers. Other parts look good to me.

service/dap/server.go

polinasok · 2021-04-06T04:48:56Z

service/dap/server.go

 	p := s.noDebugProcess
 	s.noDebugProcess = nil
-	defer s.mu.Unlock()


@hyangah Was this meant to be regular unlock to minimize scope (hence the temp var p var)? With my new change, the entire function gets locked (so I shouldn't need p anymore). Not ideal? But at the same time, what else is there to do when we are existing?

Heh, this is a bug - I meant s.mu.Unlock(), not defer s.mu.Unlock. :-)

Yeah, the intention was to reduce the scope - so the goroutine that's blocked onLaunchRequest (waiting for the termination of s.nodebugProcess) can proceed as soon as it's unblocked when p's killed. But you're right. We don't need to be too clever here.

Thanks for clarifying. I remove the temp var "p".

aarzilli

LGTM

polinasok · 2021-04-06T23:21:47Z

@suzmue
TestStepOutPreservesGoroutine is failing because it hit program termination when it expected to stop after a step.
This is not related to my comment and nodebug changes. I can't reproduce it locally - a race in the test? I think it would help to enable --log-output=dap,debugger in these tests, so failed logs have more info.

polinasok · 2021-04-07T00:07:38Z

@suzmue
TestStepOutPreservesGoroutine is failing because it hit program termination when it expected to stop after a step.
This is not related to my comment and nodebug changes. I can't reproduce it locally - a race in the test? I think it would help to enable --log-output=dap,debugger in these tests, so failed logs have more info.

Fix at #2417

derekparker · 2021-04-12T22:03:19Z

Needs rebase, I'll TAL tomorrow.

polinasok · 2021-04-13T18:51:34Z

Needs rebase, I'll TAL tomorrow.

I am working on the merge.

polinasok · 2021-04-13T23:26:29Z

service/dap/server.go

-				FailedToAttach, "Failed to attach", err.Error())
+		func() {
+			s.mu.Lock()
+			defer s.mu.Unlock() // Make sure to unlock in case of panic that will become internal error


I am not sure why this did not show up before the merge, but there is a test in TestBadAttachRequest with negative attach pid that resulted in a panic from debugger.New that skipped releasing the lock and then disconnect was forever waiting to acquire it. I changed onLaunchRequest in a similar way as well just in case.

polinasok · 2021-04-15T20:35:38Z

Unrelated TeamCity failure:
TestPackageRenames github.com/go-delve/delve/service/test

polinasok · 2021-04-19T23:09:32Z

@derekparker Is there more to be done to get this PR in? It would be great to get it out of the way to resolve more conflicts in subsequent PRs. Thank you.

* service/dap: refine teardown logic * Address review comments + add missing lock/unlock * Narrow lock scope * Update comments only * Remove redundan temp var from stopNoDebugProcess * Clarify comment * Set debugger to nil after detach to prevent dup teardown in Stop() Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>

polinasok commented Apr 4, 2021

View reviewed changes

polinasok force-pushed the Teardown branch from 4c3173f to c71818e Compare April 5, 2021 17:17

service/dap: refine teardown logic

3abb724

polinasok force-pushed the Teardown branch from c71818e to 3abb724 Compare April 5, 2021 17:34

hyangah reviewed Apr 5, 2021

View reviewed changes

polinasok added 2 commits April 5, 2021 14:41

Merge master

aafef7b

Address review comments + add missing lock/unlock

49b85a8

polinasok commented Apr 5, 2021

View reviewed changes

service/dap/server.go Show resolved Hide resolved

service/dap/server.go Outdated Show resolved Hide resolved

service/dap/server.go Outdated Show resolved Hide resolved

service/dap/server.go Outdated Show resolved Hide resolved

hyangah reviewed Apr 6, 2021

View reviewed changes

service/dap/server.go Outdated Show resolved Hide resolved

Narrow lock scope

e8a28ed

polinasok commented Apr 6, 2021

View reviewed changes

aarzilli approved these changes Apr 6, 2021

View reviewed changes

polinasok added 2 commits April 6, 2021 15:51

Update comments only

13243f9

Remove redundan temp var from stopNoDebugProcess

ae989b9

Clarify comment

ca5e7ea

polinasok mentioned this pull request Apr 8, 2021

service/dap: support running requests asynchronously #2423

Merged

Merge branch 'master'

9f4f0f6

hyangah mentioned this pull request Apr 9, 2021

debug: fix and expand extension logging for dlv-dap and process teardown golang/vscode-go#1410

Closed

hyangah approved these changes Apr 9, 2021

View reviewed changes

Merge master + unlock fix in onAttachRequest

0f2fe5e

polinasok commented Apr 13, 2021

View reviewed changes

polinasok added 2 commits April 13, 2021 16:30

Set debugger to nil after detach to prevent dup teardown in Stop()

a3048c4

Merge master

7dad96d

Merge master

4c803b0

derekparker merged commit e141c47 into go-delve:master Apr 21, 2021

polinasok deleted the Teardown branch May 4, 2021 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service/dap: refine teardown logic #2414

service/dap: refine teardown logic #2414

polinasok commented Apr 4, 2021 •

edited

Loading

polinasok Apr 4, 2021 •

edited

Loading

hyangah Apr 5, 2021

polinasok Apr 5, 2021

aarzilli Apr 6, 2021

polinasok commented Apr 5, 2021 •

edited

Loading

polinasok commented Apr 5, 2021

hyangah Apr 5, 2021

polinasok left a comment

polinasok commented Apr 5, 2021

hyangah left a comment

polinasok Apr 6, 2021

hyangah Apr 6, 2021

polinasok Apr 6, 2021

aarzilli left a comment

polinasok commented Apr 6, 2021

polinasok commented Apr 7, 2021

derekparker commented Apr 12, 2021

polinasok commented Apr 13, 2021

polinasok Apr 13, 2021 •

edited

Loading

polinasok commented Apr 15, 2021 •

edited

Loading

polinasok commented Apr 19, 2021

	func (s *ServerImpl) Stop() error {
	if s.config.AcceptMulti {
	close(s.stopChan)
	s.listener.Close()
	}
	kill := s.config.Debugger.AttachPid == 0
	return s.debugger.Detach(kill)

service/dap: refine teardown logic #2414

service/dap: refine teardown logic #2414

Conversation

polinasok commented Apr 4, 2021 • edited Loading

polinasok Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

hyangah Apr 5, 2021

Choose a reason for hiding this comment

polinasok Apr 5, 2021

Choose a reason for hiding this comment

aarzilli Apr 6, 2021

Choose a reason for hiding this comment

polinasok commented Apr 5, 2021 • edited Loading

polinasok commented Apr 5, 2021

hyangah Apr 5, 2021

Choose a reason for hiding this comment

polinasok left a comment

Choose a reason for hiding this comment

polinasok commented Apr 5, 2021

hyangah left a comment

Choose a reason for hiding this comment

polinasok Apr 6, 2021

Choose a reason for hiding this comment

hyangah Apr 6, 2021

Choose a reason for hiding this comment

polinasok Apr 6, 2021

Choose a reason for hiding this comment

aarzilli left a comment

Choose a reason for hiding this comment

polinasok commented Apr 6, 2021

polinasok commented Apr 7, 2021

derekparker commented Apr 12, 2021

polinasok commented Apr 13, 2021

polinasok Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

polinasok commented Apr 15, 2021 • edited Loading

polinasok commented Apr 19, 2021

polinasok commented Apr 4, 2021 •

edited

Loading

polinasok Apr 4, 2021 •

edited

Loading

polinasok commented Apr 5, 2021 •

edited

Loading

polinasok Apr 13, 2021 •

edited

Loading

polinasok commented Apr 15, 2021 •

edited

Loading