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

Potential race condition any time an index Set() is followed by SetSeq() #32

Open
KyleMaas opened this issue Jan 24, 2023 · 0 comments
Open

Comments

@KyleMaas
Copy link

Related to #30

These both make modifications to the database, each in its own separate transaction, but they can be called independently and the sequence number is often used by other concurrent processes giving rise to a huge opportunity for corrupting the database in a concurrent usage scenario:

func (idx *index) Set(ctx context.Context, addr indexes.Addr, v interface{}) error {
var (
raw []byte
err error
)
if m, ok := v.(encoding.BinaryMarshaler); ok {
raw, err = m.MarshalBinary()
if err != nil {
return fmt.Errorf("error marshaling value using custom marshaler:%w", err)
}
} else {
raw, err = json.Marshal(v)
if err != nil {
return fmt.Errorf("error marshaling value using json marshaler:%w", err)
}
}
err = idx.db.Set([]byte(addr), raw)
if err != nil {
return fmt.Errorf("error in store:%w", err)
}
idx.l.Lock()
defer idx.l.Unlock()
obv, ok := idx.obvs[addr]
if ok {
err = obv.Set(v)
if err != nil {
return fmt.Errorf("error setting value in observable:%w", err)
}
}
return nil
}

func (idx *index) SetSeq(seq int64) error {
var (
raw = make([]byte, 8)
err error
addr indexes.Addr = "__current_observable"
)
binary.BigEndian.PutUint64(raw, uint64(seq))
err = idx.db.Set([]byte(addr), raw)
if err != nil {
return fmt.Errorf("error during mkv update (%T): %w", seq, err)
}
idx.l.Lock()
defer idx.l.Unlock()
idx.curSeq = seq
return nil
}

I seem to recall this Set() followed by SetSeq() pattern used in go-ssb as well. We should probably have a way to do this in a single call which either uses a mutex or opens a transaction, writes both, and commits it. Right now anyone using Set() within a SinkIndex callback is going to be subject to this problem. And it readily hands the user a footgun by not even providing a mechanism to do this properly and forcing them to use this pattern externally as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant