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.

Sign up for GitHub

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

11685: Restore cancellation token behavior #11693

Merged
merged 1 commit into from
Sep 27, 2022
Merged

11685: Restore cancellation token behavior #11693

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions packages/core/src/common/message-rpc/rpc-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,23 +132,23 @@ export class RpcProtocol {
// The last element of the request args might be a cancellation token. As these tokens are not serializable we have to remove it from the
// args array and the `CANCELLATION_TOKEN_KEY` string instead.
const cancellationToken: CancellationToken | undefined = args.length && CancellationToken.is(args[args.length - 1]) ? args.pop() : undefined;
if (cancellationToken && cancellationToken.isCancellationRequested) {
return Promise.reject(this.cancelError());
}
Comment on lines 133 to -137
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the comment above about subbing in a magic string for the cancellation token itself, it looks like we don't pass the state of token - whether it's already been cancelled or not - to the receiving end. If that's the case, then by removing this block, we remove handling the case in which the token has already been cancelled when we get here, since I don't believe that it will fire the onCancellationRequested event again. In that case, should we send the cancellation notice immediately after sending the request itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Yes, sending the cancellation notice directly after sending the request sounds reasonable to me.

Copy link
Contributor Author

@tortmayr tortmayr Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colin-grant-work After investigating and testing this it turns out that both main CancellationToken implementations (Theia and vscode-languageserver) already have a special shortcut event in place that will trigger newly registered cancellation listeners on already canceled tokens immediately. So the cancellation notice for early canceled tokens is already passed correctly to the receiving side.

However, just to be on the safe side (and since in theory also custom cancellation token implementations could be used) I still added the discussed behavior. Directly after sending the request we check whether the token is already canceled. If so, send the cancellation notice, otherwise listen on the onCancellationRequested event.


if (cancellationToken) {
args.push(RpcProtocol.CANCELLATION_TOKEN_KEY);
cancellationToken.onCancellationRequested(() => {
this.sendCancel(id);
this.pendingRequests.get(id)?.reject(this.cancelError());
}
);
}

this.pendingRequests.set(id, reply);

const output = this.channel.getWriteBuffer();
this.encoder.request(output, id, method, args);
output.commit();

if (cancellationToken?.isCancellationRequested) {
this.sendCancel(id);
} else {
cancellationToken?.onCancellationRequested(() => this.sendCancel(id));
}

return reply.promise;
}

Expand All @@ -164,12 +164,6 @@ export class RpcProtocol {
output.commit();
}

cancelError(): Error {
const error = new Error('"Request has already been canceled by the sender"');
error.name = 'Cancel';
return error;
}

protected handleCancel(id: number): void {
const cancellationTokenSource = this.cancellationTokenSources.get(id);
if (cancellationTokenSource) {
Expand Down