Fix race condition on childer.

hamt/hamt.go

@@ -171,7 +171,11 @@ func (ds *Shard) Node() (ipld.Node, error) {
  continue
  }
 
- ch := ds.childer.child(sliceIndex)
+ chl := ds.childer.childOrLink(sliceIndex)
+
+ ch := chl.child
+ lnk := chl.link
+
  if ch != nil {
  clnk, err := ch.Link()
  if err != nil {
@@ -184,7 +188,6 @@ func (ds *Shard) Node() (ipld.Node, error) {
  }
  } else {
  // child unloaded, just copy in link with updated name
- lnk := ds.childer.link(sliceIndex)
  label := lnk.Name[ds.maxpadlen:]
 
  err := out.AddRawLink(ds.linkNamePrefix(childIndex)+label, lnk)
@@ -415,46 +418,55 @@ type listCidsAndShards struct {
 func (ds *Shard) walkChildren(processLinkValues func(formattedLink *ipld.Link) error) (*listCidsAndShards, error) {
  res := &listCidsAndShards{}
 
- for idx, lnk := range ds.childer.links {
- if nextShard := ds.childer.children[idx]; nextShard == nil {
- lnkLinkType, err := ds.childLinkType(lnk)
- if err != nil {
+ for i := 0; i < ds.childer.length(); i++ {
+ chl := ds.childer.childOrLink(i)
+
+ lnk := chl.link
+ nextShard := chl.child
+
+ if nextShard != nil && nextShard.val != nil {
+ formattedLink := &ipld.Link{
+ Name: nextShard.key,
+ Size: nextShard.val.Size,
+ Cid: nextShard.val.Cid,
+ }
+ if err := processLinkValues(formattedLink); err != nil {
  return nil, err
  }
 
- switch lnkLinkType {
- case shardValueLink:
- sv, err := ds.makeShardValue(lnk)
- if err != nil {
- return nil, err
- }
- formattedLink := sv.val
- formattedLink.Name = sv.key
+ continue
+ }
 
- if err := processLinkValues(formattedLink); err != nil {
- return nil, err
- }
- case shardLink:
- res.cids = append(res.cids, lnk.Cid)
- default:
- return nil, fmt.Errorf("unsupported shard link type")
+ if nextShard != nil {
+ res.shards = append(res.shards, nextShard)
+
+ continue
+ }
+
+ lnkLinkType, err := ds.childLinkType(lnk)
+ if err != nil {
+ return nil, err
+ }
+
+ switch lnkLinkType {
+ case shardValueLink:
+ sv, err := ds.makeShardValue(lnk)
+ if err != nil {
+ return nil, err
  }
+ formattedLink := sv.val
+ formattedLink.Name = sv.key
 
- } else {
- if nextShard.val != nil {
- formattedLink := &ipld.Link{
- Name: nextShard.key,
- Size: nextShard.val.Size,
- Cid: nextShard.val.Cid,
- }
- if err := processLinkValues(formattedLink); err != nil {
- return nil, err
- }
- } else {
- res.shards = append(res.shards, nextShard)
+ if err := processLinkValues(formattedLink); err != nil {
+ return nil, err
  }
+ case shardLink:
+ res.cids = append(res.cids, lnk.Cid)
+ default:
+ return nil, fmt.Errorf("unsupported shard link type")
  }
  }
+
  return res, nil
 }
 
@@ -707,7 +719,11 @@ func (ds *Shard) swapValue(ctx context.Context, hv *hashBits, key string, value
  // shard in case an implementation is broken.
 
  // Have we loaded the child? Prefer that.
- schild := child.childer.child(0)
+ chl := child.childer.childOrLink(0)
+
+ schild := chl.child
+ slnk := chl.link
+
  if schild != nil {
  if schild.isValueNode() {
  ds.childer.set(schild, i)
@@ -716,7 +732,6 @@ func (ds *Shard) swapValue(ctx context.Context, hv *hashBits, key string, value
  }
 
  // Otherwise, work with the link.
- slnk := child.childer.link(0)
  var lnkType linkType
  lnkType, err = child.childer.sd.childLinkType(slnk)
  if err != nil {
@@ -750,9 +765,14 @@ type childer struct {
  dserv ipld.DAGService
  bitfield bitfield.Bitfield
 
- // Only one of links/children will be non-nil for every child/link.
- links []*ipld.Link
- children []*Shard
+ mu sync.RWMutex
+ children []childOrLink
+}
+
+// childOrLink is a data structure that will contains a link OR a child.
+type childOrLink struct {
+ link *ipld.Link
+ child *Shard
 }
 
 func newChilder(ds ipld.DAGService, size int) *childer {
@@ -763,11 +783,13 @@ func newChilder(ds ipld.DAGService, size int) *childer {
 }
 
 func (s *childer) makeChilder(data []byte, links []*ipld.Link) *childer {
- s.children = make([]*Shard, len(links))
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
  s.bitfield.SetBytes(data)
- if len(links) > 0 {
-  s.links = make([]*ipld.Link, len(links))
- copy(s.links, links)
+
+ for _, l := range links {
+ s.children = append(s.children, childOrLink{link: l})
  }
 
  return s
@@ -778,12 +800,11 @@ func (s *childer) sliceIndex(childIndex int) (sliceIndex int) {
  return s.bitfield.OnesBefore(childIndex)
 }
 
-func (s *childer) child(sliceIndex int) *Shard {
- return s.children[sliceIndex]
-}
+func (s *childer) childOrLink(sliceIndex int) childOrLink {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
 
-func (s *childer) link(sliceIndex int) *ipld.Link {
- return s.links[sliceIndex]
+ return s.children[sliceIndex]
 }
 
 func (s *childer) insert(key string, lnk *ipld.Link, idx int) error {
@@ -795,8 +816,12 @@ func (s *childer) insert(key string, lnk *ipld.Link, idx int) error {
  i := s.sliceIndex(idx)
  sd := &Shard{key: key, val: lnk}
 
- s.children = append(s.children[:i], append([]*Shard{sd}, s.children[i:]...)...)
- s.links = append(s.links[:i], append([]*ipld.Link{nil}, s.links[i:]...)...)
+ s.mu.Lock()
+
+ s.children = append(s.children[:i], append([]childOrLink{{child: sd}}, s.children[i:]...)...)
+
+ s.mu.Unlock()
+
  // Add a `nil` placeholder in `links` so the rest of the entries keep the same
  // index as `children`.
  s.bitfield.SetBit(idx)
@@ -805,13 +830,17 @@ func (s *childer) insert(key string, lnk *ipld.Link, idx int) error {
 }
 
 func (s *childer) set(sd *Shard, i int) {
- s.children[i] = sd
- s.links[i] = nil
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
+ s.children[i] = childOrLink{child: sd}
 }
 
 func (s *childer) setLink(lnk *ipld.Link, i int) {
- s.children[i] = nil
- s.links[i] = lnk
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
+ s.children[i] = childOrLink{link: lnk}
 }
 
 func (s *childer) rm(childIndex int) error {
@@ -821,11 +850,12 @@ func (s *childer) rm(childIndex int) error {
  return err
  }
 
+ s.mu.Lock()
+
  copy(s.children[i:], s.children[i+1:])
  s.children = s.children[:len(s.children)-1]
 
- copy(s.links[i:], s.links[i+1:])
- s.links = s.links[:len(s.links)-1]
+ s.mu.Unlock()
 
  s.bitfield.UnsetBit(childIndex)
 
@@ -840,18 +870,17 @@ func (s *childer) get(ctx context.Context, sliceIndex int) (*Shard, error) {
  return nil, err
  }
 
- c := s.child(sliceIndex)
- if c != nil {
- return c, nil
+ c := s.childOrLink(sliceIndex)
+ if c.child != nil {
+ return c.child, nil
  }
 
- return s.loadChild(ctx, sliceIndex)
+ return s.loadChild(ctx, c.link, sliceIndex)
 }
 
 // loadChild reads the i'th child node of this shard from disk and returns it
 // as a 'child' interface
-func (s *childer) loadChild(ctx context.Context, sliceIndex int) (*Shard, error) {
- lnk := s.link(sliceIndex)
+func (s *childer) loadChild(ctx context.Context, lnk *ipld.Link, sliceIndex int) (*Shard, error) {
  lnkLinkType, err := s.sd.childLinkType(lnk)
  if err != nil {
  return nil, err
@@ -910,9 +939,5 @@ func (s *childer) check(sliceIndex int) error {
  return fmt.Errorf("invalid index passed to operate children (likely corrupt bitfield)")
  }
 
- if len(s.children) != len(s.links) {
- return fmt.Errorf("inconsistent lengths between children array and Links array")
- }
-
  return nil
 }

hamt/hamt_test.go

@@ -6,6 +6,7 @@ import (
  "math/rand"
  "os"
  "sort"
+ "sync"
  "testing"
  "time"
 
@@ -710,3 +711,32 @@ func TestHamtBadSize(t *testing.T) {
  t.Fatal("should have failed to construct hamt with bad size")
  }
 }
+
+func TestChilderModifyChildrenAndLinks(t *testing.T) {
+ ds := mdtest.Mock()
+ sh, _ := NewShard(ds, 256)
+
+ ch := newChilder(ds, 1024)
+ ch.makeChilder(nil, []*ipld.Link{{}})
+
+ var wg sync.WaitGroup
+ for i := 0; i < 100000; i++ {
+ wg.Add(2)
+ go func() {
+ ch.set(sh, 0)
+ wg.Done()
+ }()
+ go func() {
+ ch.setLink(&ipld.Link{}, 0)
+ wg.Done()
+ }()
+
+ chl := ch.childOrLink(0)
+ if chl.child == nil && chl.link == nil {
+ t.Fatal("both slices are nil", i)
+ }
+
+ }
+
+ wg.Wait()
+}