Merge pull request #19274 from krousey/apiserver_flake

pkg/apiserver/handlers_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package apiserver
 
 import (
+	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -41,15 +42,15 @@ func (fakeRL) Stop()             {}
 func (f fakeRL) TryAccept() bool { return bool(f) }
 func (f fakeRL) Accept()         {}
 
-func expectHTTP(url string, code int, t *testing.T) {
+func expectHTTP(url string, code int) error {
 	r, err := http.Get(url)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-		return
+		return fmt.Errorf("unexpected error: %v", err)
 	}
 	if r.StatusCode != code {
-		t.Errorf("unexpected response: %v", r.StatusCode)
+		return fmt.Errorf("unexpected response: %v", r.StatusCode)
 	}
+	return nil
 }
 
 func getPath(resource, namespace, name string) string {
@@ -82,6 +83,13 @@ func TestMaxInFlight(t *testing.T) {
 	// 'short' accounted ones.
 	calls := &sync.WaitGroup{}
 	calls.Add(AllowedInflightRequestsNo * 2)
+
+	// Responses is used to wait until all responses are
+	// received. This prevents some async requests getting EOF
+	// errors from prematurely closing the server
+	responses := sync.WaitGroup{}
+	responses.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{}
@@ -109,21 +117,25 @@ func TestMaxInFlight(t *testing.T) {
 	for i := 0; i < AllowedInflightRequestsNo; i++ {
 		// These should hang waiting on block...
 		go func() {
-			expectHTTP(server.URL+"/foo/bar/watch", http.StatusOK, t)
+			if err := expectHTTP(server.URL+"/foo/bar/watch", http.StatusOK); err != nil {
+				t.Error(err)
+			}
+			responses.Done()
 		}()
 	}
 	// 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
+	if err := expectHTTP(server.URL+"/dontwait", http.StatusOK); err != nil {
+		t.Error(err)
+	}
 
 	// 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)
+			if err := expectHTTP(server.URL, http.StatusOK); err != nil {
+				t.Error(err)
+			}
+			responses.Done()
 		}()
 	}
 	// We wait for all calls to be received by the server
@@ -133,18 +145,24 @@ func TestMaxInFlight(t *testing.T) {
 
 	// 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)
+		if err := expectHTTP(server.URL, errors.StatusTooManyRequests); err != nil {
+			t.Error(err)
+		}
 	}
 	// Validate that non-accounted URLs still work
-	expectHTTP(server.URL+"/dontwait/watch", http.StatusOK, t)
+	if err := expectHTTP(server.URL+"/dontwait/watch", http.StatusOK); err != nil {
+		t.Error(err)
+	}
 
 	// Let all hanging requests finish
 	block.Done()
 
 	// Show that we recover from being blocked up.
 	// Too avoid flakyness we need to wait until at least one of the requests really finishes.
-	oneAccountedFinished.Wait()
-	expectHTTP(server.URL, http.StatusOK, t)
+	responses.Wait()
+	if err := expectHTTP(server.URL, http.StatusOK); err != nil {
+		t.Error(err)
+	}
 }
 
 func TestReadOnly(t *testing.T) {