From c3c461343dca523d7b139564ea8c49ba400fbc12 Mon Sep 17 00:00:00 2001
From: nathanf <nathanf@squareup.com>
Date: Thu, 30 Jun 2016 18:27:00 -0700
Subject: [PATCH 1/2] initial implementation for fast fail

---
 function/make.go                   | 40 +++++++++++++++++++++---------
 query/tests/command_select_test.go | 26 +++++++++++++++++++
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/function/make.go b/function/make.go
index 91a06d5..dc2b8e5 100644
--- a/function/make.go
+++ b/function/make.go
@@ -21,6 +21,8 @@ import (
 	"time"
 
 	"github.com/square/metrics/api"
+
+	netcontext "golang.org/x/net/context"
 )
 
 // MakeFunction is a convenient way to use type-safe functions to
@@ -94,6 +96,11 @@ func MakeFunction(name string, function interface{}) MetricFunction {
 				return arg
 			}
 
+			subCtx, cancel := netcontext.WithCancel(context.Ctx())
+			defer cancel() // when done, make sure to terminate the subcontext
+			subcontext := context
+			subcontext.private.Ctx = subCtx
+
 			// evalTo takes an expression and a reflect.Type and evaluates to the appropriate type.
 			// If an Expression is requested, it just returns it.
 			evalTo := func(expression Expression, resultType reflect.Type) (interface{}, error) {
@@ -101,17 +108,17 @@ func MakeFunction(name string, function interface{}) MetricFunction {
 				case expressionType:
 					return expression, nil
 				case stringType:
-					return EvaluateToString(expression, context)
+					return EvaluateToString(expression, subcontext)
 				case scalarType:
-					return EvaluateToScalar(expression, context)
+					return EvaluateToScalar(expression, subcontext)
 				case scalarSetType:
-					return EvaluateToScalarSet(expression, context)
+					return EvaluateToScalarSet(expression, subcontext)
 				case durationType:
-					return EvaluateToDuration(expression, context)
+					return EvaluateToDuration(expression, subcontext)
 				case timeseriesType:
-					return EvaluateToSeriesList(expression, context)
+					return EvaluateToSeriesList(expression, subcontext)
 				case valueType:
-					return expression.Evaluate(context)
+					return expression.Evaluate(subcontext)
 				}
 				panic(fmt.Sprintf("Unreachable :: Attempting to evaluate to unknown type %+v", resultType))
 			}
@@ -142,9 +149,9 @@ func MakeFunction(name string, function interface{}) MetricFunction {
 				argType := funcType.In(i)
 				switch argType {
 				case contextType:
-					argumentFuncs[i] = provideValue(context)
+					argumentFuncs[i] = provideValue(subcontext)
 				case timerangeType:
-					argumentFuncs[i] = provideValue(context.Timerange())
+					argumentFuncs[i] = provideValue(subcontext.Timerange())
 				case groupsType:
 					argumentFuncs[i] = provideValue(groups)
 				case stringType, scalarType, scalarSetType, durationType, timeseriesType, valueType, expressionType:
@@ -188,10 +195,19 @@ func MakeFunction(name string, function interface{}) MetricFunction {
 					argValues[i] = reflect.ValueOf(arg)
 				}()
 			}
-			waiter.Wait() // Wait for all the arguments to be evaluated.
-
-			if len(errors) != 0 {
-				return nil, <-errors
+			waitChan := make(chan struct{})
+			go func() {
+				waiter.Wait() // Wait for all the arguments to be evaluated.
+				close(waitChan)
+			}()
+
+			select {
+			case <-waitChan:
+				// Everything completed okay and without error.
+			case err := <-errors:
+				// An error occurred, so we return it immediately.
+				// The deferred cancel will run as the function returns.
+				return nil, err
 			}
 
 			output := funcValue.Call(argValues)
diff --git a/query/tests/command_select_test.go b/query/tests/command_select_test.go
index a60bfef..d858637 100644
--- a/query/tests/command_select_test.go
+++ b/query/tests/command_select_test.go
@@ -520,3 +520,29 @@ func TestTag(t *testing.T) {
 		}
 	}
 }
+
+func TestFastError(t *testing.T) {
+	testTimerange, err := api.NewTimerange(0, 1000, 10)
+	if err != nil {
+		t.Fatalf("unexpected error creating test timerange: %s", err.Error())
+	}
+	comboAPI := mocks.NewComboAPI(testTimerange)
+	cmd, err := parser.Parse("select blah + series_timeout from -30m to now")
+	if err != nil {
+		t.Fatalf("unexpected error parsing test query: %s", err.Error())
+	}
+	now := time.Now()
+	garbage, err := cmd.Execute(command.ExecutionContext{
+		TimeseriesStorageAPI: comboAPI,
+		MetricMetadataAPI:    comboAPI,
+		FetchLimit:           100,
+		Ctx:                  context.Background(),
+	})
+	if err == nil {
+		t.Errorf("expected error from test query but got out %+v", garbage)
+	}
+	if time.Since(now) > time.Millisecond*10 {
+		t.Errorf("expected instantaneous failure, but took %+v time", time.Since(now))
+	}
+
+}

From 23dffa59d6c60a18f5f6e2968954efbde05f968a Mon Sep 17 00:00:00 2001
From: nathanf <nathanf@squareup.com>
Date: Fri, 1 Jul 2016 11:35:31 -0700
Subject: [PATCH 2/2] fix race condition

---
 function/make.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/function/make.go b/function/make.go
index dc2b8e5..2bbf97c 100644
--- a/function/make.go
+++ b/function/make.go
@@ -186,13 +186,16 @@ func MakeFunction(name string, function interface{}) MetricFunction {
 				i := i
 				waiter.Add(1)
 				go func() {
-					defer waiter.Done()
 					arg, err := argumentFuncs[i]()
 					if err != nil {
+						// Do NOT .Done() the waiter before sending an error: doing so will
+						// introduce a race between the waiter finishing and the error being
+						// received.
 						errors <- err
 						return
 					}
 					argValues[i] = reflect.ValueOf(arg)
+					waiter.Done()
 				}()
 			}
 			waitChan := make(chan struct{})