feat: remove sync fs calls from backend+plugin code (#12798)

* feat: remove sync fs calls from plugins * feat: remove sync fs calls from git * feat: remove sync fs calls from siw * feat: remove sync fs calls from workspace Signed-off-by: Akos Kitta <[email protected]>
eclipse-theia · Aug 14, 2023 · 04c8cf0 · 04c8cf0
1 parent 2d91943
commit 04c8cf0
Show file tree

Hide file tree

Showing 29 changed files with 412 additions and 231 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -44,6 +44,7 @@
     - `visual`: Display a visual preview of the tab. (The preview support was added with this PR)
 - [repo] updated GitHub workflow to stop publishing `next` versions [#12699](https://github.com/eclipse-theia/theia/pull/12699)
 - [workspace] split `CommonWorkspaceUtils` into `WorkspaceFileService` and `UntitledWorkspaceService` [#12420](https://github.com/eclipse-theia/theia/pull/12420)
+- [plugin] Removed synchronous `fs` calls from the backend application and plugins. The plugin scanner, directory and file handlers, and the plugin deploy entry has async API now. Internal `protected` APIs have been affected. [#12798](https://github.com/eclipse-theia/theia/pull/12798)
 
 ## v1.39.0 - 06/29/2023
 

diff --git a/packages/core/src/common/promise-util.spec.ts b/packages/core/src/common/promise-util.spec.ts
@@ -13,22 +13,19 @@
 //
 // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
 // *****************************************************************************
-import * as assert from 'assert';
-import { waitForEvent } from './promise-util';
+import * as assert from 'assert/strict';
+import { firstTrue, waitForEvent } from './promise-util';
 import { Emitter } from './event';
+import { CancellationError } from './cancellation';
 
 describe('promise-util', () => {
-    it('should time out', async () => {
-        const emitter = new Emitter<string>();
-        try {
-            await waitForEvent(emitter.event, 1000);
-            assert.fail('did not time out');
-        } catch (e) {
-            // OK
-        }
-    });
 
-    describe('promise-util', () => {
+    describe('waitForEvent', () => {
+        it('should time out', async () => {
+            const emitter = new Emitter<string>();
+            await assert.rejects(waitForEvent(emitter.event, 1000), reason => reason instanceof CancellationError);
+        });
+
         it('should get event', async () => {
             const emitter = new Emitter<string>();
             setTimeout(() => {
@@ -38,4 +35,38 @@ describe('promise-util', () => {
         });
     });
 
+    describe('firstTrue', () => {
+        it('should resolve to false when the promises arg is empty', async () => {
+            const actual = await firstTrue();
+            assert.strictEqual(actual, false);
+        });
+
+        it('should resolve to true when the first promise resolves to true', async () => {
+            const signals: string[] = [];
+            const createPromise = (signal: string, timeout: number, result: boolean) =>
+                new Promise<boolean>(resolve => setTimeout(() => {
+                    signals.push(signal);
+                    resolve(result);
+                }, timeout));
+            const actual = await firstTrue(
+                createPromise('a', 10, false),
+                createPromise('b', 20, false),
+                createPromise('c', 30, true),
+                createPromise('d', 40, false),
+                createPromise('e', 50, true)
+            );
+            assert.strictEqual(actual, true);
+            assert.deepStrictEqual(signals, ['a', 'b', 'c']);
+        });
+
+        it('should reject when one of the promises rejects', async () => {
+            await assert.rejects(firstTrue(
+                new Promise<boolean>(resolve => setTimeout(() => resolve(false), 10)),
+                new Promise<boolean>(resolve => setTimeout(() => resolve(false), 20)),
+                new Promise<boolean>((_, reject) => setTimeout(() => reject(new Error('my test error')), 30)),
+                new Promise<boolean>(resolve => setTimeout(() => resolve(true), 40)),
+            ), /Error: my test error/);
+        });
+    });
+
 });
diff --git a/packages/core/src/common/promise-util.ts b/packages/core/src/common/promise-util.ts
@@ -129,3 +129,15 @@ export function waitForEvent<T>(event: Event<T>, ms: number, thisArg?: any, disp
 export function isThenable<T>(obj: unknown): obj is Promise<T> {
     return isObject<Promise<unknown>>(obj) && isFunction(obj.then);
 }
+
+/**
+ * Returns with a promise that waits until the first promise resolves to `true`.
+ */
+// Based on https://stackoverflow.com/a/51160727/5529090
+export function firstTrue(...promises: readonly Promise<boolean>[]): Promise<boolean> {
+    const newPromises = promises.map(promise => new Promise<boolean>(
+        (resolve, reject) => promise.then(result => result && resolve(true), reject)
+    ));
+    newPromises.push(Promise.all(promises).then(() => false));
+    return Promise.race(newPromises);
+}
diff --git a/packages/git/src/node/dugite-git.ts b/packages/git/src/node/dugite-git.ts
@@ -766,7 +766,8 @@ export class DugiteGit implements Git {
         const out = result.stdout;
         if (out && out.length !== 0) {
             try {
-                return fs.realpathSync(out.trim());
+                const realpath = await fs.realpath(out.trim());
+                return realpath;
             } catch (e) {
                 this.logger.error(e);
                 return undefined;

diff --git a/packages/git/src/node/git-locator/git-locator-impl.ts b/packages/git/src/node/git-locator/git-locator-impl.ts
@@ -59,7 +59,7 @@ export class GitLocatorImpl implements GitLocator {
     }
 
     protected async doLocate(basePath: string, context: GitLocateContext): Promise<string[]> {
-        const realBasePath = fs.realpathSync(basePath);
+        const realBasePath = await fs.realpath(basePath);
         if (context.visited.has(realBasePath)) {
             return [];
         }
@@ -77,9 +77,9 @@ export class GitLocatorImpl implements GitLocator {
                 }
             });
             if (context.maxCount >= 0 && paths.length >= context.maxCount) {
-                return paths.slice(0, context.maxCount).map(GitLocatorImpl.map);
+                return await Promise.all(paths.slice(0, context.maxCount).map(GitLocatorImpl.map));
             }
-            const repositoryPaths = paths.map(GitLocatorImpl.map);
+            const repositoryPaths = await Promise.all(paths.map(GitLocatorImpl.map));
             return this.locateFrom(
                 newContext => this.generateNested(repositoryPaths, newContext),
                 context,
@@ -145,8 +145,8 @@ export class GitLocatorImpl implements GitLocator {
         return result;
     }
 
-    static map(repository: string): string {
-        return fs.realpathSync(path.dirname(repository));
+    static async map(repository: string): Promise<string> {
+        return fs.realpath(path.dirname(repository));
     }
 
 }
diff --git a/packages/plugin-dev/src/node/hosted-instance-manager.ts b/packages/plugin-dev/src/node/hosted-instance-manager.ts
@@ -30,6 +30,7 @@ import { HostedPluginSupport } from '@theia/plugin-ext/lib/hosted/node/hosted-pl
 import { MetadataScanner } from '@theia/plugin-ext/lib/hosted/node/metadata-scanner';
 import { PluginDebugConfiguration } from '../common/plugin-dev-protocol';
 import { HostedPluginProcess } from '@theia/plugin-ext/lib/hosted/node/hosted-plugin-process';
+import { isENOENT } from '@theia/plugin-ext/lib/common/errors';
 
 const DEFAULT_HOSTED_PLUGIN_PORT = 3030;
 
@@ -84,7 +85,7 @@ export interface HostedInstanceManager {
      *
      * @param uri uri to the plugin source location
      */
-    isPluginValid(uri: URI): boolean;
+    isPluginValid(uri: URI): Promise<boolean>;
 }
 
 const HOSTED_INSTANCE_START_TIMEOUT_MS = 30000;
@@ -224,19 +225,18 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
         }
     }
 
-    isPluginValid(uri: URI): boolean {
+    async isPluginValid(uri: URI): Promise<boolean> {
         const pckPath = path.join(FileUri.fsPath(uri), 'package.json');
-        if (fs.existsSync(pckPath)) {
-            const pck = fs.readJSONSync(pckPath);
-            try {
-                this.metadata.getScanner(pck);
-                return true;
-            } catch (e) {
-                console.error(e);
-                return false;
+        try {
+            const pck = await fs.readJSON(pckPath);
+            this.metadata.getScanner(pck);
+            return true;
+        } catch (err) {
+            if (!isENOENT(err)) {
+                console.error(err);
             }
+            return false;
         }
-        return false;
     }
 
     protected async getStartCommand(port?: number, debugConfig?: PluginDebugConfiguration): Promise<string[]> {

diff --git a/packages/plugin-dev/src/node/hosted-plugins-manager.ts b/packages/plugin-dev/src/node/hosted-plugins-manager.ts
@@ -131,15 +131,15 @@ export class HostedPluginsManagerImpl implements HostedPluginsManager {
      *
      * @param pluginPath path to plugin's root directory
      */
-    protected checkWatchScript(pluginPath: string): boolean {
+    protected async checkWatchScript(pluginPath: string): Promise<boolean> {
         const pluginPackageJsonPath = path.join(pluginPath, 'package.json');
-        if (fs.existsSync(pluginPackageJsonPath)) {
-            const packageJson = fs.readJSONSync(pluginPackageJsonPath);
+        try {
+            const packageJson = await fs.readJSON(pluginPackageJsonPath);
             const scripts = packageJson['scripts'];
             if (scripts && scripts['watch']) {
                 return true;
             }
-        }
+        } catch { }
         return false;
     }
 

diff --git a/packages/plugin-ext-vscode/src/node/plugin-vscode-directory-handler.ts b/packages/plugin-ext-vscode/src/node/plugin-vscode-directory-handler.ts
@@ -18,33 +18,47 @@ import * as path from 'path';
 import * as filenamify from 'filenamify';
 import * as fs from '@theia/core/shared/fs-extra';
 import { inject, injectable } from '@theia/core/shared/inversify';
-import { RecursivePartial } from '@theia/core';
+import type { RecursivePartial, URI } from '@theia/core';
+import { Deferred, firstTrue } from '@theia/core/lib/common/promise-util';
+import { getTempDirPathAsync } from '@theia/plugin-ext/lib/main/node/temp-dir-util';
 import {
     PluginDeployerDirectoryHandler, PluginDeployerEntry, PluginDeployerDirectoryHandlerContext,
     PluginDeployerEntryType, PluginPackage, PluginType, PluginIdentifiers
 } from '@theia/plugin-ext';
 import { FileUri } from '@theia/core/lib/node';
-import { getTempDir } from '@theia/plugin-ext/lib/main/node/temp-dir-util';
 import { PluginCliContribution } from '@theia/plugin-ext/lib/main/node/plugin-cli-contribution';
 
 @injectable()
 export class PluginVsCodeDirectoryHandler implements PluginDeployerDirectoryHandler {
 
-    protected readonly deploymentDirectory = FileUri.create(getTempDir('vscode-copied'));
+    protected readonly deploymentDirectory: Deferred<URI>;
 
     @inject(PluginCliContribution) protected readonly pluginCli: PluginCliContribution;
 
-    accept(plugin: PluginDeployerEntry): boolean {
+    constructor() {
+        this.deploymentDirectory = new Deferred();
+        getTempDirPathAsync('vscode-copied')
+            .then(deploymentDirectoryPath => this.deploymentDirectory.resolve(FileUri.create(deploymentDirectoryPath)));
+    }
+
+    async accept(plugin: PluginDeployerEntry): Promise<boolean> {
         console.debug(`Resolving "${plugin.id()}" as a VS Code extension...`);
         return this.attemptResolution(plugin);
     }
 
-    protected attemptResolution(plugin: PluginDeployerEntry): boolean {
-        return this.resolvePackage(plugin) || this.deriveMetadata(plugin);
+    protected async attemptResolution(plugin: PluginDeployerEntry): Promise<boolean> {
+        if (this.resolvePackage(plugin)) {
+            return true;
+        }
+        return this.deriveMetadata(plugin);
     }
 
-    protected deriveMetadata(plugin: PluginDeployerEntry): boolean {
-        return this.resolveFromSources(plugin) || this.resolveFromVSIX(plugin) || this.resolveFromNpmTarball(plugin);
+    protected async deriveMetadata(plugin: PluginDeployerEntry): Promise<boolean> {
+        return firstTrue(
+            this.resolveFromSources(plugin),
+            this.resolveFromVSIX(plugin),
+            this.resolveFromNpmTarball(plugin)
+        );
     }
 
     async handle(context: PluginDeployerDirectoryHandlerContext): Promise<void> {
@@ -68,11 +82,12 @@ export class PluginVsCodeDirectoryHandler implements PluginDeployerDirectoryHand
             const origin = entry.originalPath();
             const targetDir = await this.getExtensionDir(context);
             try {
-                if (fs.existsSync(targetDir) || !entry.path().startsWith(origin)) {
+                if (await fs.pathExists(targetDir) || !entry.path().startsWith(origin)) {
                     console.log(`[${id}]: already copied.`);
                 } else {
                     console.log(`[${id}]: copying to "${targetDir}"`);
-                    await fs.mkdirp(FileUri.fsPath(this.deploymentDirectory));
+                    const deploymentDirectory = await this.deploymentDirectory.promise;
+                    await fs.mkdirp(FileUri.fsPath(deploymentDirectory));
                     await context.copy(origin, targetDir);
                     entry.updatePath(targetDir);
                     if (!this.deriveMetadata(entry)) {
@@ -86,22 +101,25 @@ export class PluginVsCodeDirectoryHandler implements PluginDeployerDirectoryHand
         }
     }
 
-    protected resolveFromSources(plugin: PluginDeployerEntry): boolean {
+    protected async resolveFromSources(plugin: PluginDeployerEntry): Promise<boolean> {
         const pluginPath = plugin.path();
-        return this.resolvePackage(plugin, { pluginPath, pck: this.requirePackage(pluginPath) });
+        const pck = await this.requirePackage(pluginPath);
+        return this.resolvePackage(plugin, { pluginPath, pck });
     }
 
-    protected resolveFromVSIX(plugin: PluginDeployerEntry): boolean {
-        if (!fs.existsSync(path.join(plugin.path(), 'extension.vsixmanifest'))) {
+    protected async resolveFromVSIX(plugin: PluginDeployerEntry): Promise<boolean> {
+        if (!(await fs.pathExists(path.join(plugin.path(), 'extension.vsixmanifest')))) {
             return false;
         }
         const pluginPath = path.join(plugin.path(), 'extension');
-        return this.resolvePackage(plugin, { pluginPath, pck: this.requirePackage(pluginPath) });
+        const pck = await this.requirePackage(pluginPath);
+        return this.resolvePackage(plugin, { pluginPath, pck });
     }
 
-    protected resolveFromNpmTarball(plugin: PluginDeployerEntry): boolean {
+    protected async resolveFromNpmTarball(plugin: PluginDeployerEntry): Promise<boolean> {
         const pluginPath = path.join(plugin.path(), 'package');
-        return this.resolvePackage(plugin, { pluginPath, pck: this.requirePackage(pluginPath) });
+        const pck = await this.requirePackage(pluginPath);
+        return this.resolvePackage(plugin, { pluginPath, pck });
     }
 
     protected resolvePackage(plugin: PluginDeployerEntry, options?: {
@@ -125,9 +143,9 @@ export class PluginVsCodeDirectoryHandler implements PluginDeployerDirectoryHand
         return true;
     }
 
-    protected requirePackage(pluginPath: string): PluginPackage | undefined {
+    protected async requirePackage(pluginPath: string): Promise<PluginPackage | undefined> {
         try {
-            const plugin = fs.readJSONSync(path.join(pluginPath, 'package.json')) as PluginPackage;
+            const plugin: PluginPackage = await fs.readJSON(path.join(pluginPath, 'package.json'));
             plugin.publisher ??= PluginIdentifiers.UNPUBLISHED;
             return plugin;
         } catch {
@@ -136,6 +154,7 @@ export class PluginVsCodeDirectoryHandler implements PluginDeployerDirectoryHand
     }
 
     protected async getExtensionDir(context: PluginDeployerDirectoryHandlerContext): Promise<string> {
-        return FileUri.fsPath(this.deploymentDirectory.resolve(filenamify(context.pluginEntry().id(), { replacement: '_' })));
+        const deploymentDirectory = await this.deploymentDirectory.promise;
+        return FileUri.fsPath(deploymentDirectory.resolve(filenamify(context.pluginEntry().id(), { replacement: '_' })));
     }
 }
diff --git a/packages/plugin-ext-vscode/src/node/plugin-vscode-file-handler.ts b/packages/plugin-ext-vscode/src/node/plugin-vscode-file-handler.ts
@@ -18,8 +18,10 @@ import { PluginDeployerFileHandler, PluginDeployerEntry, PluginDeployerFileHandl
 import * as fs from '@theia/core/shared/fs-extra';
 import * as path from 'path';
 import * as filenamify from 'filenamify';
-import { injectable, inject } from '@theia/core/shared/inversify';
-import { getTempDir } from '@theia/plugin-ext/lib/main/node/temp-dir-util';
+import type { URI } from '@theia/core';
+import { inject, injectable } from '@theia/core/shared/inversify';
+import { Deferred } from '@theia/core/lib/common/promise-util';
+import { getTempDirPathAsync } from '@theia/plugin-ext/lib/main/node/temp-dir-util';
 import { PluginVSCodeEnvironment } from '../common/plugin-vscode-environment';
 import { FileUri } from '@theia/core/lib/node/file-uri';
 
@@ -31,13 +33,21 @@ export class PluginVsCodeFileHandler implements PluginDeployerFileHandler {
     @inject(PluginVSCodeEnvironment)
     protected readonly environment: PluginVSCodeEnvironment;
 
-    private readonly systemExtensionsDirUri = FileUri.create(getTempDir('vscode-unpacked'));
+    private readonly systemExtensionsDirUri: Deferred<URI>;
 
-    accept(resolvedPlugin: PluginDeployerEntry): boolean {
-        if (!resolvedPlugin.isFile()) {
-            return false;
-        }
-        return isVSCodePluginFile(resolvedPlugin.path());
+    constructor() {
+        this.systemExtensionsDirUri = new Deferred();
+        getTempDirPathAsync('vscode-unpacked')
+            .then(systemExtensionsDirPath => this.systemExtensionsDirUri.resolve(FileUri.create(systemExtensionsDirPath)));
+    }
+
+    async accept(resolvedPlugin: PluginDeployerEntry): Promise<boolean> {
+        return resolvedPlugin.isFile().then(file => {
+            if (!file) {
+                return false;
+            }
+            return isVSCodePluginFile(resolvedPlugin.path());
+        });
     }
 
     async handle(context: PluginDeployerFileHandlerContext): Promise<void> {
@@ -55,7 +65,8 @@ export class PluginVsCodeFileHandler implements PluginDeployerFileHandler {
     }
 
     protected async getExtensionDir(context: PluginDeployerFileHandlerContext): Promise<string> {
-        return FileUri.fsPath(this.systemExtensionsDirUri.resolve(filenamify(context.pluginEntry().id(), { replacement: '_' })));
+        const systemExtensionsDirUri = await this.systemExtensionsDirUri.promise;
+        return FileUri.fsPath(systemExtensionsDirUri.resolve(filenamify(context.pluginEntry().id(), { replacement: '_' })));
     }
 
     protected async decompress(extensionDir: string, context: PluginDeployerFileHandlerContext): Promise<void> {