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.

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

ProfilesCache does not handle race conditions #2831

Closed
traeok opened this issue Apr 10, 2024 · 2 comments · Fixed by #2838
Closed

ProfilesCache does not handle race conditions #2831

traeok opened this issue Apr 10, 2024 · 2 comments · Fixed by #2838
Assignees
Labels
bug Something isn't working priority-high Production outage - this quarter or at least next quarter severity-high Bug for which there may be workaround but limits the usage of the Zowe for major use cases

Comments

@traeok
Copy link
Member

traeok commented Apr 10, 2024

Describe the bug

Although JavaScript and Node.js are single-threaded, multiple VS Code extensions can activate in parallel. Extensions that grab the static ProfilesCache instance from Zowe Explorer might compete with one another when performing operations on the cache (such as refresh), causing undefined behavior in the cache (such as missing or incorrect profile values, etc.)

To Reproduce

Have two or more extensions that access the ProfilesCache instance through Zowe Explorer's extension exports (vscode.extensions.getExtension("zowe.vscode-extension-for-zowe").exports). Try to call refresh on this cache instance for each extension, as soon as the extensions are activated.

Expected behavior

The ProfilesCache instance should fill the cache successfully during refresh, and return an instance with valid profiles.

Desktop (please complete the following information):

  • OS: Windows
  • Zowe Explorer Version: 2.15.2

Additional context

A few internal developers have reported this issue. Due to the nature of VS Code extensions and asynchronous code in Node.js, we likely need to add multi-threading support to prevent any problems that may surface from ProfilesCache race conditions.

@traeok traeok added the bug Something isn't working label Apr 10, 2024
Copy link

Thank you for creating a bug report.
We will investigate the bug and evaluate its impact on the product.
If you haven't already, please ensure you have provided steps to reproduce the bug and as much context as possible.

@roman-kupriyanov
Copy link
Contributor

Hi @traeok ! Thanks for creating the issue 🙏

I would like to add also that initForZowe() call itself can produce a multiple unnecessary refresh calls by the structure of its arguments.

The signature of this function expects a profileType and an optional list of the schemas:

public async initForZowe(
  profileType: string,
  profileTypeConfigurations?: zowe.imperative.ICommandProfileTypeConfiguration[]
): Promise<void> { }

This forces the extenders to call something like code below for each type they want to register (and some extensions include more than one types, including ours):

for (const profileType of extensionProfileTypes) {
  await zoweExplorerExtenderApi.initForZowe(profileType, [profileSchemas[profileType]];
  cache.registerCustomProfilesType(profileType);
}

To avoid re-registering of the same profile schemas on each iteration we have to run the initForZowe multiple times for each profile type and its schema and this, in its order, causes multiple unnecessary refreshes and disk re-reading, which might interrupt other extensions init process and also takes more time.

If it is possible, it would be great to be able to pass all the schemas and register them at once, thus to have only one refresh for all of them at once. The updated signature may look just like this:

public async initForZoweNew(
  profileTypeConfigurations: zowe.imperative.ICommandProfileTypeConfiguration[]
): Promise<void> { }

The schemas themselves already include the type field that may be taken from there. So extenders may just call it simply like this:

await zoweExplorerExtenderApi.initForZoweNew(profileSchemas);
profileSchemas.forEach(({ type }) => cache.registerCustomProfilesType(type))

Again, just an idea. Thanks for taking a look at this problem 😃

@t1m0thyj t1m0thyj self-assigned this Apr 11, 2024
@t1m0thyj t1m0thyj linked a pull request Apr 12, 2024 that will close this issue
16 tasks
@JTonda JTonda added priority-high Production outage - this quarter or at least next quarter severity-high Bug for which there may be workaround but limits the usage of the Zowe for major use cases labels Apr 16, 2024
@JTonda JTonda closed this as completed Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-high Production outage - this quarter or at least next quarter severity-high Bug for which there may be workaround but limits the usage of the Zowe for major use cases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants