-
Notifications
You must be signed in to change notification settings - Fork 758
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: support "console" property in launch.json (RunInTerminal) #124
Comments
I managed to prototype this and let vscode launch the headless delve server in the terminal using DAP cc/ @suzmue @polinasok |
Change https://golang.org/cl/268461 mentions this issue: |
Any progress on this? |
What is the sequence of requests/responses here? Something like this?
Can you not introduce a delay like we do in the regular adapter: vscode-go/src/debugAdapter/goDebug.ts Lines 697 to 709 in bc20520
|
The For a high level summary, here are the use cases mentioned by the users in relationship to the request for
Of course, the workarounds do not offer a great user experience. But in the absence of the console option, the users are asking if we could offer some shortcuts for these workarounds via the extra extension settings and features. In the meantime, dlv added new options that look related |
This PR adds support for reverse requests that are sent from the debug adapter to the client. Currently, RunInTerminal is the only such reverse request. This is a pre-work for the 'console' support (golang/vscode-go#124) - we plan to use RunInTerminal to ask the editor to run a command in the integrated or external terminal while handling a launch request with the console attribute. Launch request was classified as a synchronous request and was blocking ServeDAPCodec loop, which means the response message from the client couldn't be read until onLaunchRequest returns. This PR adds two goroutines - one to handle requests from the client (loopHandleRequests), and another to handle responses (loopHandleResponses). serveDAPCodec will keep read DAP messages from the net.Conn, but delegate message handling to the goroutines through buffered channels. Alternatively, I tried to avoid introducing goroutines by making onLaunchRequest asynchronously complete the launch if integrated or external console mode is chosen. I.e., make onLaunchRequest return without sending LaunchResponse, free the ServeDAPCodec loop, and return LaunchResponse (or ErrorResponse) when RunInTerminal response is received. But it was hard to follow, didn't look like a typical go program, and wasn't extensible when DAP adds more reverse requests or we ever want to utilize RunInTerminal while handling other requests. For reverse requests, we maintain a pendingReverseReq map for requests sent to the client. When response messages arrive we look up the map, and notify the waiters using a buffered channel. onLaunchRequest uses RunInTerminal if the launch request has "integrated" or "external" console attribute and asks the client to run a bogus command - this is for testing. The follow up PRs will implement the real command that starts a delve and use the command from the integrated or external terminal.
This PR adds support for reverse requests that are sent from the debug adapter to the client. Currently, RunInTerminal is the only such reverse request. This is a pre-work for the 'console' support (golang/vscode-go#124) - we plan to use RunInTerminal to ask the editor to run a command in the integrated or external terminal while handling a launch request with the console attribute. Launch request was classified as a synchronous request and was blocking ServeDAPCodec loop, which means the response message from the client couldn't be read until onLaunchRequest returns. This PR adds two goroutines - one to handle requests from the client (loopHandleRequests), and another to handle responses (loopHandleResponses). serveDAPCodec will keep read DAP messages from the net.Conn, but delegate message handling to the goroutines through buffered channels. Alternatively, I tried to avoid introducing goroutines by making onLaunchRequest asynchronously complete the launch if integrated or external console mode is chosen. I.e., make onLaunchRequest return without sending LaunchResponse, free the ServeDAPCodec loop, and return LaunchResponse (or ErrorResponse) when RunInTerminal response is received. But it was hard to follow, didn't look like a typical go program, and wasn't extensible when DAP adds more reverse requests or we ever want to utilize RunInTerminal while handling other requests. For reverse requests, we maintain a pendingReverseReq map for requests sent to the client. When response messages arrive we look up the map, and notify the waiters using a buffered channel. onLaunchRequest uses RunInTerminal if the launch request has "integrated" or "external" console attribute and asks the client to run a bogus command - this is for testing. The follow up PRs will implement the real command that starts a delve and use the command from the integrated or external terminal.
This command is a helper command to be used by dlv dap when launch request with a console property (integrated, external) is received. The dlv dap server then asks the client to run this command in the integrated or external terminal using the RunInTerminal request and turns itself into a proxy mode that forwards messages between the client and the dap-reverse command run by the editor. The dap-reverse command is similar to the dap command, except that instead of opening a port and running as a server listening on the port, this command dials to the supplied address (the rendezvous port setup by the dlv dap server operating in proxy mode). Once the dlv-reverse command is connected, the dlv dap server will forward all the messages from the client (including the initialize request and the launch request) and relay all the responses from the dlv-reverse back to the client. This command is internal use only, so it's intentionally hidden from users - dlv usage and manual will not display info about this. Update golang/vscode-go#124
This PR adds support for reverse requests that are sent from the debug adapter to the client. Currently, RunInTerminal is the only such reverse request. This is a pre-work for the 'console' support (golang/vscode-go#124) - we plan to use RunInTerminal to ask the editor to run a command in the integrated or external terminal while handling a launch request with the console attribute. Launch request was classified as a synchronous request and was blocking ServeDAPCodec loop, which means the response message from the client couldn't be read until onLaunchRequest returns. This PR adds two goroutines - one to handle requests from the client (loopHandleRequests), and another to handle responses (loopHandleResponses). serveDAPCodec will keep read DAP messages from the net.Conn, but delegate message handling to the goroutines through buffered channels. Alternatively, I tried to avoid introducing goroutines by making onLaunchRequest asynchronously complete the launch if integrated or external console mode is chosen. I.e., make onLaunchRequest return without sending LaunchResponse, free the ServeDAPCodec loop, and return LaunchResponse (or ErrorResponse) when RunInTerminal response is received. But it was hard to follow, didn't look like a typical go program, and wasn't extensible when DAP adds more reverse requests or we ever want to utilize RunInTerminal while handling other requests. For reverse requests, we maintain a pendingReverseReq map for requests sent to the client. When response messages arrive we look up the map, and notify the waiters using a buffered channel. onLaunchRequest uses RunInTerminal if the launch request has "integrated" or "external" console attribute and asks the client to run a bogus command - this is for testing. The follow up PRs will implement the real command that starts a delve and use the command from the integrated or external terminal.
This command is a helper command to be used by dlv dap when launch request with a console property (integrated, external) is received. The dlv dap server then asks the client to run this command in the integrated or external terminal using the RunInTerminal request and turns itself into a proxy mode that forwards messages between the client and the dap-reverse command run by the editor. The dap-reverse command is similar to the dap command, except that instead of opening a port and running as a server listening on the port, this command dials to the supplied address (the rendezvous port setup by the dlv dap server operating in proxy mode). Once the dlv-reverse command is connected, the dlv dap server will forward all the messages from the client (including the initialize request and the launch request) and relay all the responses from the dlv-reverse back to the client. This command is internal use only, so it's intentionally hidden from users - dlv usage and manual will not display info about this. Update golang/vscode-go#124
This PR adds support for reverse requests that are sent from the debug adapter to the client. Currently, RunInTerminal is the only such reverse request. This is a pre-work for the 'console' support (golang/vscode-go#124) - we plan to use RunInTerminal to ask the editor to run a command in the integrated or external terminal while handling a launch request with the console attribute. Launch request was classified as a synchronous request and was blocking ServeDAPCodec loop, which means the response message from the client couldn't be read until onLaunchRequest returns. This PR adds two goroutines - one to handle requests from the client (loopHandleRequests), and another to handle responses (loopHandleResponses). serveDAPCodec will keep read DAP messages from the net.Conn, but delegate message handling to the goroutines through buffered channels. Alternatively, I tried to avoid introducing goroutines by making onLaunchRequest asynchronously complete the launch if integrated or external console mode is chosen. I.e., make onLaunchRequest return without sending LaunchResponse, free the ServeDAPCodec loop, and return LaunchResponse (or ErrorResponse) when RunInTerminal response is received. But it was hard to follow, didn't look like a typical go program, and wasn't extensible when DAP adds more reverse requests or we ever want to utilize RunInTerminal while handling other requests. For reverse requests, we maintain a pendingReverseReq map for requests sent to the client. When response messages arrive we look up the map, and notify the waiters using a buffered channel. onLaunchRequest uses RunInTerminal if the launch request has "integrated" or "external" console attribute and asks the client to run a bogus command - this is for testing. The follow up PRs will implement the real command that starts a delve and use the command from the integrated or external terminal.
@asilverman this works for me as far as auto launching dlv dap, but I did have to update the tasks.json label value to match the preLaunchTask value. I get a warning about no problem matcher being defined. Do you have a separate $go problemMatcher? Not sure how to quell that warning or if it matters. |
Still we don't use RunInTerminal yet. Run all debug tests with `console: "integrated"` setting. If tests are too expensive, I will consider to run only a subset of debug tests with the `console` property. Updates #124 Change-Id: Iefa48efda2a051795d5462c7f1a81d43a8155e91 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/358516 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org> TryBot-Result: kokoro <noreply+kokoro@google.com>
Do not start dlv-dap until we receive a message from the client and have to send a message to the dlv-dap. This is because we want to start dlv dap through RunInTerminal for console:integrated/external and we cannot know whether client-side session setup (including handling RunInTerminal request) is ready until the first message from the client (i.e. the `initialize` request) arrives. Updates #124 Change-Id: Ia9ce375bb07d852f705b0f95eb0ceb380521ef01 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/358536 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org>
Implementation is done by taking advantage of VS Code's RunInTerminal implementation. Upon receiving a `initializeRequest` with console=integrated/external request, the thin debug adapter (DelveDAPOutputAdapter) starts a temporary TCP listener and sends a RunInTerminal request to start the `dlv dap --client-addr` command from the specified terminal. The `dlv dap` command will connect to the TCP listener, and then DelveDAPOutputAdapter starts to forward messages between VS Code and the dlv dap process running in the terminal. Strictly speaking, sending a RunInTerminal request before `initializeResponse` is incorrect. According to DAP spec: ``` Until the debug adapter has responded to with an ‘initialize’ response, the client must not send any additional requests or events to the debug adapter. In addition the debug adapter is not allowed to send any requests or events to the client until it has responded with an ‘initialize’ response. ``` More correct ways are either 1) do not depend on RunInTerminal, but directly manage terminals using vscode terminal APIs, or - we cannot utilize the RunInTerminal implementation vscode team actively manages and keeps improving. - there is no public vscode API for external console mode support so we need to implement it from scratch. 2) first respond with an `initialize` response specifying a minimal capability before sending RunInTerminal request. Once `dlv dap` starts, forward the stashed `initialize` request to `dlv dap`. When `dlv dap` returns an `initialize` response, compute the capability difference and report them as an Capabilities event. - add complexity to the DelveDAPOutputDebugAdapter, which will no longer be a thin adapter. - need significant changes in testing we built using DebugClient. - what capabilities changed through Capabilities events are honored is also unspeficied in the spec. 3) Delve DAP implements a proper Debug Adapter including reverse requests. This is the most ideal solution that benefit other DAP-aware editors. - need to agree with delve devs, so may need more time and discussion. 4) ask the vscode team if debug's RunInTerminal capability can be exposed as a vscode API. If that's possible, we even can do it before the initialize request and set up this from the factory or tracker. - new API. no guarantee if this can be accepted. Will investigate the possibility of 4 or 1. If not working, we will work harder to convince delve devs with option 3. Updates #124 Change-Id: I3d28356a3da99832785815f8af43f7443acf8863 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/358618 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org>
When spawning delve, the extension wants to run it in the environment computed by merging 1. process.env 2. go.toolsEnvVars from settings.json 3. envFile from launch.json 4. env from launch.json This is true for both when we use legacy debug adapter and dlv dap mode. For some time, we did this environment variable merging from goDebugConfigurationProvider. Recently in https://go-review.googlesource.com/c/vscode-go/+/349749, we changed this logic for dlv dap mode - goDebugConfigurationProvider merges only 3 and 4 and stuff the result to the launch configuration. - from DelveDAPOutputAdapter (thin adapter), we merge 1, 2, and the info from the launch configuration (3, 4) and use the result when spawning the `dlv dap` process. We made the change because including 1 and 2 in the launch configuration resulted in transmitting all the process.env over the wire as launch parameter unncessarily, and that made users reluctant to share their debug traces or, made users accidentally share too much info including sensitive information. Now with the changes to support the `console` mode, the extension delegates spawning of `dlv dap` to the VS Code using VS Code's RunInTerminal implementation. Before this change, we sent environment variables merging all 1, 2, 3, and 4 as RunInTerminal's env property. It turned out VS Code's RunInTerminal formulates the final command to run using the `env` command (in Unix) and lists all the environment variables. As a result, including process.env (1) in the env property is problematic: - The terminal is crowded and hurts user experience. - It may result in a long command and that will suffer from problems like microsoft/vscode#134324 Usually, users add only a handful of extra environment variables in the launch configuration and the go.toolsEnvVars configuration. And most likely, the terminal will inherit most of the process.env so we don't need to carry them around. This change thus skips process.env in RunInTerminal's env property, but includes only 2, and 3, 4 from goDebugConfigurationProvider. We still need to merge 1 when the extension spawns 'dlv dap' without RunInTerminal mode, because Node.JS child_process requires all env variables should be included. Updates #124 Change-Id: I33ad5490c6d11a3afb0e569563689be08aefcd06 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/361101 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org>
Currently we still clarify that they are "experimental". Updates #124 Updates #558 Change-Id: I32af4fa0e3cfc73c80bdcdbd22db51c31ccce77c Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/374134 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org>
Major language extensions like vscode-python, vscode-cpptools, vscode-js-debug, vscode-java-debug all use internalConsole/integratedTerminal/externalTerminal as accepted values and translate them to RunInTerminal parameters. Let's follow the convention. Updates #124 Change-Id: Ie2d3c89487ed05a62d6108e514e985c132140b74 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/374135 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org> TryBot-Result: kokoro <noreply+kokoro@google.com>
The golang.go-nightly released today 2022.1.505 allows users to set
Feedback is welcome. |
@hyangah Great to see progress on this feature! Here is my feedback: I'm trying to debug using the following config: ==launch.json== {
"version": "0.2.0",
"configurations": [
{
"name": "Launch Package",
"type": "go",
"request": "launch",
"mode": "auto",
"program": "${fileDirname}",
"console": "externalTerminal"
}
]
} ==settings.json== {
"terminal.external.windowsExec": "cmd.exe /k \"C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\BuildTools\\Common7\\Tools\\VsDevCmd.bat\""
} When launching a debug session the external console appears on the screen but it does not parse the arguments correctly: 'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
Press any key to continue . . . On the other hand, it works well if I use the ==launch.json== {
"version": "0.2.0",
"configurations": [
{
"name": "Launch Package",
"type": "go",
"request": "launch",
"mode": "auto",
"program": "${fileDirname}",
"console": "integratedTerminal"
}
]
} ==settings.json== {
"terminal.integrated.profiles.windows": {
"msvc": {
"path": ["cmd.exe"],
"args": [
"/k",
"C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\BuildTools\\Common7\\Tools\\VsDevCmd.bat"
]
}
},
"terminal.integrated.defaultProfile.windows": "msvc",
} |
@qmuntal Many thanks for the feedback. The error message indicates the vscode's request to process the arg and run the batch failed. Even the command requested using DAP runInTerminal wasn't tried. I think this is the same issue as microsoft/vscode#57452 |
I'm using this property for a project, and it's a lifesaver. Great stuff! |
Its almost the same issue but for the integrated console instead of the external one. I've created a new issue with this finding: microsoft/vscode#140974 |
@gustavomassa Thanks for the report. What is the error message the 'external' terminal shows? It looks like |
@hyangah |
@gustavomassa Is |
@hyangah I don't know if you changed anything on the extension/code, but it's working fine now One thing to notice is that the terminal is automatically closed when the program execution ends, can we change this behavior to wait for user input before closing the terminal? For example, if we have an error that terminates the program, the terminal is closed before we can read the error |
This is done. The only remaining task is probably an entry in FAQ. @gustavomassa thanks for following up.
On my Linux, the last command is |
Change https://go.dev/cl/406295 mentions this issue: |
Please find the instruction in https://github.com/golang/vscode-go/wiki/debugging#handling-stdin |
From microsoft/vscode-go#843
This feature request has a lot of upvotes microsoft/vscode-go#843, please follow the discussion there for more context and user scenarios where this feature would be relevant
The text was updated successfully, but these errors were encountered: