cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting client #3632

fatanugraha · 2024-01-15T17:58:45Z

Print out a message and stack trace to the stderr when we hit a breakpoint and the client that sends the continue command has disconnected.

Fixes #3469

aarzilli

I still think that needing this option is a strong symptom that there is something very wrong with your development environment. In the context of running something locally during development you would want the program to stop at a panic to examine why the panic happened.
In the context of running the program in production having a debugger attached to it constantly is a security risk.

derekparker · 2024-01-23T23:53:17Z

I'm also inclined to agree with @aarzilli, however I am familiar with the workflow described in the associated issue, and in that context I could see the benefit to the actual crash being apparent and not silenced by Delve by virtue of waiting on user input to produce the panic.

My initial feedback is I am against wholesale deletion of these breakpoints, I think it would be better to actually disable them, so they can be re-enabled later without having to know the specifics of the runtime.

aarzilli · 2024-01-24T06:26:09Z

We could also print something if we stop at a breakpoint while there isn't a connected client.

derekparker · 2024-01-24T16:19:42Z

We could also print something if we stop at a breakpoint while there isn't a connected client.

That's good too. Might even be nice to print the traceback and panic message automatically when hitting the panic breakpoints anyways, to be honest.

fatanugraha · 2024-01-25T13:19:33Z

I still think that needing this option is a strong symptom that there is something very wrong with your development environment.

I agree with you. This option is needed in our orgs because not everyone are familiar with the debugger and we want to have a setup that is practical and convenient for everyone.

My initial feedback is I am against wholesale deletion of these breakpoints, I think it would be better to actually disable them, so they can be re-enabled later without having to know the specifics of the runtime.

I tried this one actually, instead of deleting them I tried to toggling off those breakpoints, but it's not possible to do that through the API because we will reject any request that has - in it. If we're okay with changing the API behaviour I can adjust the MR to do this instead.

That's good too. Might even be nice to print the traceback and panic message automatically when hitting the panic breakpoints anyways, to be honest.

Ideally yeah this is what I'm looking for, but it seems to be a bit tricky to pull off. Let me see if i can find a way to do this.

aarzilli · 2024-01-25T15:26:40Z

Ideally yeah this is what I'm looking for, but it seems to be a bit tricky to pull off. Let me see if i can find a way to do this.

One way to do it would be like this:

There's a function in service/rpccommon/server.go called serveJSONCodec, that's the function that serves requests from a client. You can have a boolean there that you set to false when that function exits and check it in (*RPCCallback).Return.
This boolean needs to be protected with a mutex so you could copy the syncflag type in service/dap/server.go.
Have (*RPCCallback).Return return false if the client has quit and then print something in (*RPCServer).Command in service/rpc2/server.go

fatanugraha · 2024-01-26T04:37:18Z

One way to do it would be like this:

oh ya i have implemented this in this PR but in a different fashion.

Might even be nice to print the traceback and panic message automatically when hitting the panic breakpoints anyways, to be honest.

Initially im thinking to move the unhandled panic breakpoint to somewhere after we print out the message but it's going to be a bit tricky to make it sure it works for older (and future) go versions so I'm going with implementing this on the server side. Hope that's acceptable.

aarzilli

(this was a leftover comment)

_fixtures/fatalthrow.go

service/rpc2/server.go

derekparker · 2024-02-22T19:35:56Z

Hey @fatanugraha there has been no update on this PR in a month, are you still planning to work on this?

fatanugraha · 2024-02-23T22:22:32Z

Hi @derekparker I have plans to pick this up again in 1-2 weeks from now, but if anyone else wants to work on this issue I'm also okay with that. Sorry for the delay :(

derekparker · 2024-02-23T22:34:13Z

Hi @derekparker I have plans to pick this up again in 1-2 weeks from now, but if anyone else wants to work on this issue I'm also okay with that. Sorry for the delay :(

No worries, and no rush! Just wanted to check in.

service/rpc2/server.go

TomK · 2024-04-05T12:01:52Z

@fatanugraha would you like some additional hands on this?
I'm happy to make the changes suggested by @aarzilli

fatanugraha · 2024-04-07T07:49:42Z

Hey folks, sorry for the bad delays. Here's how it looks right now if we hit a breakpoint (screenshot below is when we hit the panic/fatal bp) but the client that sends the continue gone already

cmd/dlv/dlv_test.go

aarzilli · 2024-04-08T14:45:45Z

service/rpc2/server.go

 	var out CommandOut
 	out.State = *st
-	cb.Return(out, nil)
+	if clientIsConnected := cb.Return(out, nil); s.config.Headless && !clientIsConnected {


We can't do this in this layer. It will not work if delve is started with dlv dap or if someone connects to it using VSCode (or any other program that uses the DAP protocol) and then that client quits.

Right, I missed that. I'm leaning towards doing this in both rpc2 and dap or do you suggest that we should do it in the debugger layer instead?

I lean slightly towards doing it in debugger but both would be ok.

Hi @aarzilli , correct me if i'm wrong, after checking dap's code I'm not sure if we need to implement this for dap. reasons being:

dlv dap doesn't support multiclient

dlv dap will quit when the client disconnected (so printing the log here won't be helpful here)

ref: (upon disconnection, exit from the serve loop)

You might be right about dlv dap but DAP connections are also accepted when delve is started with dlv --headless debug. See serveConnectionDemux in service/rpccommon/server.go as well as onDisconnectRequest in service/dap/server.go.

Ah yeah you're right, thanks for the clarification.

After reading more of the debugger code I agree that we should implement this on the debugger layer. By doing that we can also cover more cases e.g. disconnecting client after sending a long call command.

Can you help to take a look when you have time and see if it's acceptable? Thanks!

derekparker · 2024-04-19T15:56:40Z

service/rpccommon/server.go

@@ -412,9 +415,28 @@ func (cb *RPCCallback) Return(out interface{}, err error) {
 		outbytes, _ := json.Marshal(out)
 		cb.s.log.Debugf("(async %d) -> %T%s error: %q", cb.req.Seq, out, outbytes, errmsg)
 	}
+
+	if hasDisconnected := cb.hasDisconnected(); hasDisconnected {


Can be simplified to if cb.hasDisconnected()

derekparker · 2024-04-19T15:58:07Z

service/debugger/debugger.go

@@ -2428,3 +2448,43 @@ var attachErrorMessage = attachErrorMessageDefault
 func attachErrorMessageDefault(pid int, err error) error {
 	return fmt.Errorf("could not attach to pid %d: %s", pid, err)
 }
+
+func (d *Debugger) dumpGoroutineStack(currentThread *api.Thread) {


This should be named more in line with what it does. Of course it does dump the goroutine stack, but it's main purpose is to print a message / warning on an unattended breakpoint hit.

derekparker · 2024-04-19T16:00:25Z

service/dap/server.go

@@ -524,6 +527,12 @@ func (s *Session) ServeDAPCodec() {
 			s.config.triggerServerStop()
 		}
 	}()
+
+	s.disconnectCh = make(chan struct{})
+	defer func() {


Can just defer close(s.disconnectCh) without the anonymous function.

Actually, maybe the channel should live on the s.conn connection and be closed via the above defer s.conn.Close()?

aarzilli · 2024-04-24T09:35:44Z

service/debugger/debugger.go

@@ -1190,6 +1188,24 @@ func (d *Debugger) Command(command *api.DebuggerCommand, resumeNotify chan struc

 	if command.Name != api.SwitchGoroutine && command.Name != api.SwitchThread && command.Name != api.Halt {
 		d.target.ResumeNotify(resumeNotify)
+
+		if clientStatusCh != nil {
+			defer func() {


I think it's better to move this just before the return state, err at the end of the function. I think it's clearer to do it directly instead of using a defer because there is only one path that returns a non nil state and we don't need to actually receive from clientStatusCh in cases where we return an error.

fatanugraha force-pushed the support-disable-panic-catcher branch 2 times, most recently from c12a582 to 66a3316 Compare January 15, 2024 18:04

aarzilli reviewed Jan 15, 2024

View reviewed changes

fatanugraha force-pushed the support-disable-panic-catcher branch from 66a3316 to c29cb69 Compare January 25, 2024 19:39

fatanugraha changed the title ~~cmd/dlv: add --disable-break-on-panic to disable panic handler breakpoint auto creation~~ cmd/dlv: print out message for multi accept mode on panic/fatalthrow Jan 25, 2024

aarzilli reviewed Jan 28, 2024

View reviewed changes

_fixtures/fatalthrow.go Outdated Show resolved Hide resolved

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

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

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

TomK mentioned this pull request Apr 5, 2024

feature request: option to disable unrecovered panic breakpoint #3653

Closed

TomK reviewed Apr 5, 2024

View reviewed changes

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

Print out message and dump stack on pause

c9531a9

fatanugraha force-pushed the support-disable-panic-catcher branch from cf2069e to c9531a9 Compare April 7, 2024 07:48

fatanugraha changed the title ~~cmd/dlv: print out message for multi accept mode on panic/fatalthrow~~ cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting clients Apr 7, 2024

fatanugraha changed the title ~~cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting clients~~ cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting client Apr 7, 2024

aarzilli reviewed Apr 8, 2024

View reviewed changes

fatanugraha added 2 commits April 9, 2024 15:45

Fix test

1e825af

Move the logic to debugger layer

593758a

fatanugraha force-pushed the support-disable-panic-catcher branch from ac5dd6c to 593758a Compare April 17, 2024 16:24

Remove unused fields

321d1b0

derekparker requested changes Apr 19, 2024

View reviewed changes

aarzilli reviewed Apr 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting client #3632

cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting client #3632

fatanugraha commented Jan 15, 2024 •

edited

aarzilli left a comment

derekparker commented Jan 23, 2024

aarzilli commented Jan 24, 2024

derekparker commented Jan 24, 2024

fatanugraha commented Jan 25, 2024

aarzilli commented Jan 25, 2024

fatanugraha commented Jan 26, 2024

aarzilli left a comment •

edited

derekparker commented Feb 22, 2024

fatanugraha commented Feb 23, 2024

derekparker commented Feb 23, 2024

TomK commented Apr 5, 2024

fatanugraha commented Apr 7, 2024 •

edited

aarzilli Apr 8, 2024

fatanugraha Apr 9, 2024

aarzilli Apr 9, 2024

fatanugraha Apr 17, 2024 •

edited

aarzilli Apr 17, 2024

fatanugraha Apr 17, 2024

fatanugraha Apr 17, 2024

derekparker Apr 19, 2024

derekparker Apr 19, 2024

derekparker Apr 19, 2024

derekparker Apr 19, 2024

aarzilli Apr 24, 2024

cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting client #3632

Are you sure you want to change the base?

cmd/dlv: print out message with stack trace when breakpoint is hit but has no waiting client #3632

Conversation

fatanugraha commented Jan 15, 2024 • edited

aarzilli left a comment

Choose a reason for hiding this comment

derekparker commented Jan 23, 2024

aarzilli commented Jan 24, 2024

derekparker commented Jan 24, 2024

fatanugraha commented Jan 25, 2024

aarzilli commented Jan 25, 2024

fatanugraha commented Jan 26, 2024

aarzilli left a comment • edited

Choose a reason for hiding this comment

derekparker commented Feb 22, 2024

fatanugraha commented Feb 23, 2024

derekparker commented Feb 23, 2024

TomK commented Apr 5, 2024

fatanugraha commented Apr 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatanugraha Apr 17, 2024 • edited

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

fatanugraha commented Jan 15, 2024 •

edited

aarzilli left a comment •

edited

fatanugraha commented Apr 7, 2024 •

edited

fatanugraha Apr 17, 2024 •

edited