-
Notifications
You must be signed in to change notification settings - Fork 138
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
Support cancellation #76
Comments
Sounds like a reasonable approach to me. Couple of semantics questions:
The simplest approach would likely be to have the If we wanted to better support the second case, the adapter could provide a list of cancellable request types in the |
When it comes to semantics, .NET and WinRT async cancellation design could provide some guidelines, since both are well-tested and proven in production for many varied scenarios. They both handle race conditions by treating "cancel" as "stop doing this ASAP, and I don't care about the resulting state so long as it's valid" (which is how it's normally implemented in practice). From that perspective, if the request completes before cancellation could be received or processed, the cancellation was successful. |
The one weird part about making this a 'request' (instead of a new message type of 'cancel') is that doesn't that mean that a DA should both respond to the original request and the cancel request? A bit minor, but seems slightly weird to me. In the case that a client started an evaluation, and then wanted to continue the program before the evaluation finished, would you think that the client should wait for a response to the 'cancel' request before sending the continue request? Or will it be the same except now there will just be the extra 'cancel' request sent right before the 'continue'? Is VS Code planning to offer some sort of cancel button in the repl window? If so, we might want to also extend the evaluate/variables request to know that a debug adapter doesn't need to timeout the evaluation. We extended these requests in VS to add a timeout argument for this purpose. I feel like if we go with this proposal so that 'cancel' messages have a response, then the client should always ignore the response since I don't know how useful that would be. Andrew, did you have a scenario in mind for what a client would do with knowing which requests were cancelable? I can think of one scenario (see below) but I am not sure that I think it is important enough. Here is the one scenario I thought of: suppose a debugger repl window had a button that allowed cancellation. It might want to hide this button unless cancellation was actually available. But a debug adapter might want to set this because they can cancel their stack walks (since those can be slow too) even if it can't abort evaluations. |
As I understand it: The "cancel" response is sent immediately, and indicates whether cancellation is being attempted or not - e.g. if that particular request is not cancelable, then the adapter can immediately tell in the response, and the IDE can display the corresponding error, letting the user know. The response to the original request is sent only once cancellation actually succeeds (or the request completes). So in the "variables" followed by "continue" scenario, the request would send "cancel" and wait for the response to it. If the response is successful, it would then wait for completion of "variables" (disregarding any failures), and only after that it would issue "continue". OTOH, if "cancel" itself returns failure, then that would be immediately shown to the user, along the lines of, "Cannot continue because variable expansion is still in progress". As far as UI, it feels like anything to do with evaluation, except hovers, should have some kind of explicit cancellation UI? It's not just the REPL - Watch is another example where this is desirable. Really, anything that already has a spinner in it to indicate a long-running operation could accommodate a "Cancel" button right next to that spinner (or perhaps combined with it). And then, of course, you'd want the client to have some way to know if it should show that button in all those cases. However, perhaps this is better handled on a per-request level? i.e. add another request, "canCancel", that allows the client to query whether a particular request (by seq) is cancelable or not. If it's a cancelation scenario that requires UI, the client can issue that request and adjust the UI accordingly. If there's no UI, it would just use "cancel" right away. The "supportsCancelRequest" capability is still necessary in this case, but it would only reflect the overall support for these two messages. |
Thanks for the feedback! Here are additional details about the design decisions: Cancellation is a hint to the DA that the frontend is no longer interested in the result for a specific request. The purpose of DAP cancellation is not to become a visible end user feature: there are no cancel buttons. It should work behind the scenes to make the overall debugging experience more fluid by freeing the DA from preparing no longer needed results. Cancellation is a "best effort" type of request: using it doesn't guarantee that it actually has an effect. Answers for the questions raised in the feedback from above:
Based on the feedback I've tried to put more details into the spec. Proposal 2: We introduce a new "CancelRequest" that cancels a pending request specified by a request ID: /** Cancel request; value of command field is 'cancel'.
The 'cancel' request is used by the frontend to indicate that it is no longer interested in the result produced by a specific request issued earlier.
This request has a hint characteristic: a debug adapter can only be expected to make a 'best effort' in honouring this request but there are no guarantees.
The 'cancel' request may return an error if it could not cancel an operation but a frontend should refrain from presenting this error to end users.
A frontend client should only call this request if the capability 'supportsCancelRequest' is true.
The request that got canceled still needs to send a response back.
This can either be a normal result ('success' attribute true) or an error response ('success' attribute false and the 'message' set to 'cancelled').
Returning partial results from a cancelled request is possible but please note that a frontend client has no generic way for detecting that a response is partial or not.
*/
export interface CancelRequest extends Request {
// command: 'cancel';
arguments?: CancelArguments;
}
/** Arguments for 'cancel' request. */
export interface CancelArguments {
/** The ID (attribute 'seq') of the request to cancel. */
requestId?: number;
}
/** Response to 'cancel' request. This is just an acknowledgement, so no body field is required. */
export interface CancelResponse extends Response {
} If a request was cancelled, its response reflects this in a false /** Response for a request. */
export interface Response extends ProtocolMessage {
// ...
/** Outcome of the request.
If true, the request was successful and the 'body' attribute may contain the result of the request.
If the value is false, the attribute 'message' contains the error in short form and the 'body' may contain additional information (see 'ErrorResponse.body.error').
*/
success: boolean;
// ...
/** Contains the raw error in short form if 'success' is false.
This raw error might be interpreted by the frontend and is not shown in the UI.
Some predefined values exist.
Values:
'cancelled': request was cancelled.
etc.
*/
message?: string;
// ...
} A DAP client should only use a "CancelRequest" if the corresponding capability /** Information about the capabilities of a debug adapter. */
export interface Capabilities {
// ...
/** The debug adapter supports the 'cancel' request. */
supportsCancelRequest?: boolean;
} @andrewcrawley @gregg-miskelly @int19h If there are no objections, I plan to release the addition this week. |
@weinand I think the current version is good, but I do have some questions on the semantic of messages:
maybe it could be something as:
|
Currently there is no way for a DAP client to notify a debug adapter to cancel a long running request.
Example:
Proposal:
We introduce a new "CancelRequest" that cancels a pending request specified by a request ID:
A DAP client will only use a "CancelRequest" if the corresponding capability
supportsCancelRequest
returns true:/cc @andrewcrawley @gregg-miskelly please let us know what you think.
The text was updated successfully, but these errors were encountered: