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

[fatanugraha]

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]

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]

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

[derekparker]

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]

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]

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]

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)

[derekparker]

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

[fatanugraha]

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]

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.

[TomK]

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

[fatanugraha]

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

[fatanugraha]

Sorry folks for the long delay, life got in the way 😅

[aarzilli]

Overall this is good, except for a couple of changes that may have been caused by a merge conflict. Also could you remove all the extra newlines that you have added at various points.

[aarzilli]

LGTM, thank you.

[TomK]

Great work @fatanugraha 🙏