Skip to content

Commit

Permalink
correct race condition from final optimization
Browse files Browse the repository at this point in the history
Signed-off-by: Tim Bray <[email protected]>
  • Loading branch information
timbray committed Jun 9, 2024
1 parent 8eb41db commit 7454b1e
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 22 deletions.
27 changes: 17 additions & 10 deletions core_matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,20 +178,27 @@ func (m *coreMatcher) matchesForFields(fields []Field) ([]X, error) {
}
matches := newMatchSet()

// pre-allocate a pair of buffers that will be used several levels down the call stack for efficiently
// transversing NFAs
bufs := &bufpair{
buf1: make([]*faState, 0, 3072),
buf2: make([]*faState, 0, 3072),
}

// for each of the fields, we'll try to match the automaton start state to that field - the tryToMatch
// routine will, in the case that there's a match, call itself to see if subsequent fields after the
// first matched will transition through the machine and eventually achieve a match
s := m.fields()
for i := 0; i < len(fields); i++ {
tryToMatch(fields, i, s.state, matches)
tryToMatch(fields, i, s.state, matches, bufs)
}
return matches.matches(), nil
}

// tryToMatch tries to match the field at fields[index] to the provided state. If it does match and generate
// 1 or more transitions to other states, it calls itself recursively to see if any of the remaining fields
// can continue the process by matching that state.
func tryToMatch(fields []Field, index int, state *fieldMatcher, matches *matchSet) {
func tryToMatch(fields []Field, index int, state *fieldMatcher, matches *matchSet, bufs *bufpair) {
stateFields := state.fields()

// transition on exists:true?
Expand All @@ -200,16 +207,16 @@ func tryToMatch(fields []Field, index int, state *fieldMatcher, matches *matchSe
matches = matches.addXSingleThreaded(existsTrans.fields().matches...)
for nextIndex := index + 1; nextIndex < len(fields); nextIndex++ {
if noArrayTrailConflict(fields[index].ArrayTrail, fields[nextIndex].ArrayTrail) {
tryToMatch(fields, nextIndex, existsTrans, matches)
tryToMatch(fields, nextIndex, existsTrans, matches, bufs)
}
}
}

// an exists:false transition is possible if there is no matching field in the event
checkExistsFalse(stateFields, fields, index, matches)
checkExistsFalse(stateFields, fields, index, matches, bufs)

// try to transition through the machine
nextStates := state.transitionOn(&fields[index])
nextStates := state.transitionOn(&fields[index], bufs)

// for each state in the possibly-empty list of transitions from this state on fields[index]
for _, nextState := range nextStates {
Expand All @@ -221,17 +228,17 @@ func tryToMatch(fields []Field, index int, state *fieldMatcher, matches *matchSe
// of the same array
for nextIndex := index + 1; nextIndex < len(fields); nextIndex++ {
if noArrayTrailConflict(fields[index].ArrayTrail, fields[nextIndex].ArrayTrail) {
tryToMatch(fields, nextIndex, nextState, matches)
tryToMatch(fields, nextIndex, nextState, matches, bufs)
}
}
// now we've run out of fields to match this state against. But suppose it has an exists:false
// transition, and it so happens that the exists:false pattern field is lexically larger than the other
// fields and that in fact such a field does not exist. That state would be left hanging. So…
checkExistsFalse(nextStateFields, fields, index, matches)
checkExistsFalse(nextStateFields, fields, index, matches, bufs)
}
}

func checkExistsFalse(stateFields *fmFields, fields []Field, index int, matches *matchSet) {
func checkExistsFalse(stateFields *fmFields, fields []Field, index int, matches *matchSet, bufs *bufpair) {
for existsFalsePath, existsFalseTrans := range stateFields.existsFalse {
// it seems like there ought to be a more state-machine-idiomatic way to do this, but
// I thought of a few and none of them worked. Quite likely someone will figure it out eventually.
Expand All @@ -250,9 +257,9 @@ func checkExistsFalse(stateFields *fmFields, fields []Field, index int, matches
if i == len(fields) {
matches = matches.addXSingleThreaded(existsFalseTrans.fields().matches...)
if thisFieldIsAnExistsFalse {
tryToMatch(fields, index+1, existsFalseTrans, matches)
tryToMatch(fields, index+1, existsFalseTrans, matches, bufs)
} else {
tryToMatch(fields, index, existsFalseTrans, matches)
tryToMatch(fields, index, existsFalseTrans, matches, bufs)
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions field_matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,12 @@ func (m *fieldMatcher) addTransition(field *patternField, printer printer) []*fi
// or nil if no transitions are possible. An example of name/value that could produce multiple next states
// would be if you had the pattern { "a": [ "foo" ] } and another pattern that matched any value with
// a prefix of "f".
func (m *fieldMatcher) transitionOn(field *Field) []*fieldMatcher {
func (m *fieldMatcher) transitionOn(field *Field, bufs *bufpair) []*fieldMatcher {
// are there transitions on this field name?
valMatcher, ok := m.fields().transitions[string(field.Path)]
if !ok {
return nil
}

return valMatcher.transitionOn(field.Val)
return valMatcher.transitionOn(field.Val, bufs)
}
5 changes: 2 additions & 3 deletions value_matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ type vmFields struct {
startTable *smallTable
singletonMatch []byte
singletonTransition *fieldMatcher
buffers bufpair
}

func (m *valueMatcher) fields() *vmFields {
Expand All @@ -52,7 +51,7 @@ func newValueMatcher() *valueMatcher {
return &vm
}

func (m *valueMatcher) transitionOn(val []byte) []*fieldMatcher {
func (m *valueMatcher) transitionOn(val []byte, bufs *bufpair) []*fieldMatcher {
var transitions []*fieldMatcher

fields := m.fields()
Expand All @@ -69,7 +68,7 @@ func (m *valueMatcher) transitionOn(val []byte) []*fieldMatcher {
return transitions

case fields.startTable != nil:
return traverseFA(fields.startTable, val, transitions, &fields.buffers)
return traverseFA(fields.startTable, val, transitions, bufs)

default:
// no FA, no singleton, nothing to do, this probably can't happen because a flattener
Expand Down
14 changes: 7 additions & 7 deletions value_matcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func addInvalid(t *testing.T, before []typedVal) {

func TestNoOpTransition(t *testing.T) {
vm := newValueMatcher()
tr := vm.transitionOn([]byte("foo"))
tr := vm.transitionOn([]byte("foo"), &bufpair{})
if len(tr) != 0 {
t.Error("matched on empty valuematcher")
}
Expand All @@ -57,7 +57,7 @@ func TestAddTransition(t *testing.T) {
if t1 == nil {
t.Error("nil addTrans")
}
t1x := m.transitionOn([]byte("one"))
t1x := m.transitionOn([]byte("one"), &bufpair{})
if len(t1x) != 1 || t1x[0] != t1 {
t.Error("Retrieve failed")
}
Expand All @@ -73,11 +73,11 @@ func TestAddTransition(t *testing.T) {
}
t2 := m.addTransition(v2, &nullPrinter{})

t2x := m.transitionOn([]byte("two"))
t2x := m.transitionOn([]byte("two"), &bufpair{})
if len(t2x) != 1 || t2x[0] != t2 {
t.Error("trans failed T2")
}
t1x = m.transitionOn([]byte("one"))
t1x = m.transitionOn([]byte("one"), &bufpair{})
if len(t1x) != 1 || t1x[0] != t1 {
t.Error("Retrieve failed")
}
Expand All @@ -86,15 +86,15 @@ func TestAddTransition(t *testing.T) {
val: "three",
}
t3 := m.addTransition(v3, &nullPrinter{})
t3x := m.transitionOn([]byte("three"))
t3x := m.transitionOn([]byte("three"), &bufpair{})
if len(t3x) != 1 || t3x[0] != t3 {
t.Error("Match failed T3")
}
t2x = m.transitionOn([]byte("two"))
t2x = m.transitionOn([]byte("two"), &bufpair{})
if len(t2x) != 1 || t2x[0] != t2 {
t.Error("trans failed T2")
}
t1x = m.transitionOn([]byte("one"))
t1x = m.transitionOn([]byte("one"), &bufpair{})
if len(t1x) != 1 || t1x[0] != t1 {
t.Error("Retrieve failed")
}
Expand Down

0 comments on commit 7454b1e

Please sign in to comment.