Revert "Delay HTTP headers for Git HTTP responses"

This reverts commit 1fa69c8d. It
turned out that the new behavior broke 'shallow git clone'.

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

internal/delay/responsewriter.go

-// Package delay exports delay.ResponseWriter. This type implements
-// http.ResponseWriter with the ability to delay setting the HTTP
-// response code (with WriteHeader()) until writing the first bufferSize
-// bytes. This makes it possible, up to a point, to 'change your mind'
-// about the HTTP status code. The caller must call
-// ResponseWriter.Flush() before returning from the handler (e.g. using
-// 'defer').
-
-package delay
-
-import (
-	"bytes"
-	"io"
-	"net/http"
-)
-
-const bufferSize = 8192
-
-type ResponseWriter struct {
-	writer     http.ResponseWriter
-	status     int
-	bufWritten int
-	cap        int
-	flushed    bool
-	buffer     *bytes.Buffer
-}
-
-func NewResponseWriter(w http.ResponseWriter) *ResponseWriter {
-	return &ResponseWriter{
-		writer: w,
-		buffer: bytes.NewBuffer(make([]byte, 0, bufferSize)),
-		cap:    bufferSize,
-	}
-}
-
-func (rw *ResponseWriter) Write(buf []byte) (int, error) {
-	if !rw.flushed && len(buf)+rw.bufWritten <= rw.cap {
-		n, err := rw.buffer.Write(buf)
-		rw.bufWritten += n
-		return n, err
-	}
-
-	if err := rw.Flush(); err != nil {
-		return 0, err
-	}
-
-	return rw.writer.Write(buf)
-}
-
-func (rw *ResponseWriter) Header() http.Header {
-	return rw.writer.Header()
-}
-
-func (rw *ResponseWriter) WriteHeader(code int) {
-	if rw.status != 0 {
-		return
-	}
-	rw.status = code
-}
-
-func (rw *ResponseWriter) Flush() error {
-	if rw.flushed {
-		return nil
-	}
-	rw.flushed = true
-
-	if rw.status == 0 {
-		rw.writer.WriteHeader(http.StatusOK)
-	} else {
-		rw.writer.WriteHeader(rw.status)
-	}
-
-	_, err := io.Copy(rw.writer, rw.buffer)
-	rw.buffer = nil // "Release" the buffer for GC
-	return err
-}

internal/delay/responsewriter_test.go

-package delay
-
-import (
-	"fmt"
-	"net/http/httptest"
-	"strings"
-	"testing"
-)
-
-func TestSanity(t *testing.T) {
-	first, second := 200, 500
-	w := httptest.NewRecorder()
-	w.WriteHeader(first)
-	w.WriteHeader(second)
-	if code := w.Code; code != first {
-		t.Fatalf("Expected HTTP code %d, got %d", first, code)
-	}
-}
-
-func TestSmallResponse(t *testing.T) {
-	code := 500
-	body := "hello"
-	w := httptest.NewRecorder()
-	rw := NewResponseWriter(w)
-	fmt.Fprint(rw, body)
-	rw.WriteHeader(code)
-	rw.Flush()
-
-	if actualCode := w.Code; actualCode != code {
-		t.Fatalf("Expected code %d, got %d", code, actualCode)
-	}
-	if actualBody := w.Body.String(); actualBody != body {
-		t.Fatalf("Expected body %q, got %q", body, actualBody)
-	}
-}
-
-func TestLargeResponse(t *testing.T) {
-	code := 200
-	body := strings.Repeat("0123456789", bufferSize/5) // must exceed the buffer size
-	w := httptest.NewRecorder()
-	rw := NewResponseWriter(w)
-	fmt.Fprint(rw, body)
-	// Because the 'body' was too long this 500 should be ignored
-	rw.WriteHeader(500)
-	rw.Flush()
-
-	if actualCode := w.Code; actualCode != code {
-		t.Fatalf("Expected code %d, got %d", code, actualCode)
-	}
-	if actualBody := w.Body.String(); actualBody != body {
-		t.Fatalf("Expected body %q, got %q", body, actualBody)
-	}
-}

internal/git/git-http.go

@@ -16,7 +16,6 @@ import (
 	"strings"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/delay"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
@@ -54,10 +53,7 @@ func repoPreAuthorizeHandler(myAPI *api.API, handleFunc api.HandleFunc) http.Han
 	}, "")
 }
 
-func handleGetInfoRefs(_w http.ResponseWriter, r *http.Request, a *api.Response) {
-	w := delay.NewResponseWriter(_w)
-	defer w.Flush()
-
+func handleGetInfoRefs(w http.ResponseWriter, r *http.Request, a *api.Response) {
 	rpc := r.URL.Query().Get("service")
 	if !(rpc == "git-upload-pack" || rpc == "git-receive-pack") {
 		// The 'dumb' Git HTTP protocol is not supported
@@ -82,28 +78,26 @@ func handleGetInfoRefs(_w http.ResponseWriter, r *http.Request, a *api.Response)
 	// Start writing the response
 	w.Header().Add("Content-Type", fmt.Sprintf("application/x-%s-advertisement", rpc))
 	w.Header().Add("Cache-Control", "no-cache")
+	w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return
 	if err := pktLine(w, fmt.Sprintf("# service=%s\n", rpc)); err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: pktLine: %v", err))
+		helper.LogError(fmt.Errorf("handleGetInfoRefs: pktLine: %v", err))
 		return
 	}
 	if err := pktFlush(w); err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: pktFlush: %v", err))
+		helper.LogError(fmt.Errorf("handleGetInfoRefs: pktFlush: %v", err))
 		return
 	}
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: copy output of %v: %v", cmd.Args, err))
+		helper.LogError(fmt.Errorf("handleGetInfoRefs: copy output of %v: %v", cmd.Args, err))
 		return
 	}
 	if err := cmd.Wait(); err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: wait for %v: %v", cmd.Args, err))
+		helper.LogError(fmt.Errorf("handleGetInfoRefs: wait for %v: %v", cmd.Args, err))
 		return
 	}
 }
 
-func handlePostRPC(_w http.ResponseWriter, r *http.Request, a *api.Response) {
-	w := delay.NewResponseWriter(_w)
-	defer w.Flush()
-
+func handlePostRPC(w http.ResponseWriter, r *http.Request, a *api.Response) {
 	var err error
 
 	// Get Git action from URL
@@ -149,14 +143,15 @@ func handlePostRPC(_w http.ResponseWriter, r *http.Request, a *api.Response) {
 	// Start writing the response
 	w.Header().Add("Content-Type", fmt.Sprintf("application/x-%s-result", action))
 	w.Header().Add("Cache-Control", "no-cache")
+	w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return
 
 	// This io.Copy may take a long time, both for Git push and pull.
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.Fail500(w, fmt.Errorf("handlePostRPC copy output of %v: %v", cmd.Args, err))
+		helper.LogError(fmt.Errorf("handlePostRPC copy output of %v: %v", cmd.Args, err))
 		return
 	}
 	if err := cmd.Wait(); err != nil {
-		helper.Fail500(w, fmt.Errorf("handlePostRPC wait for %v: %v", cmd.Args, err))
+		helper.LogError(fmt.Errorf("handlePostRPC wait for %v: %v", cmd.Args, err))
 		return
 	}
 }