Skip to content

Commit

Permalink
refactor(share): use atomic.Int64 and fix 'racy' bugs (celestiaorg#3478)
Browse files Browse the repository at this point in the history
Moving from raw `int64` and atomic operations to the `atomic.Int64`
type. As a result fixing a few places where we were accessing atomic
variables non-atomically. Which isn't a problem 'cause there were only
reads BUT these places would reported by race detector and fail our
tests for not a big reason.
  • Loading branch information
cristaloleg authored Jun 8, 2024
1 parent bdac1d7 commit fe9c92c
Showing 1 changed file with 20 additions and 18 deletions.
38 changes: 20 additions & 18 deletions share/ipld/namespace_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,16 @@ type NamespaceData struct {

func NewNamespaceData(maxShares int, namespace share.Namespace, options ...Option) *NamespaceData {
data := &NamespaceData{
// we don't know where in the tree the leaves in the namespace are,
// so we keep track of the bounds to return the correct slice
// maxShares acts as a sentinel to know if we find any leaves
bounds: fetchedBounds{int64(maxShares), 0},
maxShares: maxShares,
namespace: namespace,
}

// we don't know where in the tree the leaves in the namespace are,
// so we keep track of the bounds to return the correct slice
// maxShares acts as a sentinel to know if we find any leaves
data.bounds.lowest.Store(int64(maxShares))
data.bounds.highest.Store(0)

for _, opt := range options {
opt(data)
}
Expand Down Expand Up @@ -107,7 +109,7 @@ func (n *NamespaceData) addLeaf(pos int, nd ipld.Node) {

// noLeaves checks that there are no leaves under the given root in the given namespace.
func (n *NamespaceData) noLeaves() bool {
return n.bounds.lowest == int64(n.maxShares)
return n.bounds.lowest.Load() == int64(n.maxShares)
}

type direction int
Expand Down Expand Up @@ -138,7 +140,7 @@ func (n *NamespaceData) Leaves() []ipld.Node {
if n.leaves == nil || n.noLeaves() || n.isAbsentNamespace.Load() {
return nil
}
return n.leaves[n.bounds.lowest : n.bounds.highest+1]
return n.leaves[n.bounds.lowest.Load() : n.bounds.highest.Load()+1]
}

// Proof returns proofs within the bounds in case if `WithProofs` option was passed,
Expand All @@ -160,17 +162,17 @@ func (n *NamespaceData) Proof() *nmt.Proof {

if n.isAbsentNamespace.Load() {
proof := nmt.NewAbsenceProof(
int(n.bounds.lowest),
int(n.bounds.highest)+1,
int(n.bounds.lowest.Load()),
int(n.bounds.highest.Load())+1,
nodes,
NamespacedSha256FromCID(n.absenceProofLeaf.Cid()),
NMTIgnoreMaxNamespace,
)
return &proof
}
proof := nmt.NewInclusionProof(
int(n.bounds.lowest),
int(n.bounds.highest)+1,
int(n.bounds.lowest.Load()),
int(n.bounds.highest.Load())+1,
nodes,
NMTIgnoreMaxNamespace,
)
Expand Down Expand Up @@ -311,24 +313,24 @@ func (n *NamespaceData) collectNDWithProofs(j job, links []*ipld.Link) []job {
}

type fetchedBounds struct {
lowest int64
highest int64
lowest atomic.Int64
highest atomic.Int64
}

// update checks if the passed index is outside the current bounds,
// and updates the bounds atomically if it extends them.
func (b *fetchedBounds) update(index int64) {
lowest := atomic.LoadInt64(&b.lowest)
lowest := b.lowest.Load()
// try to write index to the lower bound if appropriate, and retry until the atomic op is successful
// CAS ensures that we don't overwrite if the bound has been updated in another goroutine after the
// comparison here
for index < lowest && !atomic.CompareAndSwapInt64(&b.lowest, lowest, index) {
lowest = atomic.LoadInt64(&b.lowest)
for index < lowest && !b.lowest.CompareAndSwap(lowest, index) {
lowest = b.lowest.Load()
}
// we always run both checks because element can be both the lower and higher bound
// for example, if there is only one share in the namespace
highest := atomic.LoadInt64(&b.highest)
for index > highest && !atomic.CompareAndSwapInt64(&b.highest, highest, index) {
highest = atomic.LoadInt64(&b.highest)
highest := b.highest.Load()
for index > highest && !b.highest.CompareAndSwap(highest, index) {
highest = b.highest.Load()
}
}

0 comments on commit fe9c92c

Please sign in to comment.