debug: reports only one breakpoint when multiple goroutines stop simultaneously

[ramya-rao-a]

From microsoft/vscode-go#2564 by @aarzilli:

1. Debug https://github.com/aarzilli/delve_client_testing/blob/master/04simbp/main.go
2. set a breakpoint on main.go:9
3. hit continue repeatedly

I expect that a total of 100 breakpoint hits will be reported but what happens is that many fewer (between 20 and 30 on a 4 core system) will be reported. The reason is that when multiple goroutines hit the breakpoint at the same time only one is reported, with the other being silently discarded.

[lggomez]

Making this work properly will be a potential issue for applications running goroutines in the order of the hundreds or thousands, both in performance and UI terms (image from the cpp DAP for illustrative purposes):

I understand changing the UI to be more compact (i.e: listbox) is not something implementable from the DAP side and would involve a big change to the vscode workbench, but I don't know which alternative we could have aside from command to page goroutines (which the ListGoroutines method sopports)

[lggomez]

PS: Also, another issue is the naming, since we would be handling goroutine stacktraces instead of threads themselves

Seeing multiple threads with their goroutines is a scenario that I believe the call stack pane doesn't support either, and wouldn't play nice with this tree UI (also, it would be worth questioning whether the extension wants to offer such feature)

[polinasok]

I believe all 100 (or more) goroutines already show up in the UI if they are active at the same time. @aarzilli has expressed his concern in the past that fetching and displaying all goroutines every time the call stack is refreshed can be very expensive. This is an issue even if we do not have simultaneous breakpoints (see #129).

The specific issue here is that because of simultaneous breakpoints, the UI does not report all 100 breakpoints, simultaneously or consecutively. For the sake of argument, let's imagine that all 100 goroutines hit a breakpoint at the exact same time. In that case, only one breakpoint would be reported back to the user and the rest will not be flagged at all.

When execution stops at a breakpoint, the adapter generates a stopped event for the current goroutine and a flag that all other goroutines are paused as well. The current goroutine gets status "paused on breakpoint". All other goroutines get status "paused". This allows the user to expand any of the paused goroutines in the UI, which in turn generates requests for scopes and variables. If that all-stopped flag were false, all other threads would show up as "Running" without the option to expand and view code and vars.

Unfortunately, the dap spec does not allow specification of multiple thread ids in a single response. To correctly display the status for each goroutine, we would have to loop over all the threads in the debugger state and issue a stopped event for each one with a non-nil breakpoint to overwrite "paused" with "paused on breakpoint". This will cause the editor to generate threads and stackTrace requests per each stopped event. And while the stackTrace request will be unique to each stopped goroutine, the expensive threads request will be duplicated every time. In addition, the variables will be requested and displayed for the last goroutine that was reported as stopped last, not the current goroutine (which I think is the desired behavior). And while we could take care to report that one as stopped last, we would still need to make sure that AllThreadsStopped is set to true only in the first stopped event or it will overwrite "paused on breakpoint" with "paused". With care we can get this messy sequence of reporting right, but without changing the editor, there is nothing we can do about the many repeated threads and stacktrace requests that will be triggered by this.

@lggomez
What exactly is the cpp image illustrating? I am not sure. Please advise.
Also, I believe in the context of Go, the goroutines are reported back as threads to work with the UI. Delve cli does support both reporting of goroutines and threads, but I am not sure how the extra thread info (which as you explained we cannot support with the current UI together with the goroutines) would help in the case of simultaneous breakpoints.

[aarzilli]

For what it's worth, the number of simultaneously stopped goroutines is always going to be less than the number of CPU cores. It could still be 100, but usually it will be much less.