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

Support cancellation #76

Closed
weinand opened this issue Sep 5, 2019 · 6 comments
Closed

Support cancellation #76

weinand opened this issue Sep 5, 2019 · 6 comments
Assignees
Labels
feature-request Request for new features or functionality verified Verification succeeded

Comments

@weinand
Copy link
Contributor

weinand commented Sep 5, 2019

Currently there is no way for a DAP client to notify a debug adapter to cancel a long running request.

Example:

  • the UI frontend sends out a variables request which takes a long time to complete.
  • user sees the spinning in the variables view and presses "Step over".
  • this action should cancel the original variables request, however currently there is no way for UI frontend to tell the debug adapter to stop what it is currently doing.

Proposal:

We introduce a new "CancelRequest" that cancels a pending request specified by a request ID:

/** Cancel request; value of command field is 'cancel'.
	This request can be used to cancel another request. Clients should only call this request if the capability 'supportsCancelRequest' is true.
	A request that got canceled still needs to send a response back. This can either be a partial result or an error response.
*/
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 {
}

A DAP client will only use a "CancelRequest" if the corresponding capability supportsCancelRequest returns true:

/** Information about the capabilities of a debug adapter. */
export interface Capabilities {

	// ...

	/** The debug adapter supports the 'cancel' request. */
	supportsCancelRequest?: boolean;
}

/cc @andrewcrawley @gregg-miskelly please let us know what you think.

@weinand weinand added the feature-request Request for new features or functionality label Sep 5, 2019
@weinand weinand added this to the September 2019 milestone Sep 5, 2019
@weinand weinand self-assigned this Sep 5, 2019
@andrewcrawley
Copy link

Sounds like a reasonable approach to me.

Couple of semantics questions:

  • What about race conditions, e.g. trying to cancel a request that has already completed, but the client hasn't received the response yet?
  • What should a debug adapter do if it supports cancelling some operations, but not others? This would likely be the case for vsdbg.

The simplest approach would likely be to have the cancel response indicate failure in both of these cases, with an appropriate message.

If we wanted to better support the second case, the adapter could provide a list of cancellable request types in the initialize response instead of just a supportsCancelRequest bool. This list could then be used by a debugger UI to determine which operations should surface a cancellation action and which shouldn't.

@int19h
Copy link

int19h commented Sep 7, 2019

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.

@gregg-miskelly
Copy link
Member

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.

@int19h
Copy link

int19h commented Sep 7, 2019

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.

@weinand
Copy link
Contributor Author

weinand commented Sep 10, 2019

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:

  • Since a 'cancel' request is a hint, I don't think that a DAP client will ever wait for the 'cancel' request's response.
  • Trying to cancel a request that has already completed, but the client hasn't received the response, might result in an error of the 'cancel' request because it can no longer find the request to cancel (the implementation can decide what to do in this case). But for VS Code I cannot imagine that the result of the 'cancel' request is actually checked anywhere. We will definitely not show an error notification to the user.
  • What should a debug adapter do if it supports cancelling some operations, but not others? The "cancel" request would be a "no-op" for those operations that cannot be cancelled. The "cancel" request might return an error in these cases but I don't think that the frontend will make use of this information. Having more capabilities for individual operations sounds like an overkill to me.
  • We did not put the 'cancel' request in its own message category because 'cancel' is a request like any other request: it receives arguments and returns results. I do not see a benefit from having 'cancel' in its own message category.

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 success attribute and the value cancelled in the message attribute:

/** 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 supportsCancelRequest returns true:

/** 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.

@fabioz
Copy link

fabioz commented Sep 11, 2019

@weinand I think the current version is good, but I do have some questions on the semantic of messages:

  1. It's not really clear to me what does it mean to return success=True for a cancel request... I'm under the impression that it's possible to return success=True and not have the message cancelled (due to the cancel being just a hint), but I'm not completely sure about that from the description.

  2. If partial results are ok, instead of saying:

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.

maybe it could be something as:

Returning partial results from a cancelled request is possible, so, the frontend client should usually disregard the results from any cancelled request as there's no generic way for detecting that a response is partial or not after the cancellation was requested.

@isidorn isidorn added the verified Verification succeeded label Oct 2, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants