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

Implement safe plugin uninstallation #11084

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Apr 27, 2022

What it does

This PR implements several things:

  1. Deployment of folders to a temp dir, with a CLI option to prevent that copying.
    • Install some unzipped plugins in your ~/.theia/extensions directory
    • Observe that during startup, messages are logged about copying those plugins.
    • Use the --uncompressed-plugins-in-place flag in the CLI
    • Observe that you don't get those startup messages.
  2. Uninstallation without undeployment
    • Install some user plugins
    • Open more than one instance of Theia on the same backend
    • Open the plugin view in one instance
    • Uninstall one of your user plugins
    • Observe that the plugin's features continue to work in all instances.
    • Observe that in all instances targeting that backend, that plugin now shows a button saying 'Reload Required'
    • Click that button in one or more instances.
    • Observe that when the instances reload, the plugin is not loaded.
  3. Multiple version handling
    • Follow the same steps as (2) up to 'Uninstall one of your user plugins'
    • After uninstalling the plugin, use 'Install from VSIX' to install a different version of that plugin
    • Observe that you still have the 'reload required' button
    • Click it
    • When that instance reloads, it should have access to the new version of the plugin.
  4. Reimplementation of Use same tempdir for user and system compressed plugin files #10951

How to test

Review checklist

Reminder for reviewers

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

However, I noticed something which didn't work as described in the testing steps. "Installing" an extension by unzipping it into ~/.theia and starting the app did not copy the extension into the /tmp/vscode-unpacked directory (with and without the --uncompressed-plugins-in-place flag). It did work for zipped (.vsix) extensions as expected. Is that something I got incorrect?

Aside from that I noticed that every uninstalled extension now requires to reload before disappearing completely. While this is definitely an improvement to the way it worked previously, vscode seems to be able to decide on a case-by-case basis whether uninstalling an extension requires an app reload. But this is probably something for another PR :)

packages/plugin-ext/src/common/plugin-protocol.ts Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extension.tsx Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extension.tsx Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extension.tsx Outdated Show resolved Hide resolved
packages/vsx-registry/src/browser/vsx-extension.tsx Outdated Show resolved Hide resolved
@colin-grant-work
Copy link
Contributor Author

@msujew, thanks for taking a look!

However, I noticed something which didn't work as described in the testing steps. "Installing" an extension by unzipping it into ~/.theia and starting the app did not copy the extension into the /tmp/vscode-unpacked directory (with and without the --uncompressed-plugins-in-place flag). It did work for zipped (.vsix) extensions as expected. Is that something I got incorrect?

It should be copying all directories from the user folder. You said you unzipped into the ~/.theia/extensions (or just ~/.theia?) directory; what was your exact procedure?

Aside from that I noticed that every uninstalled extension now requires to reload before disappearing completely. While this is definitely an improvement to the way it worked previously, vscode seems to be able to decide on a case-by-case basis whether uninstalling an extension requires an app reload. But this is probably something for another PR :)

I have an idea for this, but it's harder to guarantee on the backend than the frontend, so I've left it for later :-).

@colin-grant-work colin-grant-work force-pushed the local/clean-plugin-uninstallation branch from 96b3fa2 to 7a59699 Compare May 17, 2022 18:23
@alvsan09
Copy link
Contributor

alvsan09 commented May 17, 2022

It should be copying all directories from the user folder. You said you unzipped into the ~/.theia/extensions (or just ~/.theia?) directory; what was your exact procedure?

@colin-grant-work There is a typo on the instructions at the top, it should be ~/.theia/extensions as you just mentioned rather than ``~/.theia/plugins`

@colin-grant-work
Copy link
Contributor Author

It should be copying all directories from the user folder. You said you unzipped into the ~/.theia/extensions (or just ~/.theia?) directory; what was your exact procedure?

@colin-grant-work There is a typo on the instructions at the top, it should be ~/.theia/extensions as you just mentioned rather than ``~/.theia/plugins`

Good catch. I've also fixed this behavior. @msujew, it looks like during a rebase, the actual call to copy VSCode plugins got deleted; I've brought it back in this commit

Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

I have tested the following scenarios and they work fine! 👍

  1. Tested deployment of folders to a 'tmp' directory
  2. Tested loading of plugin folders in place via --uncompressed-plugins-in-place flag
  3. Uninstallation without undeployment for .vsix
  4. Uninstallation without undeployment for uncompressed folder extensions
  5. Uninstalling and installing a different version

I am not too familiar with the code base, so I will let more experienced committers in the area to cover it.
I found a typo and mentioned it as an inline comment.

@colin-grant-work
Copy link
Contributor Author

@tsmaeder, it might be good to have a Red Hat perspective on this, since it makes substantial changes to the way we pass around data about plugins - changing and making explicit expectations about whether versions are included as part of identifiers - and adds some state that we didn't have before. Want to make sure that we don't blow up Che, if they're still updating the version of Theia they offer.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me 👍

I confirmed that:

  • extensions can be searched.
  • extensions can be installed.
  • extensions can be uninstalled.
  • uninstalled extensions require a reload before they are removed.
  • upon reloading the application extensions can be re-installed.
  • if an extensions is uninstalled, multiple clients will be notified and display reload required.
  • if an extension is uninstalled, and installed through install from vsix the proper extension versions is used on reload.
  • deployment of extensions works well.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Reconfirming my approval. With the latest changes putting an unzipped extension into the ~/.theia/plugins folder copies it to the /tmp/vscode-copied directory and activates it correctly. The behavior for the flag also works as expected 👍

@colin-grant-work
Copy link
Contributor Author

@tsmaeder, last call here. I'll merge tomorrow if there are no objections.

@@ -24,7 +24,7 @@ import { CancellationToken, CancellationError, cancelled } from './cancellation'
*/
export class Deferred<T = void> {
state: 'resolved' | 'rejected' | 'unresolved' = 'unresolved';
resolve: (value: T) => void;
resolve: (value: T | PromiseLike<T>) => void;
Copy link
Member

@paul-marechal paul-marechal Jun 14, 2022

Choose a reason for hiding this comment

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

I agree that Deferred should support resolving to promises, but this introduces a bug where we may resolve to an unresolved PromiseLike but we will mark the Deferred as resolved too early.

Consider the following change:

export class Deferred<T = void> {
    state: 'resolved' | 'rejected' | 'unresolved' = 'unresolved';
    resolve: (value: T | PromiseLike<T>) => void;
    reject: (reason?: unknown) => void;

    promise: Promise<T> = new Promise<T>((resolve, reject) => {
        this.resolve = resolve;
        this.reject = reject;
    }).then(
        value => { this.state = 'resolved'; return value; },
        error => { this.state = 'rejected'; throw error; }
    );
}

Copy link
Member

@paul-marechal paul-marechal Jun 14, 2022

Choose a reason for hiding this comment

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

On the other hand I could be misunderstanding the role of the state property?

Is it meant to represent when resolve has been called, or when the underlying promise resolves? Doesn't look obvious.

@colin-grant-work colin-grant-work force-pushed the local/clean-plugin-uninstallation branch from c140b15 to fc7ae12 Compare June 15, 2022 16:12
@colin-grant-work
Copy link
Contributor Author

Thanks everyone who reviewed!

@colin-grant-work colin-grant-work merged commit bfa1b44 into eclipse-theia:master Jun 15, 2022
@colin-grant-work colin-grant-work deleted the local/clean-plugin-uninstallation branch June 15, 2022 16:48
@tsmaeder
Copy link
Contributor

@colin-grant-work would have loved to chime in, but no time, unfortunately 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants