From ecf930f5f56a7f8e98c60c0d99b28d843ad7dba1 Mon Sep 17 00:00:00 2001 From: Jari Kolehmainen Date: Wed, 23 Dec 2020 11:56:11 +0200 Subject: [PATCH] Fix extension loader race conditions (#1815) * fix extension loader race conditions Signed-off-by: Jari Kolehmainen * cleanup Signed-off-by: Jari Kolehmainen * fix tests Signed-off-by: Jari Kolehmainen * fix remove Signed-off-by: Jari Kolehmainen * ensure symlinked (dev) extensions are installed on boot Signed-off-by: Jari Kolehmainen --- .../__tests__/extension-discovery.test.ts | 6 +- src/extensions/extension-discovery.ts | 67 ++++++++++--------- src/extensions/extension-installer.ts | 64 +++++++++++------- src/extensions/extension-loader.ts | 12 +++- src/main/index.ts | 6 +- 5 files changed, 95 insertions(+), 60 deletions(-) diff --git a/src/extensions/__tests__/extension-discovery.test.ts b/src/extensions/__tests__/extension-discovery.test.ts index 0317319329..d0066c3a7e 100644 --- a/src/extensions/__tests__/extension-discovery.test.ts +++ b/src/extensions/__tests__/extension-discovery.test.ts @@ -10,7 +10,7 @@ jest.mock("chokidar", () => ({ jest.mock("../extension-installer", () => ({ extensionInstaller: { extensionPackagesRoot: "", - installPackages: jest.fn() + installPackage: jest.fn() } })); @@ -41,7 +41,7 @@ describe("ExtensionDiscovery", () => { // Need to force isLoaded to be true so that the file watching is started extensionDiscovery.isLoaded = true; - await extensionDiscovery.initMain(); + await extensionDiscovery.watchExtensions(); extensionDiscovery.events.on("add", (extension: InstalledExtension) => { expect(extension).toEqual({ @@ -81,7 +81,7 @@ describe("ExtensionDiscovery", () => { // Need to force isLoaded to be true so that the file watching is started extensionDiscovery.isLoaded = true; - await extensionDiscovery.initMain(); + await extensionDiscovery.watchExtensions(); const onAdd = jest.fn(); diff --git a/src/extensions/extension-discovery.ts b/src/extensions/extension-discovery.ts index 7d0da112bb..0994f89995 100644 --- a/src/extensions/extension-discovery.ts +++ b/src/extensions/extension-discovery.ts @@ -55,6 +55,7 @@ export class ExtensionDiscovery { protected bundledFolderPath: string; private loadStarted = false; + private extensions: Map = new Map(); // True if extensions have been loaded from the disk after app startup @observable isLoaded = false; @@ -69,13 +70,6 @@ export class ExtensionDiscovery { this.events = new EventEmitter(); } - // Each extension is added as a single dependency to this object, which is written as package.json. - // Each dependency key is the name of the dependency, and - // each dependency value is the non-symlinked path to the dependency (folder). - protected packagesJson: PackageJson = { - dependencies: {} - }; - get localFolderPath(): string { return path.join(os.homedir(), ".k8slens", "extensions"); } @@ -119,7 +113,6 @@ export class ExtensionDiscovery { } async initMain() { - this.watchExtensions(); handleRequest(ExtensionDiscovery.extensionDiscoveryChannel, () => this.toJSON()); reaction(() => this.toJSON(), () => { @@ -141,6 +134,7 @@ export class ExtensionDiscovery { watch(this.localFolderPath, { // For adding and removing symlinks to work, the depth has to be 1. depth: 1, + ignoreInitial: true, // Try to wait until the file has been completely copied. // The OS might emit an event for added file even it's not completely written to the filesysten. awaitWriteFinish: { @@ -176,8 +170,9 @@ export class ExtensionDiscovery { await this.removeSymlinkByManifestPath(manifestPath); // Install dependencies for the new extension - await this.installPackages(); + await this.installPackage(extension.absolutePath); + this.extensions.set(extension.id, extension); logger.info(`${logModule} Added extension ${extension.manifest.name}`); this.events.emit("add", extension); } @@ -197,23 +192,19 @@ export class ExtensionDiscovery { const extensionFolderName = path.basename(filePath); if (path.relative(this.localFolderPath, filePath) === extensionFolderName) { - const extensionName: string | undefined = Object - .entries(this.packagesJson.dependencies) - .find(([, extensionFolder]) => filePath === extensionFolder)?.[0]; + const extension = Array.from(this.extensions.values()).find((extension) => extension.absolutePath === filePath); + + if (extension) { + const extensionName = extension.manifest.name; - if (extensionName !== undefined) { // If the extension is deleted manually while the application is running, also remove the symlink await this.removeSymlinkByPackageName(extensionName); - delete this.packagesJson.dependencies[extensionName]; - - // Reinstall dependencies to remove the extension from package.json - await this.installPackages(); - // The path to the manifest file is the lens extension id // Note that we need to use the symlinked path - const lensExtensionId = path.join(this.nodeModulesPath, extensionName, manifestFilename); + const lensExtensionId = extension.manifestPath; + this.extensions.delete(extension.id); logger.info(`${logModule} removed extension ${extensionName}`); this.events.emit("remove", lensExtensionId as LensExtensionId); } else { @@ -296,7 +287,7 @@ export class ExtensionDiscovery { await fs.ensureDir(this.nodeModulesPath); await fs.ensureDir(this.localFolderPath); - const extensions = await this.loadExtensions(); + const extensions = await this.ensureExtensions(); this.isLoaded = true; @@ -335,7 +326,6 @@ export class ExtensionDiscovery { manifestJson = __non_webpack_require__(manifestPath); const installedManifestPath = this.getInstalledManifestPath(manifestJson.name); - this.packagesJson.dependencies[manifestJson.name] = path.dirname(manifestPath); const isEnabled = isBundled || extensionsStore.isEnabled(installedManifestPath); return { @@ -347,29 +337,46 @@ export class ExtensionDiscovery { isEnabled }; } catch (error) { - logger.error(`${logModule}: can't install extension at ${manifestPath}: ${error}`, { manifestJson }); + logger.error(`${logModule}: can't load extension manifest at ${manifestPath}: ${error}`, { manifestJson }); return null; } } - async loadExtensions(): Promise> { + async ensureExtensions(): Promise> { const bundledExtensions = await this.loadBundledExtensions(); - await this.installPackages(); // install in-tree as a separate step - const localExtensions = await this.loadFromFolder(this.localFolderPath); + await this.installBundledPackages(this.packageJsonPath, bundledExtensions); - await this.installPackages(); - const extensions = bundledExtensions.concat(localExtensions); + const userExtensions = await this.loadFromFolder(this.localFolderPath); - return new Map(extensions.map(extension => [extension.id, extension])); + for (const extension of userExtensions) { + if (await fs.pathExists(extension.manifestPath) === false) { + await this.installPackage(extension.absolutePath); + } + } + const extensions = bundledExtensions.concat(userExtensions); + + return this.extensions = new Map(extensions.map(extension => [extension.id, extension])); } /** * Write package.json to file system and install dependencies. */ - installPackages() { - return extensionInstaller.installPackages(this.packageJsonPath, this.packagesJson); + async installBundledPackages(packageJsonPath: string, extensions: InstalledExtension[]) { + const packagesJson: PackageJson = { + dependencies: {} + }; + + extensions.forEach((extension) => { + packagesJson.dependencies[extension.manifest.name] = extension.absolutePath; + }); + + return await extensionInstaller.installPackages(packageJsonPath, packagesJson); + } + + async installPackage(name: string) { + return extensionInstaller.installPackage(name); } async loadBundledExtensions() { diff --git a/src/extensions/extension-installer.ts b/src/extensions/extension-installer.ts index 75b30d0b9a..04b78bbe1a 100644 --- a/src/extensions/extension-installer.ts +++ b/src/extensions/extension-installer.ts @@ -30,12 +30,49 @@ export class ExtensionInstaller { return __non_webpack_require__.resolve("npm/bin/npm-cli"); } - installDependencies(): Promise { - return new Promise((resolve, reject) => { + /** + * Write package.json to the file system and execute npm install for it. + */ + async installPackages(packageJsonPath: string, packagesJson: PackageJson): Promise { + // Mutual exclusion to install packages in sequence + await this.installLock.acquireAsync(); + + try { + // Write the package.json which will be installed in .installDependencies() + await fs.writeFile(path.join(packageJsonPath), JSON.stringify(packagesJson, null, 2), { + mode: 0o600 + }); + logger.info(`${logModule} installing dependencies at ${extensionPackagesRoot()}`); - const child = child_process.fork(this.npmPath, ["install", "--no-audit", "--only=prod", "--prefer-offline", "--no-package-lock"], { + await this.npm(["install", "--no-audit", "--only=prod", "--prefer-offline", "--no-package-lock"]); + logger.info(`${logModule} dependencies installed at ${extensionPackagesRoot()}`); + } finally { + this.installLock.release(); + } + } + + /** + * Install single package using npm + */ + async installPackage(name: string): Promise { + // Mutual exclusion to install packages in sequence + await this.installLock.acquireAsync(); + + try { + logger.info(`${logModule} installing package from ${name} to ${extensionPackagesRoot()}`); + await this.npm(["install", "--no-audit", "--only=prod", "--prefer-offline", "--no-package-lock", "--no-save", name]); + logger.info(`${logModule} package ${name} installed to ${extensionPackagesRoot()}`); + } finally { + this.installLock.release(); + } + } + + private npm(args: string[]): Promise { + return new Promise((resolve, reject) => { + const child = child_process.fork(this.npmPath, args, { cwd: extensionPackagesRoot(), - silent: true + silent: true, + env: {} }); let stderr = ""; @@ -56,25 +93,6 @@ export class ExtensionInstaller { }); }); } - - /** - * Write package.json to the file system and execute npm install for it. - */ - async installPackages(packageJsonPath: string, packagesJson: PackageJson): Promise { - // Mutual exclusion to install packages in sequence - await this.installLock.acquireAsync(); - - try { - // Write the package.json which will be installed in .installDependencies() - await fs.writeFile(path.join(packageJsonPath), JSON.stringify(packagesJson, null, 2), { - mode: 0o600 - }); - - await this.installDependencies(); - } finally { - this.installLock.release(); - } - } } export const extensionInstaller = new ExtensionInstaller(); diff --git a/src/extensions/extension-loader.ts b/src/extensions/extension-loader.ts index 966289f157..98697d252c 100644 --- a/src/extensions/extension-loader.ts +++ b/src/extensions/extension-loader.ts @@ -12,6 +12,7 @@ import type { LensExtension, LensExtensionConstructor, LensExtensionId } from ". import type { LensMainExtension } from "./lens-main-extension"; import type { LensRendererExtension } from "./lens-renderer-extension"; import * as registries from "./registries"; +import fs from "fs"; // lazy load so that we get correct userData export function extensionPackagesRoot() { @@ -71,7 +72,7 @@ export class ExtensionLoader { } await Promise.all([this.whenLoaded, extensionsStore.whenLoaded]); - + // save state on change `extension.isEnabled` reaction(() => this.storeState, extensionsState => { extensionsStore.mergeState(extensionsState); @@ -115,7 +116,6 @@ export class ExtensionLoader { protected async initMain() { this.isLoaded = true; this.loadOnMain(); - this.broadcastExtensions(); reaction(() => this.toJSON(), () => { this.broadcastExtensions(); @@ -136,7 +136,7 @@ export class ExtensionLoader { this.syncExtensions(extensions); const receivedExtensionIds = extensions.map(([lensExtensionId]) => lensExtensionId); - + // Remove deleted extensions in renderer side only this.extensions.forEach((_, lensExtensionId) => { if (!receivedExtensionIds.includes(lensExtensionId)) { @@ -276,6 +276,12 @@ export class ExtensionLoader { } if (extEntrypoint !== "") { + if (!fs.existsSync(extEntrypoint)) { + console.log(`${logModule}: entrypoint ${extEntrypoint} not found, skipping ...`); + + return; + } + return __non_webpack_require__(extEntrypoint).default; } } catch (err) { diff --git a/src/main/index.ts b/src/main/index.ts index 8da9be1a01..b0ba60d029 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -103,7 +103,6 @@ app.on("ready", async () => { } extensionLoader.init(); - extensionDiscovery.init(); windowManager = WindowManager.getInstance(proxyPort); @@ -111,6 +110,9 @@ app.on("ready", async () => { try { const extensions = await extensionDiscovery.load(); + // Start watching after bundled extensions are loaded + extensionDiscovery.watchExtensions(); + // Subscribe to extensions that are copied or deleted to/from the extensions folder extensionDiscovery.events.on("add", (extension: InstalledExtension) => { extensionLoader.addExtension(extension); @@ -122,6 +124,8 @@ app.on("ready", async () => { extensionLoader.initExtensions(extensions); } catch (error) { dialog.showErrorBox("Lens Error", `Could not load extensions${error?.message ? `: ${error.message}` : ""}`); + console.error(error); + console.trace(); } setTimeout(() => {