search

AzureAD / microsoft-authentication-library-for-dotnet

Microsoft Authentication Library (MSAL) for .NET
https://aka.ms/msal-net
MIT License
1.39k stars 340 forks source link

[Investigate] AcquireTokenSilent_ValidATs_ParallelRequests_Async test seems to fail periodically when running on release builds #2319

Open trwalke opened 3 years ago

trwalke commented 3 years ago

AcquireTokenSilent_ValidATs_ParallelRequests_Async test seems to fail periodically when running on release build.

Logs and Network traces

Error: Assert.Fail failed. Measured performance time EXCEEDED. Max allowed: 2000ms. Elapsed: 2749. AcquireTokenSilent should take roughly 100ms for its internal logic, plus the time needed to access the cache and all the thread context switches

Trace: at Microsoft.Identity.Test.Common.Core.Helpers.PerformanceValidator.Dispose() in \tests\Microsoft.Identity.Test.Common\Core\Helpers\PerformanceValidator.cs:line 30 at Microsoft.Identity.Test.Unit.RequestsTests.ParallelRequestsTests.d__9.MoveNext() in \tests\Microsoft.Identity.Test.Unit\ParallelRequestsTests.cs:line 94 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() at Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.ThreadOperations.ExecuteWithAbortSafety(Action action)

Performance image

Which Version of MSAL are you using ? MSAL 4.24.0

MichelZ commented 2 years ago 
using (new PerformanceValidator(
                    NumberOfRequests * (2 * CacheAccessPenaltyMs + 100),

Is this calculation correct? It uses 10 requests, CacheAccessPenaltyMs is 50, that means it currently calculates:

10 * (2 * 50 + 100) = 2000

or should it be: 10 * (2 * (50 + 100)) = 3000 ? NumberOfRequests * (2 * (CacheAccessPenaltyMs + 100)

Where does the 2 * come from? What is it? https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/9fa7dfd18229100a2425aa9176ed124f10077b72/tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs#L79

Also, on the AcquireTokenSilent_ExpiredATs_ParallelRequests_Asynctest, while the text says 100ms for its internal logic, the test code actually allows 1000ms. Bug? https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/9fa7dfd18229100a2425aa9176ed124f10077b72/tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs#L154

MichelZ commented 2 years ago

@trwalke @bgavrilMS what do you think? Shall I PR this?

bgavrilMS commented 2 years ago

I think we should delete this test because we have better performance tests, especially around the cache.

Afair, the 2 comes from the fact that a token request makes 2 cache accesses (one for read, one for write)