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

debugAdapter: Remove redundant support for thread events #3145

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

polinasok
Copy link
Contributor

@polinasok polinasok commented Apr 1, 2020

The overview says, “Thread events are optional, but a debug adapter can send them to force the development tool to update the threads UI dynamically even when not in a stopped state. If a debug adapter decides not to emit Thread events, the thread UI in the development tool will only update if a stopped event is received.”

Delve does not support events and cannot respond to ListGoroutines requests in running state. Therefore, there is no need for the thread event support here.

@msftclas
Copy link

msftclas commented Apr 1, 2020

CLA assistant check
All CLA requirements met.

@hyangah
Copy link
Contributor

hyangah commented Apr 1, 2020

The main reason of test failure is due to prior commits. I sent a separate PR to address that. Once the PR is merged and the branch is rebased, I think the test result will look better (except the known flakiness)

Copy link
Contributor

@quoctruong quoctruong left a comment

Choose a reason for hiding this comment

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

LGTM. This shouldn't affect debugging multi-threaded program?

@polinasok
Copy link
Contributor Author

polinasok commented Apr 2, 2020

Quoc, all threads (or more precisely goroutines) are communicated via mandatory threads requests and those are displayed in the UI. I see no impact on the UI with or without additional thread events. If you have some specific program in mind that you want me to test with, please let me know.

Also, I checked in with the delve folks on the fallback to get the current thread from goroutines[0], and their response is "a better choice would be to use GoroutineID == 0" (see here). I am not quite sure what they mean, but once I figure it out, I will make the change in this PR. This is a corner case though as in the cases I encountered during testing the current thread to list in the stopped event usually comes from debug state.

@ramya-rao-a
Copy link
Contributor

@polinasok Just one question. Have you tested the pause feature? The parts of code being removed in this PR has overlap with what was done in #2126 to implement the pause feature as requested in #978

@polinasok
Copy link
Contributor Author

polinasok commented Apr 7, 2020

I tested pausing the best I could in several scenarios with a program with multiple goroutines and sleep pauses to give me time to click around:

  • pause before stopping on entry
  • pause in a program with no breakpoints
  • pause in a program with breakpoints
  • pause before a breakpoint
  • pause after a breakpoint
  • pause and set breakpoint
  • set breakpoint while the program is running
    They all seem to be working the same with and without the change.
    I also studied the request-response logs and they all seem consistent with each other.
    But I would welcome any testing help. of course.

Looking at the change that you referenced, I do not think this particular code is what fixed the pause event. I think it was the other changes in that PR mattered. That said, I think there is a bug in the pause code. The pause request results in halt RPC to delve and on return handeReenterDebug is called to generate StoppedEvent("pause"). At the same time, the halt causes continue async request to return as well, which also calls handleReenterDebug but to generate StoppedEvent("breakpoint"). Since this is all happening in parallel, these events compete with each other, so sometimes the UI shows "paused on breakpoint" and sometimes "paused on pause" for the exact code/sequence of actions, even when there are no explicit breakpoints set. Both RPCs return identical state that is logged with "pause state" and "continue state". Also duplicate stopped events result in duplicate threads and stacktrace requests and even more stacktrace and threads requests end up happening for the same stop from the started/exited thread events. I will file an issue about this.

@ramya-rao-a
Copy link
Contributor

Thanks @polinasok, we can go ahead with this change then

@ramya-rao-a ramya-rao-a merged commit 082bcfd into microsoft:master Apr 7, 2020
@polinasok polinasok deleted the no-thread-event branch April 7, 2020 18:04
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.

5 participants