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

Jump to bottom

Flaky test Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProviderTests.MultipleThreadsForceRefresh #55227

Open
MackinnonBuck opened this issue Apr 19, 2024 · 4 comments · May be fixed by #55290
Open

Flaky test Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProviderTests.MultipleThreadsForceRefresh #55227

MackinnonBuck opened this issue Apr 19, 2024 · 4 comments · May be fixed by #55290
Assignees
@amcasey
Labels
area-dataprotection Includes: DataProtection test-failure

Comments

@MackinnonBuck
Copy link
Member

Failing Test(s)

  • Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProviderTests.MultipleThreadsForceRefresh

Error Message

Moq.MockException :
Expected invocation on the mock at most 9 times, but was 10 times: o => o.GetCacheableKeyRing(It.IsAny<DateTimeOffset>())


Configured setups:
ICacheableKeyRingProvider o => o.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)


Performed invocations:
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)
ICacheableKeyRingProvider.GetCacheableKeyRing(3/1/2015 12:00:00 AM +00:00)

Stacktrace

   at Moq.Mock.VerifyCalls(Mock targetMock, InvocationShape expectation, LambdaExpression expression, Times times, String failMessage)
   at Moq.Mock.Verify[T,TResult](Mock`1 mock, Expression`1 expression, Times times, String failMessage)
   at Moq.Mock`1.Verify[TResult](Expression`1 expression, Times times)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProviderTests.MultipleThreadsForceRefresh(Boolean failsToReadKeyRing) in /_/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs:line 870

Build
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Apr 19, 2024
@MackinnonBuck MackinnonBuck mentioned this issue Apr 19, 2024
Open
@MackinnonBuck MackinnonBuck added area-dataprotection Includes: DataProtection and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Apr 19, 2024
@amcasey amcasey self-assigned this Apr 19, 2024
@amcasey
Copy link
Member

amcasey commented Apr 19, 2024

That was fast.

@amcasey
Copy link
Member

amcasey commented Apr 19, 2024

Both failsToReadKeyRing: True and failsToReadKeyRing: False are affected. Seems more common on Ubuntu, but happens on Windows too.

I'm a little curious why this didn't fail in several CI runs during the PR itself.

@amcasey
Copy link
Member

amcasey commented Apr 19, 2024

FWIW, the test is racy, so I'm not that concerned that this could be a product bug.

@amcasey
Copy link
Member

amcasey commented Apr 19, 2024

As you might expect, it passes 1000X in a row locally. I'll try the mitigations I recommended when I wrote it. If they don't work, we can probably just delete the test.

amcasey added a commit to amcasey/aspnetcore that referenced this issue Apr 19, 2024
@amcasey
Sleep in MultipleThreadsForceRefresh
cf58f62 
...to increase the likelihood of two threads racing to enter the critical section.

Fixes dotnet#55227
@amcasey amcasey mentioned this issue Apr 19, 2024
Merged
amcasey added a commit to amcasey/aspnetcore that referenced this issue Apr 22, 2024
@amcasey
Delete flaky test MultipleThreadsForceRefresh
6f0260b 
I added it to do manual validation and kept it when it seemed to be stable.  To make it properly stable, we'd have to add a special test hook to let us manipulate lock usage, which doesn't seem worthwhile for the marginal coverage the test provides.

Fixes dotnet#55227
@amcasey amcasey linked a pull request Apr 22, 2024 that will close this issue
Open
amcasey added a commit that referenced this issue Apr 22, 2024
@amcasey
Sleep in MultipleThreadsForceRefresh (#55231)
22bf0d0 
* Sleep in MultipleThreadsForceRefresh

...to increase the likelihood of two threads racing to enter the critical section.

For #55227

* Also quarantine test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@amcasey amcasey

Labels
area-dataprotection Includes: DataProtection test-failure
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Delete flaky test MultipleThreadsForceRefresh amcasey/aspnetcore
2 participants
@MackinnonBuck @amcasey