Remove race from query finalization.

service/datastore/query.go

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package datastore
 
 import (
         "bytes"
         "fmt"
         "sort"
         "strings"
+        "sync"
 
         "github.com/luci/luci-go/common/errors"
         "github.com/luci/luci-go/common/stringset"
 )
 
 var (
         // ErrMultipleInequalityFilter is returned from Query.Finalize if you build a
         // query which has inequality filters on multiple fields.
         ErrMultipleInequalityFilter = errors.New(
                 "inequality filters on multiple properties in the same Query is not allowed")
 
         // ErrNullQuery is returned from Query.Finalize if you build a query for which
         // there cannot possibly be any results.
         ErrNullQuery = errors.New(
                 "the query is overconstrained and can never have results")
 )
 
 // Query is a builder-object for building a datastore query. It may represent
 // an invalid query, but the error will only be observable when you call
 // Finalize.
+//
+// A Query is, for the most part, not goroutine-safe. However, it is
 type Query struct {
+        queryFields
+
+        // These are set by Finalize as a way to cache the 1-1 correspondence of
+        // a Query to its FinalizedQuery form. err may also be set by intermediate
+        // Query functions if there's a problem before finalization.
+        //
+        // Query implements lazy finalization, meaning that it will happen at most
+        // once. This means that the finalization state and cached finalization must
+        // be locked around.
+        finalizeOnce sync.Once
+        finalized    *FinalizedQuery
+        finalizeErr  error
+}
+
+// queryFields are the Query's read-only fields.
+type queryFields struct {

[dnj, 2016/04/21 22:49:16]

Why!?

Because we clone the query by copying it, meaning we inherit the (racy) error
and once state.

By splitting the RO fields from the Query-unique fields, we can copy the inner
struct (see mod) easily and avoid this situation.

         kind string
 
         eventualConsistency bool
         keysOnly            bool
         distinct            bool
 
         limit  *int32
         offset *int32
 
         order   []IndexColumn
         project stringset.Set
 
         eqFilts map[string]PropertySlice
 
         ineqFiltProp     string
         ineqFiltLow      Property
         ineqFiltLowIncl  bool
         ineqFiltLowSet   bool
         ineqFiltHigh     Property
         ineqFiltHighIncl bool
         ineqFiltHighSet  bool
 
         start Cursor
         end   Cursor
 
-        // These are set by Finalize as a way to cache the 1-1 correspondence of
-        // a Query to its FinalizedQuery form. err may also be set by intermediate
-        // Query functions if there's a problem before finalization.
-        finalized *FinalizedQuery
-        err       error
+        err error
 }
 
 // NewQuery returns a new Query for the given kind. If kind may be empty to
 // begin a kindless query.
 func NewQuery(kind string) *Query {
-        return &Query{kind: kind}
+        return &Query{
+                queryFields: queryFields{
+                        kind: kind,
+                },
+        }
 }
 
 func (q *Query) mod(cb func(*Query)) *Query {
         if q.err != nil {
                 return q
         }
 
-        ret := *q
-        ret.finalized = nil
+        ret := Query{
+                queryFields: q.queryFields,
+        }
         if len(q.order) > 0 {
                 ret.order = make([]IndexColumn, len(q.order))
                 copy(ret.order, q.order)
         }
         if q.project != nil {
                 ret.project = q.project.Dup()
         }
         if len(q.eqFilts) > 0 {
                 ret.eqFilts = make(map[string]PropertySlice, len(q.eqFilts))
                 for k, v := range q.eqFilts {
@@ skipping 373 matching lines @@
                 }
                 q.ineqFiltLowSet = false
                 q.ineqFiltHighSet = false
         })
 }
 
 // Finalize converts this Query to a FinalizedQuery. If the Query has any
 // inconsistencies or violates any of the query rules, that will be returned
 // here.
 func (q *Query) Finalize() (*FinalizedQuery, error) {
-        if q.err != nil || q.finalized != nil {
-                return q.finalized, q.err
+        if q.err != nil {
+                return nil, q.err
         }
 
+        q.finalizeOnce.Do(func() {
+                q.finalized, q.finalizeErr = q.finalizeImpl()
+        })
+        return q.finalized, q.finalizeErr
+}
+
+func (q *Query) finalizeImpl() (*FinalizedQuery, error) {
         ancestor := (*Key)(nil)
         if slice, ok := q.eqFilts["__ancestor__"]; ok {
                 ancestor = slice[0].Value().(*Key)
         }
 
         err := func() error {
 
                 if q.kind == "" { // kindless query checks
                         if q.ineqFiltProp != "" && q.ineqFiltProp != "__key__" {
                                 return fmt.Errorf(
@@ skipping 52 matching lines @@
                                 if _, iseq := q.eqFilts[p]; iseq {
                                         err = fmt.Errorf("cannot project on equality filter field: %s", p)
                                         return false
                                 }
                                 return true
                         })
                 }
                 return err
         }()
         if err != nil {
-                q.err = err
                 return nil, err
         }
 
         ret := &FinalizedQuery{
                 original: q,
                 kind:     q.kind,
 
                 keysOnly:             q.keysOnly,
                 eventuallyConsistent: q.eventualConsistency || ancestor == nil,
                 limit:                q.limit,
@@ skipping 79 matching lines @@
                 }
         }
 
         // If the suffix format ends with __key__ already (e.g. .Order("__key__")),
         // then we're good to go. Otherwise we need to add it as the last bit of the
         // suffix, since all indexes implicitly have it as the last column.
         if len(ret.orders) == 0 || ret.orders[len(ret.orders)-1].Property != "__key__" {
                 ret.orders = append(ret.orders, IndexColumn{Property: "__key__"})
         }
 
-        q.finalized = ret
         return ret, nil
 }
 
 func (q *Query) String() string {
         ret := &bytes.Buffer{}
         needComma := false
         p := func(fmtStr string, stuff ...interface{}) {
                 if needComma {
                         if _, err := ret.WriteString(", "); err != nil {
                                 panic(err)
@@ skipping 80 matching lines @@
         if q.keysOnly {
                 p("KeysOnly")
         }
 
         if _, err := ret.WriteRune(')'); err != nil {
                 panic(err)
         }
 
         return ret.String()
 }