Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Feb 11, 2022
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f89f08
Don't block other extensions from loading if one activation fails.
panuhorsmalahti Feb 3, 2022
95cef8b
Handle synchronous errors in onActivate
panuhorsmalahti Feb 3, 2022
199e96a
Keep extensions that can't be activated in this.intances
panuhorsmalahti Feb 4, 2022
66b8a91
Return extensions that failed to be activated from loadExtensions to …
panuhorsmalahti Feb 4, 2022
5452405
Simplify style.
panuhorsmalahti Feb 4, 2022
16e8e6c
Simplify object. Remove unnecessary action.
panuhorsmalahti Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/extensions/extension-loader/extension-loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,11 +324,12 @@ export class ExtensionLoader {
return {
extId,
instance,
installedExtension: extension,
Nokel81 marked this conversation as resolved.
Show resolved Hide resolved
isBundled: extension.isBundled,
activated: instance.activate(),
};
} catch (err) {
logger.error(`${logModule}: activation extension error`, { ext: extension, err });
logger.error(`${logModule}: error loading extension`, { ext: extension, err });
}
} else if (!extension.isEnabled && alreadyInit) {
this.removeInstance(extId);
Expand All @@ -339,9 +340,16 @@ export class ExtensionLoader {
// Remove null values
.filter(extension => Boolean(extension));

// We first need to wait until each extension's `onActivate` is resolved,
// We first need to wait until each extension's `onActivate` is resolved or rejected,
// as this might register new catalog categories. Afterwards we can safely .enable the extension.
await Promise.all(extensions.map(extension => extension.activated));
await Promise.all(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@panuhorsmalahti panuhorsmalahti Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

@panuhorsmalahti panuhorsmalahti Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sounds good

extensions.map(extension =>
// If extension activation fails, log error
extension.activated.catch((error) => {
ixrock marked this conversation as resolved.
Show resolved Hide resolved
logger.error(`${logModule}: activation extension error`, { ext: extension.installedExtension, error });
}),
),
);

// Return ExtensionLoading[]
return extensions.map(extension => {
Expand Down
2 changes: 1 addition & 1 deletion src/extensions/lens-extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export class LensExtension {
}

@action
panuhorsmalahti marked this conversation as resolved.
Show resolved Hide resolved
activate() {
async activate(): Promise<void> {
return this.onActivate();
}

Expand Down