Add write deadline to Git HTTP responses

Fixes https://gitlab.com/gitlab-org/gitlab-workhorse/issues/66

High-level approach to cleaning up connections that don't finish reading their HTTP response.

A Go HTTP response has two parts: one call to WriteHeader and multiple calls to Write. Each of these calls, which normally takes less than a second (think: sending 32kB of data over a TCP connection), gets a 10-minute deadline. By putting a new deadline on each call, instead of a deadline on the entire request-reponse cycle, we allow arbitrarily large responses to be sent.

This is a rough sketch but it seems to work and I think this is an approach that may solve the problem of lingering git upload-pack processes without requiring careful configuration from the administrator, or getting in the way of normal clone requests.

assigned to @nick.thomas

@pcarranza what do you think about this approach?

mentioned in issue #66

It makes sense, if we manage to write something we return, else we linger until we hit the timeout.

I just wonder if we should consider retrying in case the connection is flapping somehow, but I'm good with the approach.

(Interesting to see how we have to use channels because we don't have select available)

I would rather raise the timeout than complicate this algorithm with retries. Other parts of the stack already handle retries, no need to interfere with that.

(Interesting to see how we have to use channels because we don't have select available)

Actually, I think it is a nice thing that Go allows you to write select-style code with arbitrary inputs (the inputs being channels). The channels make it more general and friendly than Unix select(2).

marked as a Work In Progress

added 1 commit

• 41f630bf - WIP TimeoutResponseWriter

Compare with previous version

We can use syscall.Select if we really want to, but it's not very go-ish.

A better option than select would be to Hijack() the ResponseWriter, get the net.Conn and write to that, using the normal SetDeadline methods to manage timeouts.

What I like about this approach is that all the magic is hidden inside a responsewriter. I can imagine another approach where we have a byte counter inside the writer 'monitor' goroutine that periodically checks if the counter went up. If the counter stalled, send a message on a channel, and then the handler can decide to abort the request.

I am less enthusiastic about hijacking because that means taking on a lot more responsibility in our code.

I think what we have here (modulo better tests etc.) is the easiest to use: just wrap the responsewriter.

mentioned in merge request !80 (merged)

added 1 commit

• 021a65a8 - TimeoutResponseWriter, not just Writer

Compare with previous version

added 1 commit

• 63e81f17 - Put responsewriter in timeout packge

Compare with previous version

added 1 commit

• 14444864 - Add write deadline to Git HTTP responses

Compare with previous version

unmarked as a Work In Progress

changed milestone to %8.16

@nick.thomas no longer WIP as far as I am concerned.

mentioned in merge request !110 (merged)

resolved all discussions

@jacobvosmaer-gitlab the code itself LGTM. You mention in !110 (merged) that this may be unneeded; feel free to merge or not depending on your decision on that!

assigned to @jacobvosmaer-gitlab

removed milestone

I took the milestone off because https://gitlab.com/gitlab-org/gitlab-workhorse/issues/92 suggests this MR would not solve the hanging upload-pack problem in the first place.

added 1 commit

• 38836efe - Panic upon timeout

Compare with previous version

It is suggested here we could also use panic as an abort mechanism: https://github.com/golang/go/issues/16100#issuecomment-255498503

I am pushing a new commit that implements panicking. @nick.thomas what do you think?

I am somehow hesitant to merge this. I think I will close this MR for the time being.

closed