Don't block other extensions from loading if one activation fails

[panuhorsmalahti]

• Fixes Extension activation error blocks other extensions #4804
• Adjust error message to be correct (error was with loading, not activation)
• The issue was that Promise.all would throw if just one extension's onActivate method would throw.

[Nokel81]

Given that we have already started all the activations above, why not just use a for loop and try...catch, then we don't need another field, or the filter, or the map. Just an array to push to.

[panuhorsmalahti]

I checked the original code, and it seems we want to return even extensions that fail to be activated from loadExtensions. I removed the filter and the field to simplify it.

[Nokel81]

I don't think we should really be mixing await and .catch here since we can easily avoid doing so.

I would suggest something like the following:

for (const extension of extensions) {
  try {
    await extension.activated;
  } catch (error) {
    logger.error(`${logModule}: activation extension error`, { ext: extension.installedExtension, error });
  }
}

Furthermore I think the use of Promise.all here is a case of using the wrong tool, since we don't actually care about the result. Only the errors (if there are any)

[Nokel81]

So looking at the code around this, we could easily make activate call enable, have the logging within LensExtension and then pass those promises directly to the return extensions.map(...) bellow, because we don't actually need this awaits here.

[panuhorsmalahti]

Promise.all is valid use here, because we want to wait until all the Promises have been resolved.

Your try / catch / await is okay.. but has a bit different semantics. It would wait until each extension has been activated, and only print the error in order (e.g. loading extensions A, B and C, if C fails, it would log error from C only after A and B are done). With Promise.all each error from activation will be logged immediately, which is slightly beneficial.

[Nokel81]

I would do the following:

protected async loadExtensions(installedExtensions: Map<string, InstalledExtension>, register: (ext: LensExtension) => Promise<Disposer[]>) {
  const extensions: ExtensionLoading[] = [];

  for (const [extId, ext] of installedExtensions) {
    const alreadyInit = this.instances.has(extId) || this.nonInstancesByName.has(ext.manifest.name);

    if (ext.isCompatible && ext.isEnabled && !alreadyInit) {
      try {
        const LensExtensionClass = this.requireExtension(ext);

        if (!LensExtensionClass) {
          this.nonInstancesByName.add(ext.manifest.name);

          continue;
        }

        const instance = this.dependencies.createExtensionInstance(
          LensExtensionClass,
          ext,
        );

        this.instances.set(extId, instance);

        extensions.push({
          isBundled: ext.isBundled,
          loaded: (async (): Promise<void> => {
            try {
              await instance.activate();
            } catch (error) {
              return void logger.error(`${logModule}: extension activation error`, { ext, error });
            }

            try {
              await instance.enable(register);
            } catch (error) {
              return void logger.error(`${logModule}: extension enabling error`, { ext, error });
            }
          })(),
        });
      } catch (error) {
        logger.error(`${logModule}: extension loading error`, { ext, error });
      }
    } else if (!ext.isEnabled && alreadyInit) {
      this.removeInstance(extId);
    }
  }

  return extensions;
}

[panuhorsmalahti]

That doesn't work for this reason:

    // We first need to wait until each extension's `onActivate` is resolved,
    // as this might register new catalog categories. Afterwards we can safely .enable the extension.

Your code example might call .enable before another extension is activated fully, thus causing error.

See earlier PR for some discussion:
#4702

[Nokel81]

If that parameter becomes IComputedValue<CatalogEntity | undefined> and you do an await when(() => Boolean(entity.get()); before that if, then the race condition would be fixed as well and it would also be fixed for non-bundled extensions.

[panuhorsmalahti]

I propose we fix that in another PR as that's becoming not related to the issue at hand.

[Nokel81]

Okay sounds good