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

fix: add concurrency fence on traceContext to avoid data races #11117

Merged
merged 4 commits into from
Feb 8, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (store) [\#11117](https://github.com/cosmos/cosmos-sdk/pull/11117) Fix data race in store trace component
* (cli) [\#11065](https://github.com/cosmos/cosmos-sdk/pull/11065) Ensure the `tendermint-validator-set` query command respects the `-o` output flag.
* (grpc) [\#10985](https://github.com/cosmos/cosmos-sdk/pull/10992) The `/cosmos/tx/v1beta1/txs/{hash}` endpoint returns a 404 when a tx does not exist.
* (rosetta) [\#10340](https://github.com/cosmos/cosmos-sdk/pull/10340) Use `GenesisChunked(ctx)` instead `Genesis(ctx)` to get genesis block height
Expand Down
30 changes: 25 additions & 5 deletions store/rootmulti/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"math"
"sort"
"strings"
"sync"

iavltree "github.com/cosmos/iavl"
protoio "github.com/gogo/protobuf/io"
Expand Down Expand Up @@ -57,8 +58,9 @@ type Store struct {
initialVersion int64
removalMap map[types.StoreKey]bool

traceWriter io.Writer
traceContext types.TraceContext
traceWriter io.Writer
traceContext types.TraceContext
traceContextMutex sync.Mutex

interBlockCache types.MultiStorePersistentCache

Expand Down Expand Up @@ -329,6 +331,8 @@ func (rs *Store) SetTracer(w io.Writer) types.MultiStore {
// be overwritten. It is implied that the caller should update the context when
// necessary between tracing operations. It returns a modified MultiStore.
func (rs *Store) SetTracingContext(tc types.TraceContext) types.MultiStore {
rs.traceContextMutex.Lock()
defer rs.traceContextMutex.Unlock()
if rs.traceContext != nil {
for k, v := range tc {
rs.traceContext[k] = v
Expand All @@ -340,6 +344,22 @@ func (rs *Store) SetTracingContext(tc types.TraceContext) types.MultiStore {
return rs
}

func (rs *Store) getTracingContext() types.TraceContext {
rs.traceContextMutex.Lock()
defer rs.traceContextMutex.Unlock()

if rs.traceContext == nil {
return nil
}

ctx := types.TraceContext{}
for k, v := range rs.traceContext {
ctx[k] = v
}

return ctx
}

// TracingEnabled returns if tracing is enabled for the MultiStore.
func (rs *Store) TracingEnabled() bool {
return rs.traceWriter != nil
Expand Down Expand Up @@ -477,7 +497,7 @@ func (rs *Store) CacheMultiStore() types.CacheMultiStore {
for k, v := range rs.stores {
stores[k] = v
}
return cachemulti.NewStore(rs.db, stores, rs.keysByName, rs.traceWriter, rs.traceContext, rs.listeners)
return cachemulti.NewStore(rs.db, stores, rs.keysByName, rs.traceWriter, rs.getTracingContext(), rs.listeners)
}

// CacheMultiStoreWithVersion is analogous to CacheMultiStore except that it
Expand Down Expand Up @@ -507,7 +527,7 @@ func (rs *Store) CacheMultiStoreWithVersion(version int64) (types.CacheMultiStor
}
}

return cachemulti.NewStore(rs.db, cachedStores, rs.keysByName, rs.traceWriter, rs.traceContext, rs.listeners), nil
return cachemulti.NewStore(rs.db, cachedStores, rs.keysByName, rs.traceWriter, rs.getTracingContext(), rs.listeners), nil
}

// GetStore returns a mounted Store for a given StoreKey. If the StoreKey does
Expand Down Expand Up @@ -539,7 +559,7 @@ func (rs *Store) GetKVStore(key types.StoreKey) types.KVStore {
store := s.(types.KVStore)

if rs.TracingEnabled() {
store = tracekv.NewStore(store, rs.traceWriter, rs.traceContext)
store = tracekv.NewStore(store, rs.traceWriter, rs.getTracingContext())
}
if rs.ListeningEnabled(key) {
store = listenkv.NewStore(store, key, rs.listeners[key])
Expand Down
51 changes: 51 additions & 0 deletions store/rootmulti/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"math/rand"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -803,6 +804,56 @@ func TestCacheWraps(t *testing.T) {
require.IsType(t, cachemulti.Store{}, cacheWrappedWithListeners)
}

func TestTraceConcurrency(t *testing.T) {
db := dbm.NewMemDB()
multi := newMultiStoreWithMounts(db, types.PruneNothing)
err := multi.LoadLatestVersion()
require.NoError(t, err)

b := &bytes.Buffer{}
key := multi.keysByName["store1"]
tc := types.TraceContext(map[string]interface{}{"blockHeight": 64})

multi.SetTracer(b)
multi.SetTracingContext(tc)

cms := multi.CacheMultiStore()
store1 := cms.GetKVStore(key)
cw := store1.CacheWrapWithTrace(b, tc)
Copy link
Contributor

@roysc roysc Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the existence of this wrapper affect the outcome even though it's not used?

I'm trying to backport this fix for #10174 and want to make sure I fully understand the race condition.

Seems this has no effect and was WIP

_ = cw
require.NotNil(t, store1)

stop := make(chan struct{})
stopW := make(chan struct{})

go func(stop chan struct{}) {
for {
select {
case <-stop:
return
default:
store1.Set([]byte{1}, []byte{1})
cms.Write()
}
}
}(stop)

go func(stop chan struct{}) {
for {
select {
case <-stop:
return
default:
multi.SetTracingContext(tc)
}
}
}(stopW)

time.Sleep(3 * time.Second)
stop <- struct{}{}
stopW <- struct{}{}
}

func BenchmarkMultistoreSnapshot100K(b *testing.B) {
benchmarkMultistoreSnapshot(b, 10, 10000)
}
Expand Down