Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

debug: refactor DebugService into DebugServer #11121

Open
paul-marechal opened this issue May 10, 2022 · 4 comments
Open

debug: refactor DebugService into DebugServer #11121

paul-marechal opened this issue May 10, 2022 · 4 comments
Labels
debug issues that related to debug functionality quality issues related to code and application quality

Comments

@paul-marechal
Copy link
Member

The DebugService interface is doing too many things: it is both implemented in the frontend (PluginDebugService) and in the backend (DebugServiceImpl). There should be a single DebugService that handles multiple debugging sources as contributions. The plugin-ext package should not rebind DebugService as it is currently done.

@paul-marechal paul-marechal added quality issues related to code and application quality debug issues that related to debug functionality labels May 10, 2022
@tsmaeder
Copy link
Contributor

tsmaeder commented Oct 4, 2022

If we start changing those interfaces (and we should), we should really make sure to get the community on board, in particular the folks at ARM, for example. They seem to use this stuff beyond VS Code extensions. @thegecko

@thegecko
Copy link
Member

thegecko commented Oct 4, 2022

Indeed, the debug services can execute in both frontend and backend contexts.

If merging into a single service, it needs to:

@tsmaeder
Copy link
Contributor

tsmaeder commented Oct 6, 2022

I've been reading through the debugger code again. Here are a couple of observations:

  1. We're not consuming an official version of the debug adapter protocol (https://github.com/microsoft/vscode-debugadapter-node/tree/main/protocol), but our own protocol definition. This makes it hard to keep up with development and to be sure that we're using the correct messages.
  2. There is no concept of a debug adapter in our implementation debugger architecture (it is present as an implementation detail only). This makes it hard to get the lifecycle events correct. We have the concept of a DebugSessionConnection and the concept of a "session" in the DebugService, but for the very least, the naming is confusing. Debug adapters are a core concept of the Debug Adapter Protocol and there is nothing wrong with having them present in the architecture.
  3. As @paul-marechal's states in the description, it really should not be necessary to override the DefaultDebugService in order to implement the plugins debug facilities. We should have a single debug service and the variability should come in mostly through different kinds of DebugContributions like BackEndDebugContribution and PluginDebugContribution. These can be contributed either through dependency injection or registered dynamically. The consequence would be that the contributions would be responsible for starting debug adapters and establishing connections to them.
  4. We should separate the debugger entities and the display of these entities in the UI: right now DebugSession is implmented in a *.tsx file and implements TreeElement.
  5. The debug session lifecycle is implemented by the DebugSessionsManager in cooperation with the DebugSession object. It's not always possible to determine whether something happened due to user interaction or because of an event in the debuggee. This makes implementing stuff like lifecycleControlledByParent difficult.

These changes would not affect clients that use plugins to contribute debuggers, but only folks who override the debug service or contribute debuggers through the dependency injection mechanism. I believe the refactoring would be highly useful to make further changes in the debug module easier and the code to be of higher quality.

If we go this route, we should work closely with adopters to make the change as painless as possible.

@paul-marechal
Copy link
Member Author

paul-marechal commented Oct 6, 2022

A couple notes regarding what you wrote about:

  1. In the VS Code API, the DebugAdapter interface is basically a DAP connection abstraction: A debug adapter is something that reads and spits out DAP messages. Doesn't matter where the messages are originated from or how the messages are transported. I think our API's first class citizen should be a Debugger interface supporting everything supported by the DAP, and debug adapters would be implementation details hidden behind wrappers to create Debugger instances internally: e.g. const debugger: Debugger = createDebuggerFromDebugAdapter(debugAdapter);
  2. In broad strokes: A DebugSession should be wrapping debugger(s?). We should have a DebugWidget/DebugView driving one or several DebugSessions from the UI?
  3. No opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality quality issues related to code and application quality
Projects
None yet
Development

No branches or pull requests

3 participants