etcd: make commit queue test deterministic#5117
etcd: make commit queue test deterministic#5117bhandras wants to merge 1 commit intolightningnetwork:masterfrom
Conversation
| ) | ||
|
|
||
| // Allow Tx1 to continue with the commit. | ||
| block.Unlock() |
There was a problem hiding this comment.
nit: might be simpler to use a ready channel, e.g.:
commit := func(...) {
...
if ready != nil {
<-ready
}
}
...
ready := make(chan struct{})
...
commit(_, _, ready)
...
close(ready)This approach will also work with more than one "free" transaction
There was a problem hiding this comment.
See my other comment. We can use a channel, but in the test example it's not possible to submit overlapping conflicting/non-conflicting txns without introducing a deadlock in the test (even if we resolve it later).
There was a problem hiding this comment.
I agree that using a channel to signal that the commit can continue is better than a mutex
| // We don't expect queue add to block as this txn will queue up after | ||
| // tx1. | ||
| q.Add( | ||
| commit("free", true), |
There was a problem hiding this comment.
This test seems a bit weaker now, in that it doesn't actually test for transaction reordering. If the commit queue simply executed the transactions in the order they are added, the test would pass. The new version doesn't test that concurrent non-conflicting transactions are executed immediately.
I think it makes sense to keep the current transactions, possibly even making the test stronger by naming the transactions free1 and free2 to assert their order.
There was a problem hiding this comment.
Without the sleeps it's not really possible to do this deterministically without submitting non-conflicting txns first since then the non-conflicting txn that should be executed inside the commit queue's read mutex will be behind the conflicting txn which waits on that mutex as a writer. See a simplified example: https://play.golang.org/p/66zi9waGZj-
| block *sync.Mutex) func() { | ||
|
|
||
| return func() { | ||
| if commit != nil { |
There was a problem hiding this comment.
can names this signalStart or something other that is more descriptive than commit
|
|
||
| if sleep { | ||
| time.Sleep(commitDuration) | ||
| if block != nil { |
There was a problem hiding this comment.
this needs a comment, same for the channel close above
| ) | ||
|
|
||
| // Allow Tx1 to continue with the commit. | ||
| block.Unlock() |
There was a problem hiding this comment.
I agree that using a channel to signal that the commit can continue is better than a mutex
|
Closing this as another solution has been implemented in: #5513 |
3 participants
Sometimes commit queue test fails due to timing issue introduced by sleeps. This PR changes the test to make it deterministic.