-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
service/dap: Add error handlers for unsupported and not-yet-supported requests #1918
Conversation
service/dap/server_test.go
Outdated
} | ||
} | ||
|
||
client.InitializeRequest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be taken out once #1914 goes in, so the request numbers are treated as 1-based without changing all the constants below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1914 was merged.
service/dap/server.go
Outdated
@@ -451,6 +453,153 @@ func (s *Server) onContinueRequest(request *dap.ContinueRequest) { | |||
s.doContinue() | |||
} | |||
|
|||
// | |||
// The rest of the handlers below are no-ops because the adaptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the UX implications of this. Presumably controls to send this requests still exist in VSCode's UI, if the adapter does a no-op then the user can select them and receive no feedback? I presume an error response is percolated to the user, is it not? How is no feedback better than seeing an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR only focuses on the requests that have corresponding capabilities flags set or unset/absent at the initialization (see 'supportsFoo' references in the comments for each handler).
From the spec, "the protocol is still at its first version because it was an explicit design goal to support new feature in a completely backward compatible way. Making this possible without version numbers requires that every new feature gets a corresponding flag that lets a development tool know whether a debug adapter supports the feature or not. The absence of the flag always means that the feature is not supported."
I think the idea is that based on these the editor will know when to disable the UI, avoid issuing unsupported requests, etc. That said I did find one example where this was not quite working as intended, but the vscode side was very responsive in fixing this when I flagged it (see microsoft/vscode#90001). But I figured it would be best to just maintain feature parity with the existing implementation and just do what they are doing - make these no-ops. For now, I left out the couple of requests that are also no-ops in vscode-go, but do not have corresponding capability flags to let the tool know explicitly not to rely on them. See also my questions at microsoft/debug-adapter-protocol#99
Also, when I asked my vscode contacts how to decide when a no-op vs an error response is more appropriate, they said "if it is an error that the user should see, then return an ErrorResponse, if it is not an error that the user should see, then don't return an ErrorResponse", which matches your assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird that we care whether or not they return an error if they are disabled by a feature flag, but alright. I think however we should at least put a TODO comment on the ones we want to implement in the future, as opposed to ones that just don't make sense for us (like RestartFrame)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is a bit odd. I am not completely happy with this change myself. I can take the handlers out and trust the development tools not to issue these requests and if they do, just fallback on the default unsupported errors that I had before, maybe differentiating between truly unsupported and not yet supported ones.
Which ones do you think deserve a TODO for future implementation? Here is the full list.
TerminateRequest
RestartRequest <== ?
SetFunctionBreakpointsRequest
StepBackRequest
RestartFrameRequest
SetExpressionRequest <==?
TerminateThreadsRequest
StepInTargetsRequest
GotoTargetsRequest
CompletionsRequest
ExceptionInfoRequest
LoadedSourcesRequest
SetDataBreakpointsRequest
ReadMemoryRequest
DisassembleRequest
CancelRequest
BreakpointLocationsRequest
ModulesRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on a quick look at the spec I think these can be implemented:
TerminateRequest
RestartRequest
SetFunctionBreakpointsRequest
StepBackRequest
SetExpressionRequest
LoadedSourcesRequest
ReadMemoryRequest
DisassembleRequest
and these can not:
RestartFrameRequest
TerminateThreadsRequest
StepInTargetsRequest
GotoTargetsRequest
CompletionsRequest
SetDataBreakpointsRequest
BreakpointLocationsRequest
I'm unsure about:
ExceptionInfoRequest
CancelRequest
ModulesRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am updating the PR based on this discussion. Please stay tuned. In the meantime, you listed StepBack as a request to support. It comes in a pair with ReverseContinue. I see "rev" and "rewind", so this one can also be supported, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds like it could be implemented as well.
I have expanded the scope of this PR (initial comment and title updated). You can now think of the main dispatch as well as the unittests as a sort of roadmap showing which requests are
|
go.mod
Outdated
@@ -21,5 +21,7 @@ require ( | |||
golang.org/x/arch v0.0.0-20190927153633-4e8777c89be4 | |||
golang.org/x/sys v0.0.0-20190626221950-04f50cda93cb | |||
golang.org/x/tools v0.0.0-20191127201027-ecd32218bd7f | |||
gopkg.in/airbrake/gobrake.v2 v2.0.9 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these dependencies keep showing up? As far as I can tell nothing is using them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not add them manually :) I took them out now. I think the previous merge must have not gone right, and they did not get in sync with master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM - please just rebase and we can merge! |
What do you mean by rebase? There should be no merge conflicts with master - I am already up to date with it. Do you want me to squash all the commits, so you can just merge instead of squash and merge? |
My fault, you're right! The GitHub UI made it look like there were actual conflicts to resolve, but squashing will solve it. |
… requests (go-delve#1918) * Add no-op handlers for unsupported requests * Fix whitespace * Add couple of missing unsupported requests * More comments * Separate errors for unsupported and not-yet-implemented requests. * Fix go.mod/sum dependencies Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>
… requests (go-delve#1918) * Add no-op handlers for unsupported requests * Fix whitespace * Add couple of missing unsupported requests * More comments * Separate errors for unsupported and not-yet-implemented requests. * Fix go.mod/sum dependencies Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>
Reflect which requests are required and which ones are optional and controlled by flags (aka capabilities) that can be set in the initialize response to the client. For the optional ones that make sense in the context of Go/delve as well as the missing required handlers, provide stubs that currently just return the not-yet-implemented error response. This is different from vscode-go, which falls back on no-op implementation for all unsupported messages defined in the base classes for debug adaptors (see https://github.com/microsoft/vscode-debugadapter-node/blob/master/adapter/src/debugSession.ts).
Updates #1515