microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.2k stars 29.29k forks source link

Throwing exceptions across extension host RPC boundaries doesn't preserve Error.name #156281

Open AddieCohen opened 2 years ago

AddieCohen commented 2 years ago

Does this issue occur when all extensions are disabled?: No

Steps to Reproduce:

  1. Clone https://github.com/AddieCohen/auth-provider-error
  2. npm install & run the extension.
  3. Run the command "Login with Azure DevOps"
  4. Check the debug console, the error name property is "Error" instead of LoginTimeOutError.

The line in: https://github.com/microsoft/vscode/blob/2980862817f29911a2231f4c88bfc783bd763cab/src/vs/workbench/services/extensions/common/rpcProtocol.ts#L428 doesn't seem to work to preserve the exception name.

alexdima commented 2 years ago

Unfortunately we cannot serialize/deserialize arbitrary Error subclasses across RPC boundaries. Having a certain subclass of Error does not automatically lead to the .name property to be set to the subclass name. e.g.

image

If you would manually set the .name of your error, then that would be serialized/deserialized, but the instanceof will always point to the generic Error because we can't serialize/deserialize arbitrary subclasses.

segevfiner commented 2 years ago

There is a line in the RPC code that tries to preserve the name property but it isn't working, see the issue description.

alexdima commented 2 years ago

@segevfiner Please see the screenshot I posted and note how the name property is always Error even for a custom error class that subclasses Error

segevfiner commented 2 years ago

Exactly. See the issue description, this line tries to preserve it: https://github.com/microsoft/vscode/blob/2980862817f29911a2231f4c88bfc783bd763cab/src/vs/workbench/services/extensions/common/rpcProtocol.ts#L428

But it doesn't work. Probably an Object.defineProperty blocking changing it?

alexdima commented 2 years ago

@segevfiner The OP is doing debugging and is under the impression that the debugger displays the .name property of an error in the debugger. But that is false, the debugger displays the ctor name.

The following code snippet demonstrates that simply subclassing from Error does not define the .name property:

export class LoginTimeOutError extends Error {
    constructor(message?: string) {
        super(message);
    }
}
const y = new LoginTimeOutError();
console.log(y.name); // prints Error

In order for the .name property to be changed, a manual step must be taken by whomever is writing the custom class e.g.:

export class LoginTimeOutError extends Error {
    constructor(message?: string) {
        super(message);
        this.name = 'LoginTimeOutError';
    }
}
const y = new LoginTimeOutError();
console.log(y.name); // prints LoginTimeOutError

So the problem here is that the OP and you believe that there is a problem with the RPC code, but the RPC code is doing its job just fine, it serializes the name Error and restores the name Error. The root problem is that the initial error doesn't have a custom name.

segevfiner commented 2 years ago

This could be it. Do we have any docs about this? If we don't, can we improve the documentation so it's obvious that this is how this should be used?

alexdima commented 2 years ago

PR to improve docs are always welcome. @jrieken do you have any ideas where we could document how errors are transported across processes?