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

Intermittent failure for NonGenericCreationSpecs.CacheSuccessfulConstructor #1443

Closed
thomaslevesque opened this issue Sep 10, 2018 · 17 comments
Assignees
Milestone

Comments

@thomaslevesque
Copy link
Member

I've seen this several times in the past few days:

Failed! The process exited with code 1: FakeItEasy.Specs.NonGenericCreationSpecs.CacheSuccessfulConstructor() [09] And the parameterless constructor was only called once [FAIL]

So I guess the concurrency issues are not completely fixed yet...

@blairconrad
Copy link
Member

Oh, bother. I thought the fix (wait before faking, release the lock at the end of the test) would fix it, and it seemed to in my tests. I'll take a look again.

@thomaslevesque
Copy link
Member Author

BTW, the failed assertion message is:

FluentAssertions.Execution.AssertionFailedException : Expected value to be 1, but found 2.

@thomaslevesque
Copy link
Member Author

I don't know if it's related to your fix, but there seems to be a deadlock in the specs. I've seen it twice when building from the command line, it remains stuck at this line:

Starting test execution, please wait...

@blairconrad
Copy link
Member

Well, it's reasonable to believe that it's related to my fix. But I ran 300 iterations without a hitch.
I'll start some trials. I wonder if --blame would help identify.

Hmmm. I used a TryEnter with a 30 second delay. Shouldn't deadlock for more than 2 * number of test frameworks minutes.

@blairconrad
Copy link
Member

Are you working off develop? Do you see the problem in any particular framework?

@thomaslevesque
Copy link
Member Author

Are you working off develop?

Yes

Do you see the problem in any particular framework?

I don't know... It only happened twice, and I didn't keep the output.

@blairconrad
Copy link
Member

blairconrad commented Sep 14, 2018

Ugh. Reproduced. In 461, at least. --blame did not help at all.

I wonder if the issue is that

Monitor has thread affinity. That is, a thread that entered the monitor must exit by calling Exit or Wait.

I don't understand xBehave/xUnit enough to know whether it would always call the exit on the same thread as the one that called TryEnter.

Even still, all the other TryEnters should time out. I'm very confused.

@thomaslevesque
Copy link
Member Author

Are you working off develop?

Yes

In fact, I'm on my netstandard20 branch, which is based on develop. So I guess it's possible that it only happens only on this branch.

@thomaslevesque
Copy link
Member Author

I don't understand xBehave/xUnit enough to know whether it would always call the exit on the same thread.

I think it should, at least as long as we're not using async code.

@thomaslevesque
Copy link
Member Author

Are you working off develop?

Yes

Sorry, in fact that's not even true. I was working off my local develop branch, which was out of date...

I'll rebase

@thomaslevesque
Copy link
Member Author

I ran the specs a few times, and haven't seen the deadlock again yet.

@blairconrad
Copy link
Member

I'm at work, and it's showing up by about the 30th run:

$iteration = 0
while ( $True ) {
    ++$iteration
    echo "$(Get-Date -Format g): Iteration $iteration"
    .\build.cmd -s spec
}

I've a better machine here. Maybe at home it was single-threading more often. Although I could force the old failure before my change.

@blairconrad
Copy link
Member

Fun fact. I just rolled back to before my change and dropped the steps that compare the contructor call counts and… hung.

@thomaslevesque
Copy link
Member Author

Maybe it would be easier to just remove the synchronization code and make GenericDummyCreationSpecs and NonGenericDummyCreationSpecs part of the same xUnit collection, so that they just don't run together?

@blairconrad
Copy link
Member

@thomaslevesque, I had been considering something like that, but I don't think it's the answer. See #1451.

@thomaslevesque
Copy link
Member Author

Well, even it it's not actually this test that fails, it might still be a better approach than manual synchronization.

@blairconrad
Copy link
Member

This change has been released as part of FakeItEasy 4.9.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants