Merge branch 'x-accel-buffering' into 'master'

Disable NGINX response buffering when not proxying

Closes https://gitlab.com/gitlab-org/gitlab-workhorse/issues/78

See merge request !87

internal/helper/helpers.go

@@ -10,6 +10,8 @@ import (
 	"syscall"
 )
 
+const NginxResponseBufferHeader = "X-Accel-Buffering"
+
 func Fail500(w http.ResponseWriter, r *http.Request, err error) {
 	http.Error(w, "Internal server error", 500)
 	captureRavenError(r, err)
@@ -132,3 +134,11 @@ func ExitStatus(err error) (int, bool) {
 
 	return waitStatus.ExitStatus(), true
 }
+
+func DisableResponseBuffering(w http.ResponseWriter) {
+	w.Header().Set(NginxResponseBufferHeader, "no")
+}
+
+func AllowResponseBuffering(w http.ResponseWriter) {
+	w.Header().Del(NginxResponseBufferHeader)
+}

internal/proxy/proxy.go

@@ -34,5 +34,7 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	req.Header.Set("Gitlab-Workhorse", p.Version)
 	req.Header.Set("Gitlab-Workhorse-Proxy-Start", fmt.Sprintf("%d", time.Now().UnixNano()))
 
+	helper.AllowResponseBuffering(w)
+
 	p.reverseProxy.ServeHTTP(w, &req)
 }

internal/senddata/senddata.go

@@ -2,6 +2,8 @@ package senddata
 
 import (
 	"net/http"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
 type sendDataResponseWriter struct {
@@ -62,6 +64,7 @@ func (s *sendDataResponseWriter) tryInject() bool {
 	for _, injecter := range s.injecters {
 		if injecter.Match(header) {
 			s.hijacked = true
+			helper.DisableResponseBuffering(s.rw)
 			injecter.Inject(s.rw, s.req, header)
 			return true
 		}

internal/sendfile/sendfile.go

@@ -66,6 +66,7 @@ func (s *sendFileResponseWriter) WriteHeader(status int) {
 		s.hijacked = true
 
 		// Serve the file
+		helper.DisableResponseBuffering(s.rw)
 		sendFileFromDisk(s.rw, s.req, file)
 		return
 	}

internal/upstream/upstream.go

@@ -65,6 +65,8 @@ func (u *Upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
 	w := helper.NewLoggingResponseWriter(ow)
 	defer w.Log(r)
 
+	helper.DisableResponseBuffering(&w)
+
 	// Drop WebSocket connection and CONNECT method
 	if r.RequestURI == "*" {
 		helper.HTTPError(&w, r, "Connection upgrade not allowed", http.StatusBadRequest)

main_test.go

@@ -362,6 +362,9 @@ func TestRegularProjectsAPI(t *testing.T) {
 		if resp.StatusCode != 200 {
 			t.Errorf("GET %q: expected 200, got %d", resource, resp.StatusCode)
 		}
+		if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "" {
+			t.Errorf("GET %q: Expected %s not to be present, got %q", resource, helper.NginxResponseBufferHeader, h)
+		}
 	}
 }
 
@@ -416,6 +419,9 @@ func TestAllowedStaticFile(t *testing.T) {
 		if proxied {
 			t.Errorf("GET %q: should not have made it to backend", resource)
 		}
+		if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "no" {
+			t.Errorf("GET %q: Expected %s to equal %q, got %q", resource, helper.NginxResponseBufferHeader, "no", h)
+		}
 	}
 }
 
@@ -488,6 +494,9 @@ func TestAllowedPublicUploadsFile(t *testing.T) {
 		if !proxied {
 			t.Fatalf("GET %q: never made it to backend", resource)
 		}
+		if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "no" {
+			t.Errorf("GET %q: Expected %s to equal %q, got %q", resource, helper.NginxResponseBufferHeader, "no", h)
+		}
 	}
 }
 
@@ -642,6 +651,10 @@ func TestArtifactsGetSingleFile(t *testing.T) {
 	if string(body) != fileContents {
 		t.Fatalf("Expected file contents %q, got %q", fileContents, body)
 	}
+
+	if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "no" {
+		t.Errorf("GET %q: Expected %s to equal %q, got %q", resourcePath, helper.NginxResponseBufferHeader, "no", h)
+	}
 }
 
 func TestGetGitBlob(t *testing.T) {
@@ -669,6 +682,10 @@ func TestGetGitBlob(t *testing.T) {
 	if !strings.HasPrefix(string(body), "The MIT License (MIT)") {
 		t.Fatalf("Expected MIT license, got %q", body)
 	}
+
+	if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "no" {
+		t.Errorf("Expected %s to equal %q, got %q", helper.NginxResponseBufferHeader, "no", h)
+	}
 }
 
 func TestGetGitDiff(t *testing.T) {
@@ -693,6 +710,10 @@ func TestGetGitDiff(t *testing.T) {
 	if bodyLengthBytes != 155 {
 		t.Fatal("Expected the body to consist of 155 bytes, got %v", bodyLengthBytes)
 	}
+
+	if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "no" {
+		t.Errorf("Expected %s to equal %q, got %q", helper.NginxResponseBufferHeader, "no", h)
+	}
 }
 
 func TestGetGitPatch(t *testing.T) {
@@ -712,6 +733,10 @@ func TestGetGitPatch(t *testing.T) {
 
 	// Only the two commits on the fix branch should be included
 	testhelper.AssertPatchSeries(t, body, "12d65c8dd2b2676fa3ac47d955accc085a37a9c1", toSha)
+
+	if h := resp.Header.Get(helper.NginxResponseBufferHeader); h != "no" {
+		t.Errorf("Expected %s to equal %q, got %q", helper.NginxResponseBufferHeader, "no", h)
+	}
 }
 
 func TestApiContentTypeBlock(t *testing.T) {