Skip to content

Fix for concurrent iframe calls #6962

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

Merged
merged 9 commits into from
Apr 3, 2024
Merged

Fix for concurrent iframe calls #6962

merged 9 commits into from
Apr 3, 2024

Conversation

tnorling
Copy link
Collaborator

When multiple acquireTokenSilent requests are made in parallel there is a chance that they will all need to fallback to the iframe flow. It is both unnecessary for more than 1 iframe call to be made and results in a perf and reliability degradation for all calls. This PR introduces a mechanism to track in progress iframe calls and cause subsequent requests to wait before retrying the cache and/or RT redemption.

Note: Telemetry dashboards will need to be updated after this change is released to avoid counting awaited iframe calls against our perf metrics more than once.

Note: This PR does not make any changes to ssoSilent - follow up work should add ssoSilent calls to the active request tracking variable and log warnings when more than 1 ssoSilent requests are made but should not block the calls.

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Mar 20, 2024
@tnorling tnorling marked this pull request as ready for review March 27, 2024 21:54
awaitIframeCorrelationId: activeCorrelationId,
});

// Await for errors first so we can distinguish errors thrown by activePromise versus errors thrown by .then below
Copy link
Member

Choose a reason for hiding this comment

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

Comment incomplete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont think so - do I need to rephrase?

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Please leave docs for ssoSilent use case, for users who are using this API instead of acquireTokenSilent - It is anti pattern but I agree that blocking them is more harmful for prod changes.

@tnorling tnorling merged commit 8a4aa37 into dev Apr 3, 2024
44 of 45 checks passed
@tnorling tnorling deleted the fix-iframe-concurrency branch April 3, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants