Prevent writing the receive-pack response to early

With the new way we handle Git HTTP subprocesses https://gitlab.com/gitlab-org/gitlab-workhorse/merge_requests/118 there is a possibility we start writing an HTTP response before consuming the entire request. This may cause errors (none have been reported yet); this change introduces a buffer on the response writer that gets flushed once the request body has been consumed.

https://golang.org/pkg/net/http/#ResponseWriter

For HTTP/1.x requests, handlers should read any needed request body data before writing the response. Once the headers have been flushed (due to either an explicit Flusher.Flush call or writing enough data to trigger a flush), the request body may be unavailable.

assigned to @nick.thomas

assigned to @jacobvosmaer-gitlab

added 1 commit

• 5e76a4dc - Prevent writing the receive-pack response to early

Compare with previous version

I changed things up a bit but the mutex is still there. What do you think @nick.thomas ?

assigned to @nick.thomas

resolved all discussions

Sorry for the delay reviewing this @jacobvosmaer-gitlab

The code now looks correct for the purpose it's being put to, but I still have some reservations about the approach. I don't know if I'm just being too conservative, or thinking too far ahead, but imagine what happens if this code is used as part of a system that expects temporary errors, and retries reads/writes in that case. It will act in very surprising ways.

If you're confident in this approach, I'm happy enough to merge it; I'm struggling to find any reason why I don't like it now!

assigned to @jacobvosmaer-gitlab

@nick.thomas what if we make the error state 'sticky'?

• in the reader, if err != io.EOF, decorate it with a type of our own (so no one can classify it as transient)
• in the writer, if any error occurs, decorate it and keep it in a struct field, and return it on all future Write() calls

Or is the thing that bothers you that busyReader.Read() will keep repeating the same error?

added 2 commits

• f0fd9fdb - Prevent callers from recognizing read errors
• e696f2ac - Make tempfile write errors permanent

Compare with previous version

assigned to @nick.thomas

mentioned in issue #102 (closed)

@nick.thomas this can use another round of review.

mentioned in commit 261e6951

merged