Make breaker tests deterministic again. (knative#4784)

It's very important in these tests to assert failing requests to make sure the breaker's state is consistent. The change caused the "to be canceled" request to race the "supposed to be successful" request before it to the semaphore. If the "to be canceled" request won, it'd get capacity immediately and the cancel is defunct. The "supposed to be successful" request before it gets put into the queue and the `expectFailure` call hangs forever.
casualuser · Jul 17, 2019 · bc20336 · bc20336
1 parent 0e250d1
commit bc20336
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 13 deletions.
diff --git a/pkg/queue/breaker.go b/pkg/queue/breaker.go
@@ -86,12 +86,10 @@ func (b *Breaker) Maybe(ctx context.Context, thunk func()) bool {
 			return false
 		}
 		// Defer releasing capacity in the active.
-		defer func() {
-			// It's safe to ignore the error returned by release since we
-			// make sure the semaphore is only manipulated here and acquire
-			// + release calls are equally paired.
-			b.sem.release()
-		}()
+		// It's safe to ignore the error returned by release since we
+		// make sure the semaphore is only manipulated here and acquire
+		// + release calls are equally paired.
+		defer b.sem.release()
 
 		// Do the thing.
 		thunk()

diff --git a/pkg/queue/breaker_test.go b/pkg/queue/breaker_test.go
@@ -131,21 +131,27 @@ func TestBreakerCancel(t *testing.T) {
 	cancel1()
 	reqs.expectFailure(t)
 
-	// Let through a request with capacity.
-	b.UpdateConcurrency(1)
-	reqs.request()
-
-	// This request fails due to canceled.
+	// This request cannot get capacity either. This reproduced a bug we had when
+	// freeing slots on the pendingRequests channel.
 	ctx2, cancel2 := context.WithCancel(context.Background())
 	reqs.requestWithContext(ctx2)
 	cancel2()
 	reqs.expectFailure(t)
 
-	// This request is cached
+	// Let through a request with capacity then timeout following request
+	b.UpdateConcurrency(1)
 	reqs.request()
 
-	// This request fails due to overflows.
+	// Exceed capacity and assert one failure. This makes sure the Breaker is consistently
+	// at capacity.
 	reqs.request()
+	reqs.request()
+	reqs.expectFailure(t)
+
+	// This request cannot get capacity.
+	ctx3, cancel3 := context.WithCancel(context.Background())
+	reqs.requestWithContext(ctx3)
+	cancel3()
 	reqs.expectFailure(t)
 
 	// The requests that were put in earlier should succeed.