From a5300a0c63ddbb9cb6891aec9e86d8b1bf47f3ef Mon Sep 17 00:00:00 2001
From: gmarek <gmarek@google.com>
Date: Mon, 23 Nov 2015 19:00:21 +0100
Subject: [PATCH] Add more comments to TestMaxInflight

---
 pkg/apiserver/handlers_test.go | 82 +++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 32 deletions(-)

diff --git a/pkg/apiserver/handlers_test.go b/pkg/apiserver/handlers_test.go
index cabac4b8ea3..e9c97e1af13 100644
--- a/pkg/apiserver/handlers_test.go
+++ b/pkg/apiserver/handlers_test.go
@@ -57,71 +57,89 @@ func pathWithPrefix(prefix, resource, namespace, name string) string {
 	return testapi.Default.ResourcePathWithPrefix(prefix, resource, namespace, name)
 }
 
+// Tests that MaxInFlightLimit works, i.e.
+// - "long" requests such as proxy or watch, identified by regexp are not accounted despite
+//   hanging for the long time,
+// - "short" requests are correctly accounted, i.e. there can be only size of channel passed to the
+//   constructor in flight at any given moment,
+// - subsequent "short" requests are rejected instantly with apropriate error,
+// - subsequent "long" requests are handled normally,
+// - we correctly recover after some "short" requests finish, i.e. we can process new ones.
 func TestMaxInFlight(t *testing.T) {
-	const Iterations = 3
+	const AllowedInflightRequestsNo = 3
+	// Size of inflightRequestsChannel determines how many concurent inflight requests
+	// are allowed.
+	inflightRequestsChannel := make(chan bool, AllowedInflightRequestsNo)
+	// notAccountedPathsRegexp specifies paths requests to which we don't account into
+	// requests in flight.
+	notAccountedPathsRegexp := regexp.MustCompile(".*\\/watch")
+
+	// Calls is used to wait until all server calls are received. We are sending
+	// AllowedInflightRequestsNo of 'long' not-accounted requests and the same number of
+	// 'short' accounted ones.
+	calls := &sync.WaitGroup{}
+	calls.Add(AllowedInflightRequestsNo * 2)
+	// Block is used to keep requests in flight for as long as we need to. All requests will
+	// be unblocked at the same time.
 	block := sync.WaitGroup{}
 	block.Add(1)
-	oneAccountedFinished := sync.WaitGroup{}
-	oneAccountedFinished.Add(1)
-	var once sync.Once
-	sem := make(chan bool, Iterations)
 
-	re := regexp.MustCompile("[.*\\/watch][^\\/proxy.*]")
-
-	// Calls verifies that the server is actually blocked up before running the rest of the test
-	calls := &sync.WaitGroup{}
-	calls.Add(Iterations * 3)
-
-	server := httptest.NewServer(MaxInFlightLimit(sem, re, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if strings.Contains(r.URL.Path, "dontwait") {
-			return
-		}
-		if calls != nil {
-			calls.Done()
-		}
-		block.Wait()
-	})))
+	server := httptest.NewServer(
+		MaxInFlightLimit(
+			inflightRequestsChannel,
+			notAccountedPathsRegexp,
+			http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				// A short, accounted request that does not wait for block WaitGroup.
+				if strings.Contains(r.URL.Path, "dontwait") {
+					return
+				}
+				if calls != nil {
+					calls.Done()
+				}
+				block.Wait()
+			}),
+		),
+	)
 	defer server.Close()
 
 	// These should hang, but not affect accounting.
-	for i := 0; i < Iterations; i++ {
+	for i := 0; i < AllowedInflightRequestsNo; i++ {
 		// These should hang waiting on block...
 		go func() {
 			expectHTTP(server.URL+"/foo/bar/watch", http.StatusOK, t)
 		}()
 	}
-
-	// These should hang, but not affect accounting.
-	for i := 0; i < Iterations; i++ {
-		// These should hang waiting on block...
-		go func() {
-			expectHTTP(server.URL+"/proxy/foo/bar", http.StatusOK, t)
-		}()
-	}
+	// Check that sever is not saturated by not-accounted calls
 	expectHTTP(server.URL+"/dontwait", http.StatusOK, t)
+	oneAccountedFinished := sync.WaitGroup{}
+	oneAccountedFinished.Add(1)
+	var once sync.Once
 
-	for i := 0; i < Iterations; i++ {
+	// These should hang and be accounted, i.e. saturate the server
+	for i := 0; i < AllowedInflightRequestsNo; i++ {
 		// These should hang waiting on block...
 		go func() {
 			expectHTTP(server.URL, http.StatusOK, t)
 			once.Do(oneAccountedFinished.Done)
 		}()
 	}
+	// We wait for all calls to be received by the server
 	calls.Wait()
+	// Disable calls notifications in the server
 	calls = nil
 
 	// Do this multiple times to show that it rate limit rejected requests don't block.
 	for i := 0; i < 2; i++ {
 		expectHTTP(server.URL, errors.StatusTooManyRequests, t)
 	}
-
 	// Validate that non-accounted URLs still work
 	expectHTTP(server.URL+"/dontwait/watch", http.StatusOK, t)
 
+	// Let all hanging requests finish
 	block.Done()
 
 	// Show that we recover from being blocked up.
-	// However, we should until at least one of the requests really finishes.
+	// Too avoid flakyness we need to wait until at least one of the requests really finishes.
 	oneAccountedFinished.Wait()
 	expectHTTP(server.URL, http.StatusOK, t)
 }