[Identity] Fix InteractiveBrowserCredential.GetToken deadlock with embedded browser #19864
Conversation
…ing embedded browser
ghost
added
the
| .ExecuteAsync(async, cancellationToken) | ||
| .ConfigureAwait(false); | ||
| .ExecuteAsync(cancellationToken) | ||
| #pragma warning disable AZC0102 // Do not use GetAwaiter().GetResult(). |
There was a problem hiding this comment.
Works, but I'm curious why the EnsureCompleted extension method can't be used here to avoid the warning.
|
We still end up blocking the UI thread even when offloading the call to the thread pool. I'd like us to understand why that fixes things. |
| if (async) | ||
|
|
||
| #pragma warning disable AZC0109 // Misuse of 'async' parameter. | ||
| if (!async && !IdentityCompatSwitches.DisableInteractiveBrowserThreadpoolExecution) |
There was a problem hiding this comment.
Is there a reasonable way to test this switch behavior?
There was a problem hiding this comment.
I was planning on adding a test for the IdentityCompatSwitches directly to verify we're reading the value properly. I could also add a test hook of some sort to allow us to verify we honor it here. If you have thoughts on a reasonable approach I'd welcome ideas.
There was a problem hiding this comment.
In the past I wrote an IDisposable that saved current and set a new environment variable, then on Dispose reset it. Unless the process crashes, it has generally worked well.
| .WithPrompt(prompt) | ||
| .WithClaims(claims) | ||
| .ExecuteAsync(cancellationToken) | ||
| .ConfigureAwait(false); | ||
| #pragma warning disable AZC0102 // Do not use GetAwaiter().GetResult(). | ||
| .ConfigureAwait(false)).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Nit: needs to be tabbed one more to align with proper scope.
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps: For data-plane/track 2 SDKs Issue the following command as a pull request comment:
For track 1 management-plane SDKsPlease open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification. |
| string expToken = Guid.NewGuid().ToString(); | ||
| DateTimeOffset expExpiresOn = DateTimeOffset.UtcNow.AddMinutes(5); | ||
|
|
||
| AppContext.SetSwitch("Azure.Identity.DisableInteractiveBrowserThreadpoolExecution", appSwitchSet); |
There was a problem hiding this comment.
don't we need to set DisableInteractiveThreadpoolExecutionEnvVar also?
There was a problem hiding this comment.
I tried setting both. The problem is once the AppContext switch is set there is no unsetting it, you can set it to false but then AppContext.TryGetSwitch still successfully reads the switch as false so we don't fall through to using the environment variable. I could write a long test which sequentially tests nothing set, then the environment variable, then the AppContext switch if you think it's warranted.
I tested these changes manually with a WinForms application, I'm not sure how we could automate the testing of it. I was able to reproduce the deadlock, and verify that this does fix the deadlock, Possibly there's a better approach than using
Task.Runand I'm open to suggestion.Fixes #18418