Skip to content

Commit

Permalink
fix: Improve BFT Tests Stability (gnolang#1385)
Browse files Browse the repository at this point in the history
ref gnolang#1320 

This draft PR demonstrates a way for fixing flaky BFT tests. 
As mentioned in gnolang#1320 the main issue is that some channels are not fully
consumed, leading to deadlocks. By using buffered channels as proxy in
state tests, we can easily avoid these deadlocks. However, I don't think
this is the best approach, as these channels were originally set to zero
(capacity) for good reasons, but perhaps it could make sense for testing
purposes.
In any case, if anyone has a better idea for a solution, your
suggestions are welcome.

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
  • Loading branch information
gfanton and moul authored Dec 7, 2023
1 parent 0f440aa commit 63ff15b
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 6 deletions.
38 changes: 37 additions & 1 deletion tm2/pkg/bft/consensus/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"path"
"path/filepath"
"reflect"
"sort"
"sync"
"testing"
Expand Down Expand Up @@ -245,15 +246,50 @@ func validatePrevoteAndPrecommit(t *testing.T, cs *ConsensusState, thisRound, lo
validatePrecommit(t, cs, thisRound, lockRound, privVal, votedBlockHash, lockedBlockHash)
}

func bufferedEventChannel(in <-chan events.Event, size int) (out chan events.Event) {
out = make(chan events.Event, size)
go func() {
defer close(out)
for evt := range in {
out <- evt
}
}()

return out
}

func subscribe(evsw events.EventSwitch, protoevent events.Event) <-chan events.Event {
name := reflect.ValueOf(protoevent).Type().Name()
listenerID := fmt.Sprintf("%s-%s", testSubscriber, name)
ch := events.SubscribeToEvent(evsw, listenerID, protoevent)

// Similar to subscribeToVoter, this modification introduces
// a buffered channel to ensures that events are consumed
// asynchronously, thereby avoiding the deadlock situation described in
// #1320 where the eventSwitch.FireEvent method was blocked.
bch := bufferedEventChannel(ch, 16)

return bch
}

func subscribeToVoter(cs *ConsensusState, addr crypto.Address) <-chan events.Event {
return events.SubscribeFiltered(cs.evsw, testSubscriber, func(event events.Event) bool {
ch := events.SubscribeFiltered(cs.evsw, testSubscriber, func(event events.Event) bool {
if vote, ok := event.(types.EventVote); ok {
if vote.Vote.ValidatorAddress == addr {
return true
}
}
return false
})

// This modification addresses the deadlock issue outlined in issue
// #1320. By creating a buffered channel, we ensure that events are
// consumed even if the main thread is blocked. This prevents the
// deadlock that occurred when eventSwitch.FireEvent was blocked due to
// no available consumers for the event.
bch := bufferedEventChannel(ch, 16)

return bch
}

// -------------------------------------------------------------------------------
Expand Down
5 changes: 0 additions & 5 deletions tm2/pkg/bft/consensus/state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

cstypes "github.com/gnolang/gno/tm2/pkg/bft/consensus/types"
"github.com/gnolang/gno/tm2/pkg/bft/types"
"github.com/gnolang/gno/tm2/pkg/events"
p2pmock "github.com/gnolang/gno/tm2/pkg/p2p/mock"
"github.com/gnolang/gno/tm2/pkg/random"
"github.com/gnolang/gno/tm2/pkg/testutils"
Expand Down Expand Up @@ -1778,7 +1777,3 @@ func TestStateOutputVoteStats(t *testing.T) {
case <-time.After(50 * time.Millisecond):
}
}

func subscribe(evsw events.EventSwitch, protoevent events.Event) <-chan events.Event {
return events.SubscribeToEvent(evsw, testSubscriber, protoevent)
}

0 comments on commit 63ff15b

Please sign in to comment.