Another distributed race condition, #1571 #1609
Conversation
fredericDelaporte
force-pushed
the
dtc
branch
2 times, most recently
from
e3eb373
to
b633a2d
Compare
| [Test, Explicit("Failing test and 100 iteration loop.")] | ||
| public void DistributedRace() | ||
| [Test(Description = "Transaction race, bool distributed"), Explicit("Failing test and 100 iteration loop.")] | ||
| public void TransactionRace([Values(false, true)] bool distributed) |
There was a problem hiding this comment.
Changing a bit the pattern for reducing test code duplication
| var resource = new EnlistResource(shouldRollBack, name, eventQueue); | ||
| if (durable) | ||
| Transaction.Current.EnlistDurable(Guid.NewGuid(), resource, EnlistmentOptions.None); | ||
| else | ||
| Transaction.Current.EnlistVolatile(resource, EnlistmentOptions.None); | ||
|
|
||
| Transaction.Current.TransactionCompleted += resource.Current_TransactionCompleted; |
There was a problem hiding this comment.
Just to check where this one is inserted in the 2PC. It appears, anywhere after the first phase: can occurs before second phase, concurrently to it, or after, varying from one test run to the other. Sometimes it is even on the same thread than a second phase (obviously not occurring concurrently those times).
| preparingEnlistment.Prepared(); | ||
| } | ||
| Thread.Sleep(1); | ||
| _eventQueue?.Enqueue(new TransactionEvent(_name + ": prepare phase end")); |
There was a problem hiding this comment.
More logs in order to detect more easily thing occurring concurrently. (Adding the thread number is an indication too, but who knows if those threads are launched concurrently or one after another with some synchronization mechanism?)
| public byte[] Promote() => null; | ||
| public void Initialize() {} | ||
| public void SinglePhaseCommit(SinglePhaseEnlistment singlePhaseEnlistment) {} | ||
| public void Rollback(SinglePhaseEnlistment singlePhaseEnlistment) {} |
There was a problem hiding this comment.
It appears I had messed them up again in the previous PR, sorry...
|
@fredericDelaporte thanks for this. However, now that we have official confirmation that the 2nd phase is asynchronous, I'm wondering if it still makes sense to have AssertEventQueueCoherent - your explicit tests are failing because we see in "incoherent" sequence of events, but we now know that this sequence is in fact the expected behavior... Shouldn't remove this check? |
|
BTW the same is probably true of the check that data has been committed to the table. Ideally, we should:
What do you think? If you agree, do you want to amend your PR? |
|
Yes those checks are no more really relevant. It should no more cause a failure, going to amend. |
|
Rebased, and changed as written above and in #1610 review. |
…lback race, enabling fixed race. npgsql#1571
|
Failure of the |
As said in #1571, test demonstrating another distributed race condition with Npgsql.