plugin-ext: Change PluginsKeyValueStorage to not continuously access the disk

packages/core/package.json

@@ -34,6 +34,7 @@
     "@types/yargs": "^15",
     "@vscode/codicons": "*",
     "ajv": "^6.5.3",
+    "async-mutex": "^0.4.0",
     "body-parser": "^1.17.2",
     "cookie": "^0.4.0",
     "dompurify": "^2.2.9",

packages/core/src/node/backend-application-module.ts

@@ -40,6 +40,7 @@ import { ProxyCliContribution } from './request/proxy-cli-contribution';
 import { bindNodeStopwatch, bindBackendStopwatchServer } from './performance';
 import { OSBackendApplicationContribution } from './os-backend-application-contribution';
 import { BackendRequestFacade } from './request/backend-request-facade';
+import { FileSystemLocking, FileSystemLockingImpl } from './filesystem-locking';
 
 decorate(injectable(), ApplicationPackage);
 
@@ -128,4 +129,6 @@ export const backendApplicationModule = new ContainerModule(bind => {
 
     bindNodeStopwatch(bind);
     bindBackendStopwatchServer(bind);
+
+    bind(FileSystemLocking).to(FileSystemLockingImpl).inSingletonScope();
 });

packages/core/src/node/filesystem-locking.ts

@@ -0,0 +1,77 @@
+// *****************************************************************************
+// Copyright (C) 2023 Ericsson and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// http://www.eclipse.org/legal/epl-2.0.
+//
+// This Source Code may also be made available under the following Secondary
+// Licenses when the conditions for such availability set forth in the Eclipse
+// Public License v. 2.0 are satisfied: GNU General Public License, version 2
+// with the GNU Classpath Exception which is available at
+// https://www.gnu.org/software/classpath/license.html.
+//
+// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+// *****************************************************************************
+
+import { Mutex } from 'async-mutex';
+import { injectable, interfaces } from 'inversify';
+import * as path from 'path';
+
+export const FileSystemLocking = Symbol('FileSystemLocking') as symbol & interfaces.Abstract<FileSystemLocking>;
+/**
+ * Use this backend service to help prevent race access to files on disk.
+ */
+export interface FileSystemLocking {
+    /**
+     * Get exclusive access to a file for reading and/or writing.
+     * @param lockPath The path to request exclusive access to.
+     * @param transaction The job to do while having exclusive access.
+     * @param thisArg `this` argument used when calling `transaction`.
+     */
+    lockPath<T>(lockPath: string, transaction: (lockPath: string) => T | Promise<T>, thisArg?: unknown): Promise<T>;
+}
+
+@injectable()
+export class FileSystemLockingImpl implements FileSystemLocking {
+
+    lockPath<T>(lockPath: string, transaction: (lockPath: string) => T | Promise<T>, thisArg?: unknown): Promise<T> {
+        const resolvedLockPath = this.resolveLockPath(lockPath);
+        return this.getLock(resolvedLockPath).runExclusive(async () => transaction.call(thisArg, resolvedLockPath));
+    }
+
+    protected resolveLockPath(lockPath: string): string {
+        // try to normalize the path to avoid two paths pointing to the same file
+        return path.resolve(lockPath);
+    }
+
+    protected getLocks(): Map<string, Mutex> {
+        const kLocks = Symbol.for('FileSystemLockingImpl.Locks');
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        return (globalThis as any)[kLocks] ??= this.initializeLocks();
+    }
+
+    protected initializeLocks(): Map<string, Mutex> {
+        const locks = new Map();
+        const cleanup = setInterval(() => this.cleanupLocks(locks), 60_000);
+        process.once('beforeExit', () => clearInterval(cleanup));
+        return locks;
+    }
+
+    protected cleanupLocks(locks: Map<string, Mutex>): void {
+        locks.forEach((lock, lockPath) => {
+            if (!lock.isLocked()) {
+                locks.delete(lockPath);
+            }
+        });
+    }
+
+    protected getLock(lockPath: string): Mutex {
+        const locks = this.getLocks();
+        let lock = locks.get(lockPath);
+        if (!lock) {
+            locks.set(lockPath, lock = new Mutex());
+        }
+        return lock;
+    }
+}

packages/core/src/node/index.ts

@@ -19,3 +19,4 @@ export * from './debug';
 export * from './file-uri';
 export * from './messaging';
 export * from './cli';
+export { FileSystemLocking } from './filesystem-locking';

packages/plugin-ext/src/main/node/plugins-key-value-storage.spec.ts

@@ -0,0 +1,106 @@
+/********************************************************************************
+ * Copyright (C) 2023 Ericsson and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+import 'reflect-metadata';
+import { expect } from 'chai';
+import { Container } from '@theia/core/shared/inversify';
+import { EnvVariablesServer } from '@theia/core/lib/common/env-variables';
+import { PluginsKeyValueStorage } from './plugins-key-value-storage';
+import { PluginPathsService } from '../common/plugin-paths-protocol';
+import { PluginPathsServiceImpl } from './paths/plugin-paths-service';
+import { PluginCliContribution } from './plugin-cli-contribution';
+import { ILogger } from '@theia/core/lib/common/logger';
+import { MockLogger } from '@theia/core/lib/common/test/mock-logger';
+import { MockEnvVariablesServerImpl } from '@theia/core/lib/browser/test/mock-env-variables-server';
+import { FileSystemLocking, FileUri } from '@theia/core/lib/node';
+import { FileSystemLockingImpl } from '@theia/core/lib/node/filesystem-locking';
+import * as temp from 'temp';
+
+const GlobalStorageKind = undefined;
+
+describe('Plugins Key Value Storage', () => {
+
+    let container: Container;
+
+    beforeEach(async () => {
+        container = new Container();
+        container.bind(PluginsKeyValueStorage).toSelf().inSingletonScope();
+        container.bind(PluginCliContribution).toSelf().inSingletonScope();
+        container.bind(FileSystemLocking).to(FileSystemLockingImpl).inSingletonScope();
+        container.bind(EnvVariablesServer).toConstantValue(new MockEnvVariablesServerImpl(FileUri.create(temp.track().mkdirSync())));
+        container.bind(PluginPathsService).to(PluginPathsServiceImpl).inSingletonScope();
+        container.bind(ILogger).toConstantValue(MockLogger);
+        const storage = container.get(PluginsKeyValueStorage);
+        expect(await getNumEntries(storage), 'Expected that storage should initially be empty').to.equal(0);
+    });
+
+    afterEach(() => {
+        container.get(PluginsKeyValueStorage)['dispose']();
+    });
+
+    it('Should be able to set and overwrite a storage entry', async () => {
+        const aKey = 'akey';
+        const aValue = { 'this is a test': 'abc' };
+        const anotherValue = { 'this is an updated value': 'def' };
+        const storage = container.get(PluginsKeyValueStorage);
+        await storage.set(aKey, aValue, GlobalStorageKind);
+        expect(await getNumEntries(storage), 'Expected 1 storage entry').to.be.equal(1);
+        expect(await storage.get(aKey, GlobalStorageKind), 'Expected storage entry to have initially set value')
+            .to.be.deep.equal(aValue);
+        await storage.set(aKey, anotherValue, GlobalStorageKind);
+        expect(await getNumEntries(storage), 'Expected 1 storage entry').to.be.equal(1);
+        expect(await storage.get(aKey, GlobalStorageKind), 'Expected storage entry to have updated value')
+            .to.be.deep.equal(anotherValue);
+    });
+
+    // This test should fail if the storage does not protect itself against concurrent accesses
+    it('Should be able to save several entries to storage and retrieve them as set', async () => {
+        const n = 100;
+        const key = 'test';
+        const valuePropName = 'test-value';
+        const storage = container.get(PluginsKeyValueStorage);
+        await populateStorage(storage, key, valuePropName, n);
+        await checkStorageContent(storage, key, valuePropName, n);
+    });
+
+});
+
+const populateStorage = async (storage: PluginsKeyValueStorage, keyPrefix: string, valuePropName: string, num: number) => {
+    const tasks: Promise<boolean>[] = [];
+    for (let i = 0; i < num; i++) {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const value: { [key: string]: any } = {};
+        value[valuePropName] = i;
+        tasks.push(storage.set(keyPrefix + i, value, GlobalStorageKind));
+    }
+    await Promise.allSettled(tasks);
+};
+
+const getNumEntries = async (storage: PluginsKeyValueStorage) => {
+    const all = await storage.getAll(GlobalStorageKind);
+    return Object.keys(all).length;
+};
+
+const checkStorageContent = async (storage: PluginsKeyValueStorage, keyPrefix: string, valuePropName: string, num: number) => {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const expectedValue: { [key: string]: any } = {};
+    const all = await storage.getAll(GlobalStorageKind);
+    for (let i = 0; i < num; i++) {
+        expectedValue[valuePropName] = i;
+        expect(all[keyPrefix + i], 'Expected storage entry ' + i + ' to have kept previously set value')
+            .to.be.deep.equal(expectedValue);
+    }
+};

packages/plugin-ext/src/main/node/plugins-key-value-storage.ts

@@ -14,7 +14,9 @@
 // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 // *****************************************************************************
 
+import { deepmerge } from '@theia/core/shared/@theia/application-package';
 import { injectable, inject, postConstruct } from '@theia/core/shared/inversify';
+import { FileSystemLocking } from '@theia/core/lib/node';
 import * as fs from '@theia/core/shared/fs-extra';
 import * as path from 'path';
 import { FileUri } from '@theia/core/lib/node/file-uri';
@@ -25,76 +27,117 @@ import { PluginPathsService } from '../common/plugin-paths-protocol';
 import { KeysToAnyValues, KeysToKeysToAnyValue } from '../../common/types';
 import { PluginStorageKind } from '../../common';
 
+export interface Store {
+    fsPath: string
+    values: KeysToKeysToAnyValue
+}
+
 @injectable()
 export class PluginsKeyValueStorage {
 
-    private readonly deferredGlobalDataPath = new Deferred<string | undefined>();
+    private stores: Record<string, Store> = Object.create(null);
+    private storesToSync = new Set<Store>();
+    private syncStoresTimeout?: NodeJS.Timeout;
+
+    private deferredGlobalDataPath = new Deferred<string | undefined>();
 
     @inject(PluginPathsService)
-    private readonly pluginPathsService: PluginPathsService;
+    private pluginPathsService: PluginPathsService;
 
     @inject(EnvVariablesServer)
-    protected readonly envServer: EnvVariablesServer;
+    private envServer: EnvVariablesServer;
+
+    @inject(FileSystemLocking)
+    private fsLocking: FileSystemLocking;
 
     @postConstruct()
-    protected async init(): Promise<void> {
-        try {
-            const configDirUri = await this.envServer.getConfigDirUri();
-            const globalStorageFsPath = path.join(FileUri.fsPath(configDirUri), PluginPaths.PLUGINS_GLOBAL_STORAGE_DIR);
-            const exists = await fs.pathExists(globalStorageFsPath);
-            if (!exists) {
-                await fs.mkdirs(globalStorageFsPath);
-            }
-            const globalDataFsPath = path.join(globalStorageFsPath, 'global-state.json');
-            this.deferredGlobalDataPath.resolve(globalDataFsPath);
-        } catch (e) {
-            console.error('Failed to initialize global state path: ', e);
-            this.deferredGlobalDataPath.resolve(undefined);
-        }
+    protected init(): void {
+        this.deferredGlobalDataPath.resolve(this.getGlobalDataPath().catch(error => {
+            console.error('Failed to initialize global state path:', error);
+            return undefined;
+        }));
+        process.once('beforeExit', () => this.dispose());
+        this.syncStores();
     }
 
     async set(key: string, value: KeysToAnyValues, kind: PluginStorageKind): Promise<boolean> {
-        const dataPath = await this.getDataPath(kind);
-        if (!dataPath) {
+        const store = await this.getStore(kind);
+        if (!store) {
             console.warn('Cannot save data: no opened workspace');
             return false;
         }
-
-        const data = await this.readFromFile(dataPath);
-
-        if (value === undefined) {
-            delete data[key];
+        if (value === undefined || Object.keys(value).length === 0) {
+            delete store.values[key];
         } else {
-            data[key] = value;
+            store.values[key] = value;
         }
-
-        await this.writeToFile(dataPath, data);
+        this.storesToSync.add(store);
         return true;
     }
 
     async get(key: string, kind: PluginStorageKind): Promise<KeysToAnyValues> {
-        const dataPath = await this.getDataPath(kind);
-        if (!dataPath) {
-            return {};
-        }
-        const data = await this.readFromFile(dataPath);
-        return data[key];
+        const store = await this.getStore(kind);
+        return store?.values[key] ?? {};
     }
 
     async getAll(kind: PluginStorageKind): Promise<KeysToKeysToAnyValue> {
+        const store = await this.getStore(kind);
+        return store?.values ?? {};
+    }
+
+    private async getGlobalDataPath(): Promise<string> {
+        const configDirUri = await this.envServer.getConfigDirUri();
+        const globalStorageFsPath = path.join(FileUri.fsPath(configDirUri), PluginPaths.PLUGINS_GLOBAL_STORAGE_DIR);
+        await fs.ensureDir(globalStorageFsPath);
+        return path.join(globalStorageFsPath, 'global-state.json');
+    }
+
+    private async initializeStore(storePath: string): Promise<Store> {
+        return this.fsLocking.lockPath(storePath, async resolved => {
+            const values = await this.readFromFile(resolved);
+            return {
+                values,
+                fsPath: storePath
+            };
+        });
+    }
+
+    private async getStore(kind: PluginStorageKind): Promise<Store | undefined> {
         const dataPath = await this.getDataPath(kind);
-        if (!dataPath) {
-            return {};
+        if (dataPath) {
+            return this.stores[dataPath] ??= await this.initializeStore(dataPath);
         }
-        return this.readFromFile(dataPath);
+    }
+
+    private syncStores(): void {
+        this.syncStoresTimeout = setTimeout(async () => {
+            await Promise.all(Array.from(this.storesToSync, async store => {
+                await this.fsLocking.lockPath(store.fsPath, async storePath => {
+                    const valuesOnDisk = await this.readFromFile(storePath);
+                    store.values = deepmerge(valuesOnDisk, store.values);
+                    await this.writeToFile(storePath, store.values);
+                });
+            }));
+            this.storesToSync.clear();
+            if (this.syncStoresTimeout) {
+                this.syncStores();
+            }
+        }, this.getSyncStoreTimeout());
+    }
+
+    private getSyncStoreTimeout(): number {
+        // 0-10s + 1min
+        return 10_000 * Math.random() + 60_000;
     }
 
     private async getDataPath(kind: PluginStorageKind): Promise<string | undefined> {
         if (!kind) {
             return this.deferredGlobalDataPath.promise;
         }
         const storagePath = await this.pluginPathsService.getHostStoragePath(kind.workspace, kind.roots);
-        return storagePath ? path.join(storagePath, 'workspace-state.json') : undefined;
+        if (storagePath) {
+            return path.join(storagePath, 'workspace-state.json');
+        }
     }
 
     private async readFromFile(pathToFile: string): Promise<KeysToKeysToAnyValue> {
@@ -114,4 +157,8 @@ export class PluginsKeyValueStorage {
         await fs.writeJSON(pathToFile, data);
     }
 
+    private dispose(): void {
+        clearTimeout(this.syncStoresTimeout);
+        this.syncStoresTimeout = undefined;
+    }
 }