Skip to content

Commit

Permalink
fix: fixed data race in result (schemata caching)
Browse files Browse the repository at this point in the history
Signed-off-by: Frederic BIDON <[email protected]>
  • Loading branch information
fredbi committed Mar 4, 2024
1 parent e818230 commit 238809f
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 27 deletions.
62 changes: 45 additions & 17 deletions result.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ type Result struct {
// Schemata for slice items
itemSchemata []itemSchemata

cachedFieldSchemta map[FieldKey][]*spec.Schema
cachedItemSchemata map[ItemKey][]*spec.Schema
cachedFieldSchemata map[FieldKey][]*spec.Schema
cachedItemSchemata map[ItemKey][]*spec.Schema

wantsRedeemOnMerge bool
}
Expand Down Expand Up @@ -140,8 +140,8 @@ func (r *Result) RootObjectSchemata() []*spec.Schema {

// FieldSchemata returns the schemata which apply to fields in objects.
func (r *Result) FieldSchemata() map[FieldKey][]*spec.Schema {
if r.cachedFieldSchemta != nil {
return r.cachedFieldSchemta
if r.cachedFieldSchemata != nil {
return r.cachedFieldSchemata
}

ret := make(map[FieldKey][]*spec.Schema, len(r.fieldSchemata))
Expand All @@ -153,7 +153,8 @@ func (r *Result) FieldSchemata() map[FieldKey][]*spec.Schema {
ret[key] = append(ret[key], fs.schemata.multiple...)
}
}
r.cachedFieldSchemta = ret
r.cachedFieldSchemata = ret

return ret
}

Expand All @@ -177,7 +178,7 @@ func (r *Result) ItemSchemata() map[ItemKey][]*spec.Schema {
}

func (r *Result) resetCaches() {
r.cachedFieldSchemta = nil
r.cachedFieldSchemata = nil
r.cachedItemSchemata = nil
}

Expand All @@ -194,10 +195,11 @@ func (r *Result) mergeForField(obj map[string]interface{}, field string, other *
if r.fieldSchemata == nil {
r.fieldSchemata = make([]fieldSchemata, len(obj))
}
// clone other schemata, as other is about to be redeemed to the pool
r.fieldSchemata = append(r.fieldSchemata, fieldSchemata{
obj: obj,
field: field,
schemata: other.rootObjectSchemata,
schemata: other.rootObjectSchemata.Clone(),
})
}
if other.wantsRedeemOnMerge {
Expand All @@ -220,12 +222,14 @@ func (r *Result) mergeForSlice(slice reflect.Value, i int, other *Result) *Resul
if r.itemSchemata == nil {
r.itemSchemata = make([]itemSchemata, slice.Len())
}
// clone other schemata, as other is about to be redeemed to the pool
r.itemSchemata = append(r.itemSchemata, itemSchemata{
slice: slice,
index: i,
schemata: other.rootObjectSchemata,
schemata: other.rootObjectSchemata.Clone(),
})
}

if other.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(other)
}
Expand Down Expand Up @@ -272,17 +276,21 @@ func (r *Result) mergeWithoutRootSchemata(other *Result) {

if other.fieldSchemata != nil {
if r.fieldSchemata == nil {
r.fieldSchemata = other.fieldSchemata
} else {
r.fieldSchemata = append(r.fieldSchemata, other.fieldSchemata...)
r.fieldSchemata = make([]fieldSchemata, 0, len(other.fieldSchemata))
}
for _, field := range other.fieldSchemata {
field.schemata = field.schemata.Clone()
r.fieldSchemata = append(r.fieldSchemata, field)
}
}

if other.itemSchemata != nil {
if r.itemSchemata == nil {
r.itemSchemata = other.itemSchemata
} else {
r.itemSchemata = append(r.itemSchemata, other.itemSchemata...)
r.itemSchemata = make([]itemSchemata, 0, len(other.itemSchemata))
}
for _, field := range other.itemSchemata {
field.schemata = field.schemata.Clone()
r.itemSchemata = append(r.itemSchemata, field)
}
}
}
Expand Down Expand Up @@ -465,8 +473,8 @@ func (r *Result) cleared() *Result {
r.rootObjectSchemata.multiple = r.rootObjectSchemata.multiple[:0]
r.fieldSchemata = r.fieldSchemata[:0]
r.itemSchemata = r.itemSchemata[:0]
for k := range r.cachedFieldSchemta {
delete(r.cachedFieldSchemta, k)
for k := range r.cachedFieldSchemata {
delete(r.cachedFieldSchemata, k)
}
for k := range r.cachedItemSchemata {
delete(r.cachedItemSchemata, k)
Expand Down Expand Up @@ -502,7 +510,7 @@ func (s *schemata) Slice() []*spec.Schema {
return s.multiple
}

// appendSchemata appends the schemata in other to s. It mutated s in-place.
// appendSchemata appends the schemata in other to s. It mutates s in-place.
func (s *schemata) Append(other schemata) {
if other.one == nil && len(other.multiple) == 0 {
return
Expand Down Expand Up @@ -533,3 +541,23 @@ func (s *schemata) Append(other schemata) {
}
}
}

func (s schemata) Clone() schemata {
var clone schemata

if s.one != nil {
clone.one = new(spec.Schema)
*clone.one = *s.one
}

if len(s.multiple) > 0 {
clone.multiple = make([]*spec.Schema, len(s.multiple))
for idx := 0; idx < len(s.multiple); idx++ {
sp := new(spec.Schema)
*sp = *s.multiple[idx]
clone.multiple[idx] = sp
}
}

return clone
}
10 changes: 0 additions & 10 deletions schema_props_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,6 @@ func TestSchemaPropsValidator_EdgeCases(t *testing.T) {
res := s.Validate(data)
require.NotNil(t, res)
require.Empty(t, res.Errors)

/* TODO(fred)
t.Run("validator should run once", func(t *testing.T) {
// we should not do that: the pool chain list is populated with a duplicate: needs a reset
t.Cleanup(resetPools)
require.NotPanics(t, func() {
_ = s.Validate(data)
})
})
*/
})

t.Run("should NOT validate unformatted string", func(t *testing.T) {
Expand Down

0 comments on commit 238809f

Please sign in to comment.