Add CancellationError #10035

mtsmfm · 2021-09-01T14:39:17Z

What it does

Fixes: #9988.

Add CancellationError to be compatible with VSCode.

#9136 (comment)

How to test

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

vince-fugnitto · 2021-09-01T14:45:26Z

@mtsmfm fixes #9988?

mtsmfm · 2021-09-01T14:54:00Z

@vince-fugnitto Sorry I overlooked that issue. Yes, this PR fixes that.

tsmaeder · 2021-09-02T06:42:23Z

@mtsmfm thanks for the PR. How can I test it? Why I'm asking is this: what happens when a plugin actually throws "CancelledError"? I think we should handle that more gracefully than to just display an error. What does VS Code do in this instance?

mtsmfm · 2021-09-02T12:28:37Z

@tsmaeder

What does VS Code do in this instance?

This error was introduced to avoid showing busy error on host log.
microsoft/vscode#93686

I'm not sure the equivalent place for theia so I understand this implementation is not enough though,
it's worth to be merged since this PR can solve plugin initialization error TypeError: Class extends value undefined is not a constructor or null.

#9136 (comment)

Currently Theia can't load plugins which use newer vscode-languageserver package because it extends this class even if a plugin doesn't use this class at all.

https://github.com/microsoft/vscode-languageserver-node/blob/eba6a7308b21ab94bd412fbfa63e36964b6d82ad/client/src/common/client.ts#L2710

microsoft/vscode-languageserver-node@645eb3d

paul-marechal · 2021-09-17T18:40:15Z

@tsmaeder VS Code made a function to intercept some calls and log or not some of the errors:

https://github.com/microsoft/vscode/blob/1.60.1/src/vs/workbench/api/common/extHostLanguageFeatures.ts#L1595-L1624

It is then extensively used, but they only parameterize it in a few places to ignore logging CancellationError:

Since this is just related to logging, I think it is fine to do this as a follow-up.

paul-marechal · 2021-09-17T22:18:07Z

@mtsmfm overall the changes looks ok to me, but do you know which plugin/version I could use to compare the state of master vs your PR? I tried the latest release of rust-analyzer but I got a nasty path bug error on start, which I assume comes from our implementations...

tsmaeder · 2021-09-20T08:59:51Z

@mtsmfm Theia is handling errors on remote calls generally here:

theia/packages/core/src/common/messaging/proxy-factory.ts

Line 247 in 96930a2

.catch((err: any) => reject(this.deserializeError(capturedError, err)))

I agree we should not delay merging this PR because we're not "properly" handling CancellationError because of the following argument: for a plugin that does not use CancellationError, nothing changes. For a plugin that does use cancellation error, we now would get a different error each time it tries to throw CancellationError. So in both cases, we end up with a log full of bogus error messages. So as far as I'm concerned, it passes the "the code is no worse than before" test.

tsmaeder · 2021-09-20T09:15:30Z

The code looks good to me, but I can't provoke the error from #9136 (comment) on master. @vince-fugnitto you had analyzed the problem, right?

vince-fugnitto · 2021-09-20T12:08:52Z

The code looks good to me, but I can't provoke the error from #9136 (comment) on master. @vince-fugnitto you had analyzed the problem, right?

@tsmaeder I don't really remember, in the issue all I did was help identify where CancellationError came from, and how someone might implement it.

mtsmfm · 2021-09-20T13:14:37Z

@paul-marechal @tsmaeder Currently rust-analyzer compiles its extension for node 14, not 12. So latest rust-analyzer doesn't work because theia only supports 12 until #9936

Can you try my fork? https://github.com/mtsmfm/rust-analyzer/releases/tag/nightly

It applies a patch to change target

mtsmfm/rust-analyzer#1

I tried che-theia with this PR and with my fork (https://gist.github.com/mtsmfm/a9b34a4004d20c38caf8e06f7929feb4) and I confirmed Class extends value undefined is not a constructor or null is not appeared.
Without this PR, it'll throw Class extends value undefined is not a constructor or null error.

I got another error now though, it's not related to this PR.

Activating extension 'rust-analyzer (nightly)' failed: The language client requires VS Code version ^1.53.0 but received version 1.50.0

For che-theia, rust-analyzer still doesn't work since its dependency requires vscode ^1.53.0.

https://github.com/rust-analyzer/rust-analyzer/blob/f1d7f98ed07b9934286b9c4809dd4d7a47537879/editors/code/package-lock.json#L3764
https://github.com/microsoft/vscode-languageserver-node/blob/release/client/7.1.0-next.5/client/src/node/main.ts#L25

I think if you can use latest theia, it should work since theia's default is 1.53.2 now.
#9959

For che-theia, I think this patch is needed
eclipse-che/che-theia#1213

mtsmfm · 2021-09-20T13:16:27Z

Or perhaps this release can work without patch since it's the last release which supports node 12

https://github.com/rust-analyzer/rust-analyzer/releases/tag/2021-08-09

tsmaeder · 2021-09-27T08:56:27Z

Luckily, the newest vscode-java uses CancellationError.

tsmaeder

I was able to test with vscode-java v.0.82.0, but we should move the type declaration.

tsmaeder · 2021-09-27T09:03:08Z

packages/plugin-ext/src/plugin/quick-open.ts

@@ -40,6 +40,13 @@ export function isPromiseCanceledError(error: any): boolean {
    return error instanceof Error && error.name === canceledName && error.message === canceledName;
 }

+export class CancellationError extends Error {


I just noticed that this is in quick-open.ts. However, this is a general class and should be moved to types-impl.ts

Let me correct that: we're consuming cancellation-related stuff from core/common/cancellation.ts. We probably put the implementation of the class there, as well.

mtsmfm · 2021-09-27T11:56:14Z

@tsmaeder I've moved class into core/common/cancellation.ts. Can you review again?

tsmaeder

lgtm now

vince-fugnitto added the vscode issues related to VSCode compatibility label Sep 1, 2021

vince-fugnitto requested a review from tsmaeder September 1, 2021 15:06

tsmaeder requested changes Sep 27, 2021

View reviewed changes

Add CancellationError

14d7717

mtsmfm force-pushed the cancellation-error branch from 7fa4af1 to 14d7717 Compare September 27, 2021 11:55

tsmaeder approved these changes Sep 27, 2021

View reviewed changes

tsmaeder merged commit ce996cc into eclipse-theia:master Sep 27, 2021

mtsmfm deleted the cancellation-error branch September 27, 2021 13:11

vince-fugnitto added this to the 1.18.0 milestone Sep 30, 2021

tortmayr mentioned this pull request Jun 9, 2022

Use binary message RPC protocol for plugin API #11261

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CancellationError #10035

Add CancellationError #10035

mtsmfm commented Sep 1, 2021 •

edited by vince-fugnitto

Loading

vince-fugnitto commented Sep 1, 2021

mtsmfm commented Sep 1, 2021

tsmaeder commented Sep 2, 2021

mtsmfm commented Sep 2, 2021 •

edited

Loading

paul-marechal commented Sep 17, 2021 •

edited

Loading

paul-marechal commented Sep 17, 2021

tsmaeder commented Sep 20, 2021

tsmaeder commented Sep 20, 2021

vince-fugnitto commented Sep 20, 2021

mtsmfm commented Sep 20, 2021 •

edited

Loading

mtsmfm commented Sep 20, 2021

tsmaeder commented Sep 27, 2021

tsmaeder left a comment

tsmaeder Sep 27, 2021

tsmaeder Sep 27, 2021

mtsmfm commented Sep 27, 2021

tsmaeder left a comment

Add CancellationError #10035

Add CancellationError #10035

Conversation

mtsmfm commented Sep 1, 2021 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto commented Sep 1, 2021

mtsmfm commented Sep 1, 2021

tsmaeder commented Sep 2, 2021

mtsmfm commented Sep 2, 2021 • edited Loading

paul-marechal commented Sep 17, 2021 • edited Loading

paul-marechal commented Sep 17, 2021

tsmaeder commented Sep 20, 2021

tsmaeder commented Sep 20, 2021

vince-fugnitto commented Sep 20, 2021

mtsmfm commented Sep 20, 2021 • edited Loading

mtsmfm commented Sep 20, 2021

tsmaeder commented Sep 27, 2021

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder Sep 27, 2021

Choose a reason for hiding this comment

tsmaeder Sep 27, 2021

Choose a reason for hiding this comment

mtsmfm commented Sep 27, 2021

tsmaeder left a comment

Choose a reason for hiding this comment

mtsmfm commented Sep 1, 2021 •

edited by vince-fugnitto

Loading

mtsmfm commented Sep 2, 2021 •

edited

Loading

paul-marechal commented Sep 17, 2021 •

edited

Loading

mtsmfm commented Sep 20, 2021 •

edited

Loading