Skip to content

Commit

Permalink
Fix data race on dynFields length in Builder.Clone (#72)
Browse files Browse the repository at this point in the history
Builder.Clone was reading the length of dynFields without holding
dynFieldsLock. This moves the read to be covered by the lock.
  • Loading branch information
vcabbage authored Jan 4, 2021
1 parent 040dd1c commit 0b5ed0c
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 4 deletions.
6 changes: 2 additions & 4 deletions libhoney.go
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,6 @@ func (b *Builder) Clone() *Builder {
Dataset: b.Dataset,
SampleRate: b.SampleRate,
APIHost: b.APIHost,
dynFields: make([]dynamicField, 0, len(b.dynFields)),
client: b.client,
}
newB.data = make(map[string]interface{})
Expand All @@ -927,9 +926,8 @@ func (b *Builder) Clone() *Builder {
// copy dynamic metric generators
b.dynFieldsLock.RLock()
defer b.dynFieldsLock.RUnlock()
for _, dynFd := range b.dynFields {
newB.dynFields = append(newB.dynFields, dynFd)
}
newB.dynFields = make([]dynamicField, len(b.dynFields))
copy(newB.dynFields, b.dynFields)
return newB
}

Expand Down
26 changes: 26 additions & 0 deletions libhoney_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,32 @@ func TestBuilderStaticFields(t *testing.T) {
testEquals(t, ev4.data["floatF"], nil)
}

func TestBuilderDynFieldsCloneRace(t *testing.T) {
resetPackageVars()

b := NewBuilder()

const interations = 100

var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
for i := 0; i < interations; i++ {
b.Clone()
}
}()
wg.Add(1)
go func() {
defer wg.Done()
for i := 0; i < interations; i++ {
b.AddDynamicField("dyn_field", nil)
}
}()

wg.Wait()
}

func TestOutputInterface(t *testing.T) {
resetPackageVars()
testTx := &MockOutput{}
Expand Down

0 comments on commit 0b5ed0c

Please sign in to comment.