fix: revise vscode extension directories #13178

xai · 2023-12-15T13:11:14Z

What it does

This patch eliminates the use of the temporary directories vscode-unpacked and vscode-copied for installing and deploying vscode extensions. The file-handler and directory handler for vscode extensions have been adjusted accordingly:

A temporary directory is now only used for downloading extensions from the registry. This temporary directory is now user-specific and resides within the configdir (i.e., per default in the user's home: /.theia/tmp/) to avoid clashes and permission issues on multi-user operating systems that share temporary directories, such as Linux or BSDs. Having this temporary directory in a location that is configurable by the user also seems the more sensible approach when extensions are considered confidential data.
$configDir/deployedPlugins replaces our volatile /tmp/vscode-copied deployment directory. Having a more permanent way of handling installed extensions should improve startup time and reduce issues with multiple instances running in parallel.
The local file resolver unpacks the vsix file from the temp dir into $configDir/deployedPlugins/<extension-id>.
The simplified directory handler loads unpacked extensions directly from $configDir/deployedPlugins/<extension-id>.
We use $configDir/extensions as a location for the user to drop vsix files that will be installed to the deployment location automatically on startup. We do not manage or remove files within $configDir/extensions.

Overall, this should improve the stability on systems with shared temp dir locations and reduce the startup of the first application start after a reboot.

Fixes #12757

Contributed on behalf of STMicroelectronics

How to test

Installing from registry

Start Theia
Use the vsix registry to install an extension
Verify that the extension is loaded and its functionality is enabled.
Verify that the extension is permanently stored unpacked in $configDir/deployedPlugins/.
Try starting Theia again to verify the extension gets correctly loaded
Remove the extension (this should indicate that a reload of the window is required), and verify that the extension is no longer active
Verify that $configDir/extensions/ has been removed

Installing a local vsix extensions

Start Theia
Select Install from VSIX in the triple-dot menu of the extensions view
Browse to a local .vsix file and install ut
Verify that the extension is permanently stored unpacked in $configDir/deployedPlugins/.
Remove the extension (this should indicate that a reload of the window is required), and verify that the extension is no longer active
Verify that $configDir/extensions/ has been removed

Store a vsix file in the drop-in location

Put a .vsix file into $configDir/extensions
Start Theia
Verify that the extension is permanently stored unpacked in $configDir/deployedPlugins/.
Removing the extension will cause it to be reinstalled until you remove it from the drop-in location

Follow-ups

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

tsmaeder · 2023-12-15T15:22:57Z

I have a couple of general comments:

We should not download extensions to a shared directory. I have heard of use cases where even the existence of certain extensions may be a trade secret. Isn't there a user-specific tmp on linux?
We should never move or delete files that are managed by the user. So if the user puts a *.vsix into .theia/extensions, we should not move nor delete it from there.
If we download *.vsix files as a side-effect of an installation, we should clean up those files once we no longer need them. Relying on the OS or user to clean up files created by software is bad practice, IMO.
I think it would be a good idea to separate "drop in locations" and "deployment locations". Any given directory should be one or the other. If an application does not have the ability to install additional plugins, we should never deal with drop-in locations. Concretely, this would mean that we have .theia/extensions as a drop-in location where the user can put extensions and .theia/deployedPlugins where stuff is extracted to.

xai · 2023-12-15T17:10:04Z

Thanks, @tsmaeder, for the quick response!

1. We should not download extensions to a shared directory. I have heard of use cases where even the existence of certain extensions may be a trade secret. Isn't there a user-specific tmp on linux?

I don't think there is a consistent user-specific temp dir in Linux. According to man 3 tmpfile:

The  standard  does  not  specify  the  directory  that  tmpfile()  will  use.  Glibc will try the path prefix P_tmpdir defined in <stdio.h>, and if that fails the directory /tmp.

The shared /tmp is often also on a different filesystem/mountpoint than the user home. In several scenarios, the user home will be better secured (e.g., encrypted with user-specific credentials, mounted after kerberos auth, ...) than the shared temp dir location.

In our use case, the download directory is meant to be user-specific (that's why I encoded the username into it), but sure, the default permissions will most likely be world-readable (755) if we do not change them. So I see two options:

Make sure the permission of /tmp/vscode-download-$username is 700
Download to a path within the user home, such as .theia/tmp/download

I think if we want to treat extensions as potential secrets, then the second approach is more sensible, considering the user home to be a much safer place than /tmp. Of course, this not being a temp dir that the OS handles, we have to clean it up ourselves.
So what do you think about <userhome>/.theia/tmp/download-<randomuuid/processid>?

2. We should never move or delete files that are managed by the user. So if the user puts a *.vsix into `.theia/extensions`, we should not move nor delete it from there.

ACK.

3. If we download *.vsix files as a side-effect of an installation, we should clean up those files once we no longer need them. Relying on the OS or user to clean up files created by software is bad practice, IMO.

That makes sense, especially if we decide to use a location within the user's home. We have to be careful here with regard to robustness in case of multiple Theia processes (run by the same user). So, cleaning up after successful unpacking is easy enough. What do we do if the application gets killed hard by the user or the OOM killer and our cleanup is not finished? Should we try to clean at the application start? Then we have to make sure not to remove stuff that another running Theia process is currently using.

4. I think it would be a good idea to separate "drop in locations" and "deployment locations". Any given directory should be one or the other. If an application does not have the ability to install additional plugins, we should never deal with drop-in locations. Concretely, this would mean that we have `.theia/extensions` as a drop-in location where the user can put extensions and `.theia/deployedPlugins` where stuff is extracted to.

Makes sense; this is backward-compatible and avoids unwanted interactions between our unpacking/deployment and the user. I'll revise the code accordingly.

xai · 2023-12-15T22:04:29Z

I revised my PR to use

~/.theia/deployedPlugins/ as a permanent location for installed extensions. This replaces the volatile /tmp/vscode-copied
~/.theia/extensions/ as a drop-in location that the user manages (we do not touch anything there) and that only serves as a source of extensions that will be installed on startup
~/.theia/tmp/ as a replacement for the remaining shared temp directories. It can be used to download .vsix files from the registry (they do not belong in ~/.theia/extensions`, as we no longer manage this directory).

tsmaeder

Mostly comments, but I think we should fix the "move" logic.

tsmaeder · 2023-12-18T09:26:31Z

packages/plugin-ext-vscode/src/common/plugin-vscode-environment.ts

+        if (!this._tmpDirUri) {
+            const configDir: URI = new URI(await this.environments.getConfigDirUri());
+            const processId: string = process.pid.toString();
+            this._tmpDirUri = configDir.resolve('tmp').resolve(`${prefix ?? 'tmp-'}-${processId}`);


Would https://nodejs.org/api/fs.html#fs_fs_mkdtemp_prefix_options_callback do the trick here?

tsmaeder · 2023-12-18T09:30:05Z

packages/plugin-ext-vscode/src/node/local-vsix-file-plugin-deployer-resolver.ts

+            console.log(`[${pluginResolverContext.getOriginId()}]: trying to decompress "${localPath}" into "${tempUnpackDir}"...`);
+            if (await fs.pathExists(tempUnpackDir)) {
+                console.log(`[${pluginResolverContext.getOriginId()}]: Removing existing target dir "${tempUnpackDir}"...`);
+                fs.removeSync(tempUnpackDir);


Why is this sync?

tsmaeder · 2023-12-18T09:32:37Z

packages/plugin-ext-vscode/src/node/local-vsix-file-plugin-deployer-resolver.ts

-            } catch (e) {
-                console.warn(`Problem copying plugin at ${localPath}:`, e);
+            await fs.ensureDir(tempUnpackDir);
+            if (!await decompressExtension(localPath, tempUnpackDir)) {


The path of the vsix and the destination dir are even more important in the case of failure.

tsmaeder · 2023-12-18T09:32:58Z

packages/plugin-ext-vscode/src/node/local-vsix-file-plugin-deployer-resolver.ts

            }
+        } catch (e) {
+            console.error(`[${pluginResolverContext.getOriginId()}]: error while decompressing`, e);


Again: log the relevant paths.

tsmaeder · 2023-12-18T09:41:46Z

packages/plugin-ext-vscode/src/node/local-vsix-file-plugin-deployer-resolver.ts

+            return;
+        }
+        console.log(`[${pluginResolverContext.getOriginId()}]: moving to extension dir "${extensionDeploymentDir}"...`);
+        await fs.move(tempUnpackDir, extensionDeploymentDir, { overwrite: true });


I don't think we should use fs.move here: we're trying to do this replacement atomically (either completely succeed or fail). The safe way to do this is by using rename. But that only works when the source and destination are on the same device. So IMO, we should to this:

Unpack to a tmp-subdir inside extensionDeploymentDir.

If the destination folder already exists, rename it to something else

If that works, rename the tmp folder to the destination folder, otherwise rename the original folder back and fail, cleaning up any new files.

Remove the pre-existing folder, if it exists

packages/plugin-ext-vscode/src/node/plugin-vscode-deployer-participant.ts

tsmaeder · 2023-12-18T10:18:48Z

packages/plugin-ext-vscode/src/node/plugin-vscode-decompress.ts

+        }
+        return true;
+    } catch (error) {
+        console.error(`Failed to decompress ${sourcePath} to ${destPath}: ${error}`);


Why are we not just letting the exception propagate?

Sure, stupid mistake.

tsmaeder · 2023-12-18T10:20:34Z

packages/plugin-ext-vscode/src/node/plugin-vscode-file-handler.ts

+
+            console.log(`[${id}]: decompressed`);
+        } catch (error) {
+            console.error(`[${id}]: error while decompressing`, error);


Why not let the error propagate? How is the invoker going to tell sometihing failed?

tsmaeder · 2023-12-18T10:21:45Z

packages/plugin-ext-vscode/src/node/plugin-vscode-file-handler.ts

+            const extensionDeploymentDirPath = FileUri.fsPath(deployedPluginsDirUri.resolve(pluginId));
+            return extensionDeploymentDirPath;
+        } catch (error) {
+            console.error('Error resolving extension directory:', error);


If the error is unhandled, we'll get a stack trace. If it isn't shouldn't the handler print the error?

packages/vsx-registry/src/node/vsx-extension-resolver.ts

tsmaeder · 2023-12-18T10:47:34Z

Some notes from testing:

Dropping the "vscode-drawio" plugin into the "extensions" folder inside the ~/.theia folder blocks the UI at startup for 20 seconds with a white screen. When we install extensions via the UI, the extensions can startup after being installed, so there's no reason to block while we wait for the plugins to install.
This happens on every startup, not just the first.
After startup, we end up with an "uninstall" button for the extesnsion in the plugins view. When you click it, the extension is uninstalled. However, the extension is reinstalled the next time we start Theia. Maybe we should consider extensions installed via "drop-in" folder as "built-in" and the way to remove them would be to remove the *.vsix from the drop-in folder?

I can kind of live with the second behavior, but Nr. 1 is a must-fix, IMO.

xai · 2023-12-18T21:21:50Z

1. Dropping the "vscode-drawio" plugin into the "extensions" folder inside the `~/.theia` folder blocks the UI at startup for 20 seconds with a white screen. When we install extensions via the UI, the extensions can startup after being installed, so there's no reason to block while we wait for the plugins to install.
   This happens on **every** startup, not just the first.

It should be fixed now. I reverted back to using the filename of a dropped vsix file as extension-id instead of always unpacking and parsing package.json. Would be nicer to have the correct extension id, but we can deal with this in a future PR, I'd be happy to look into this.

2. After startup, we end up with an "uninstall" button for the extesnsion in the plugins view. When you click it, the extension is uninstalled. However, the extension is reinstalled the next time we start Theia. Maybe we should consider extensions installed via "drop-in" folder as "built-in" and the way to remove them would be to remove the *.vsix from the drop-in folder?

Treating them as "built-in" would be in line with the user being the only one who manages them. This would probably also fit well with having a lookup mechanism for extensions because then we could track the source of deployed extensions (e.g., downloaded, dropped-in, installed-from-file) and treat them accordingly.

I can kind of live with the second behavior, but Nr. 1 is a must-fix, IMO.

Nr. 1 should be done now. Nr. 2 is definitely a good candidate for a follow-up ticket.

tsmaeder

Code looks good to me now, but I still get a 20s pause with a blank window the first time I drop the draw.io vsix into the .theia/extensions folder.

tsmaeder · 2023-12-21T09:30:13Z

@xai we have a conflict.

This patch eliminates the use of the temporary directories vscode-unpacked and vscode-copied for installing and deploying vscode extensions. The file-handler and directory handler for vscode extensions have been adjusted accordingly: * A temporary directory is now only used for downloading extensions from the registry. This temporary directory is now user-specific and resides within the configdir (i.e., per default in the user's home: /.theia/tmp/) to avoid clashes and permission issues on multi-user operating systems that share temporary directories, such as Linux or BSDs. Having this temporary directory in a location that is configurable by the user also seems the more sensible approach when extensions are considered confidential data. * $configDir/deployedPlugins replaces our volatile /tmp/vscode-copied deployment directory. Having a more permanent way of handling installed extensions should improve startup time and reduce issues with multiple instances running in parallel. * The local file resolver unpacks the vsix file from the temp dir into $configDir/deployedPlugins/<extension-id>. * The simplified directory handler loads unpacked extensions directly from $configDir/deployedPlugins/<extension-id>. * We use $configDir/extensions as a location for the user to drop vsix files that will be installed to the deployment location automatically on startup. We do not manage or remove files within $configDir/extensions. Overall, this should improve the stability on systems with shared temp dir locations and reduce the startup of the first application start after a reboot. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>

To avoid delaying the application startup while loading plugins, the initialize function of the PluginDeployerContribution class explicitly returns a resolved Promise. Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com> Co-authored-by: Thomas Mäder <t.s.maeder@gmail.com>

Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>

xai · 2023-12-21T09:41:53Z

@xai we have a conflict.

Resolved.

xai force-pushed the issues/12757 branch 2 times, most recently from 84ecd86 to 2850ea3 Compare December 15, 2023 13:37

tsmaeder requested changes Dec 18, 2023

View reviewed changes

xai force-pushed the issues/12757 branch 2 times, most recently from 9cef267 to 87ee02b Compare December 18, 2023 21:14

xai requested a review from tsmaeder December 18, 2023 21:21

xai changed the title ~~fix: use path in user home as extension dir~~ fix: revise vscode extension directories Dec 18, 2023

xai marked this pull request as ready for review December 18, 2023 21:26

xai force-pushed the issues/12757 branch from 87ee02b to cc91418 Compare December 18, 2023 21:33

JonasHelming assigned tsmaeder Dec 20, 2023

tsmaeder reviewed Dec 20, 2023

View reviewed changes

xai force-pushed the issues/12757 branch from 4f0d2b9 to 56a8b4a Compare December 20, 2023 16:28

tsmaeder approved these changes Dec 20, 2023

View reviewed changes

xai and others added 3 commits December 21, 2023 10:33

chore: update CHANGELOG

36505f8

Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>

xai force-pushed the issues/12757 branch from 56a8b4a to 36505f8 Compare December 21, 2023 09:41

tsmaeder merged commit a20db92 into eclipse-theia:master Dec 21, 2023
14 checks passed

vince-fugnitto added this to the 1.45.0 milestone Dec 21, 2023

xai mentioned this pull request Jan 17, 2024

Builtin VSIX plugin is added to the installed list instead of the builtin list #13222

Closed

msujew mentioned this pull request Feb 12, 2024

Uninstall an extension does not deleted the extension from the ".theia/deployedPlugins" folder #13379

Closed

sdirix deleted the issues/12757 branch February 12, 2024 15:09

t1m0thyj mentioned this pull request Jun 26, 2024

Fix Theia integration tests zowe/zowe-explorer-vscode#2979

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: revise vscode extension directories #13178

fix: revise vscode extension directories #13178

xai commented Dec 15, 2023 •

edited

Loading

tsmaeder commented Dec 15, 2023

xai commented Dec 15, 2023

xai commented Dec 15, 2023

tsmaeder left a comment

tsmaeder Dec 18, 2023

xai Dec 18, 2023

tsmaeder Dec 18, 2023

tsmaeder Dec 18, 2023

tsmaeder Dec 18, 2023

tsmaeder Dec 18, 2023

tsmaeder Dec 18, 2023

xai Dec 18, 2023

tsmaeder Dec 18, 2023

tsmaeder Dec 18, 2023

tsmaeder commented Dec 18, 2023

xai commented Dec 18, 2023

tsmaeder left a comment

tsmaeder commented Dec 21, 2023

xai commented Dec 21, 2023

fix: revise vscode extension directories #13178

fix: revise vscode extension directories #13178

Conversation

xai commented Dec 15, 2023 • edited Loading

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

tsmaeder commented Dec 15, 2023

xai commented Dec 15, 2023

xai commented Dec 15, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Dec 18, 2023

xai commented Dec 18, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder commented Dec 21, 2023

xai commented Dec 21, 2023

xai commented Dec 15, 2023 •

edited

Loading