Skip to content

Commit

Permalink
Wrapping request client calls with a read/write mutex (#3701)
Browse files Browse the repository at this point in the history
Refs #3699

Depends on #3704

It happens that the keep-core client panics when interacting with the
`go-electrum` library. We noticed a "concurrent write to websocket
connection" error was thrown when calling `GetLatestBlockHeight()`
function. The stack trace leads to gorilla/websocket/WriteMessage which
is called from the `go-electrum` library. The latest block height call
was already wrapped with a read mutex, but we should also wrap it with
the write mutex to prevent such concurrent errors.
  • Loading branch information
lukasz-zimnoch authored Sep 14, 2023
2 parents da8b4ac + da09f53 commit 5c2604c
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 4 deletions.
8 changes: 4 additions & 4 deletions pkg/bitcoin/electrum/electrum.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ var (
type Connection struct {
parentCtx context.Context
client *electrum.Client
clientMutex *sync.RWMutex
clientMutex *sync.Mutex
config Config
}

Expand All @@ -59,7 +59,7 @@ func Connect(parentCtx context.Context, config Config) (bitcoin.Chain, error) {
c := &Connection{
parentCtx: parentCtx,
config: config,
clientMutex: &sync.RWMutex{},
clientMutex: &sync.Mutex{},
}

if err := c.electrumConnect(); err != nil {
Expand Down Expand Up @@ -771,9 +771,9 @@ func requestWithRetry[K interface{}](
requestCtx, requestCancel := context.WithTimeout(ctx, c.config.RequestTimeout)
defer requestCancel()

c.clientMutex.RLock()
c.clientMutex.Lock()
r, err := requestFn(requestCtx, c.client)
c.clientMutex.RUnlock()
c.clientMutex.Unlock()

if err != nil {
return fmt.Errorf("request failed: [%w]", err)
Expand Down
43 changes: 43 additions & 0 deletions pkg/bitcoin/electrum/electrum_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,49 @@ func TestGetTransactionConfirmations_Negative_Integration(t *testing.T) {
})
}

// TODO: We should uncomment this test once https://github.com/checksum0/go-electrum/issues/10
// is fixed. This test was added to validate the fix of the following issue
// https://github.com/keep-network/keep-core/issues/3699 but at the same time
// made `panic: assignment to entry in nil map` happen very frequently which is
// disturbing during the development and running the existing integration tests.

// func TestGetLatestBlockHeightConcurrently_Integration(t *testing.T) {
// goroutines := 20

// for testName, testConfig := range testConfigs {
// t.Run(testName+"_get", func(t *testing.T) {
// electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
// defer cancelCtx()

// var wg sync.WaitGroup

// for i := 0; i < goroutines; i++ {
// wg.Add(1)

// go func() {
// result, err := electrum.GetLatestBlockHeight()

// if err != nil {
// t.Fatal(err)
// }

// if result == 0 {
// t.Errorf(
// "returned block height is 0",
// )
// }

// wg.Done()
// }()
// }

// wg.Wait()
// })

// // Passed if no "panic: concurrent write to websocket connection"
// }
// }

func TestGetLatestBlockHeight_Integration(t *testing.T) {
expectedBlockHeightRef := map[string]uint{}
results := map[string]map[string]uint{}
Expand Down

0 comments on commit 5c2604c

Please sign in to comment.