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

reschedule before AutoResetEvent.Reset #496

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Conversation

aoli-al
Copy link
Contributor

@aoli-al aoli-al commented Feb 28, 2024

This fixes #495

public static async Task TestConcurrentAccountCreation()
{
    var threadReady = new AutoResetEvent(false);
    var autoResetEvent1 = new AutoResetEvent(false);

    var task = Task.Run(() =>
    {
        threadReady.WaitOne(); 
        autoResetEvent1.Set();
    });

    threadReady.Set();
    Thread.Yield();  // Without this forced interleave, coyote can not identify the deadlock.
    autoResetEvent1.Reset();
    autoResetEvent1.WaitOne();

    await Task.WhenAll(task);
}

Manually tested with the above code. Can confirm coyote successfully identifies the bug.

@akashlal
Copy link
Collaborator

akashlal commented Mar 1, 2024

Thanks -- can you add the corresponding test for deadlock as well? Best place for it is here:
https://github.com/microsoft/coyote/blob/main/Tests/Tests.BugFinding/Synchronization/AutoResetEventTests.cs

@aoli-al
Copy link
Contributor Author

aoli-al commented Mar 1, 2024

@microsoft-github-policy-service agree

@pdeligia
Copy link
Member

pdeligia commented Mar 1, 2024

This is great addition @aoli-al , thanks for adding the test too! Just kicked off CI to run on this

@pdeligia pdeligia changed the title Reschedule before AutoResetEvent.Reset reschedule before AutoResetEvent.Reset Mar 1, 2024
@pdeligia pdeligia added the area-systematic-testing Issues related to systematic testing label Mar 1, 2024
@pdeligia pdeligia self-requested a review March 1, 2024 18:09
@pdeligia
Copy link
Member

pdeligia commented Mar 1, 2024

Could you also update the hashes in this test script: https://github.com/microsoft/coyote/blob/main/Tests/compare-rewriting-diff-logs.ps1

This is because the new added test changed the expected binary-rewriting hash of the Tests.BugFinding project, which Coyote CI validates to make sure binary-rewriting doesn't break unexpectedly. See:

Error: The 'Tests.BugFinding' project's IL diff hash '786F4C49E4C7361F9D91BA0DC61E05F33E3E04BB284ACF204BB9DBD73498AFEB' is not the expected '0112C27DC0C301BA980E598C484CFABA4F881F5E91FDD74107ADE0002A6901A4'.

in CI: https://github.com/microsoft/coyote/actions/runs/8113588744/job/22183054931?pr=496

@pdeligia
Copy link
Member

pdeligia commented Mar 1, 2024

If everything passes, I will merge this. It was great that you added this feature under the IsLockAccessRaceCheckingEnabled this was the right thing to do!

@aoli-al
Copy link
Contributor Author

aoli-al commented Mar 2, 2024

Is it expected that Coyote will generate different .diff.json on different platforms? I've tried to run compare-rewriting-diff-logs.ps1 on Ubuntu 22.04 and Windows 11 and got different results on different platforms.

@pdeligia
Copy link
Member

pdeligia commented Mar 4, 2024

Is it expected that Coyote will generate different .diff.json on different platforms? I've tried to run compare-rewriting-diff-logs.ps1 on Ubuntu 22.04 and Windows 11 and got different results on different platforms.

Great observation, yea exactly what you said happens. To simplify this we just run this specific check on windows only (${{ matrix.platform == 'windows-latest' }}).

@aoli-al
Copy link
Contributor Author

aoli-al commented Mar 5, 2024

2024-03-04T18:36:58.0613449Z   Failed Microsoft.Coyote.BugFinding.Tests.ThreadRunTests.TestThreadStartAndJoinStress [74 ms]
2024-03-04T18:36:58.0613628Z   Error Message:
2024-03-04T18:36:58.0614179Z    State of thread '312' is Background, Stopped instead of Stopped.
2024-03-04T18:36:58.0614195Z 
2024-03-04T18:36:58.0614337Z Expected: True
2024-03-04T18:36:58.0614479Z Actual:   False
2024-03-04T18:36:58.0615186Z    at Xunit.Assert.True(Nullable`1 condition, String userMessage) in /_/src/xunit.assert/Asserts/BooleanAsserts.cs:line 132
2024-03-04T18:36:58.0615702Z    at Xunit.Assert.True(Boolean condition, String userMessage) in /_/src/xunit.assert/Asserts/BooleanAsserts.cs:line 116
2024-03-04T18:36:58.0616908Z    at Microsoft.Coyote.Tests.Common.BaseTest.RunSystematicTest(Delegate test, Configuration configuration, Action`1 startIterationCallBack, Action`1 endIterationCallBack) in D:\a\coyote\coyote\Tests\Tests.Common\BaseTest.cs:line 114
2024-03-04T18:36:58.0617059Z Expected: False
2024-03-04T18:36:58.0617200Z Actual:   True
2024-03-04T18:36:58.0617324Z   Stack Trace:
2024-03-04T18:36:58.0618789Z      at Microsoft.Coyote.Tests.Common.BaseTest.RunSystematicTest(Delegate test, Configuration configuration, Action`1 startIterationCallBack, Action`1 endIterationCallBack) in D:\a\coyote\coyote\Tests\Tests.Common\BaseTest.cs:line 121
2024-03-04T18:36:58.0620595Z    at Microsoft.Coyote.Tests.Common.BaseTest.RunSystematicTest(Action test, Configuration configuration, Action`1 startIterationCallBack, Action`1 endIterationCallBack) in D:\a\coyote\coyote\Tests\Tests.Common\BaseTest.cs:line 95
2024-03-04T18:36:58.0622078Z    at Microsoft.Coyote.Tests.Common.BaseTest.Test(Action test, Configuration configuration) in D:\a\coyote\coyote\Tests\Tests.Common\BaseTest.cs:line 47
2024-03-04T18:36:58.0623393Z    at Microsoft.Coyote.BugFinding.Tests.ThreadRunTests.TestThreadStartAndJoinStress() in D:\a\coyote\coyote\Tests\Tests.BugFinding\Threads\ThreadRunTests.cs:line 66
2024-03-04T18:36:58.0624204Z    at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
2024-03-04T18:36:58.0624895Z    at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Here is the log for the failing test. I'm not able to reproduce it on my machine. Is this test flaky?

@pdeligia
Copy link
Member

pdeligia commented Mar 5, 2024

Yea seems that test is somehow flaky. But orthogonal to your PR. Everything passed so I will approve and merge. Thanks for your contribution!

@pdeligia pdeligia merged commit 8abdd30 into microsoft:main Mar 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-systematic-testing Issues related to systematic testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reschdule after AutoResetEvent.Set
3 participants