Fix extension loader race conditions (#1815)

* fix extension loader race conditions
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* cleanup
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* fix tests
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* fix remove
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* ensure symlinked (dev) extensions are installed on boot
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

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();
 

src/extensions/extension-discovery.ts

@@ -55,6 +55,7 @@ export class ExtensionDiscovery {
   protected bundledFolderPath: string;
 
   private loadStarted = false;
+  private extensions: Map<string, InstalledExtension> = 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<Map<LensExtensionId, InstalledExtension>> {
+  async ensureExtensions(): Promise<Map<LensExtensionId, InstalledExtension>> {
     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() {

src/extensions/extension-installer.ts

@@ -30,12 +30,49 @@ export class ExtensionInstaller {
     return __non_webpack_require__.resolve("npm/bin/npm-cli");
   }
 
-  installDependencies(): Promise<void> {
-    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<void> {
+    // 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<void> {
+    // 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<void> {
+    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<void> {
-    // 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();

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) {

src/main/index.ts

@@ -103,7 +103,6 @@ app.on("ready", async () => {
   }
 
   extensionLoader.init();
-
   extensionDiscovery.init();
   windowManager = WindowManager.getInstance<WindowManager>(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(() => {