Closing upstream keepalive connections in an invalid state
mdounin at mdounin.ru
Tue Dec 1 14:06:48 UTC 2015
On Tue, Dec 01, 2015 at 12:19:27PM +0000, Chris Branch wrote:
> Thanks for your feedback!
> > On 30 Nov 2015, at 18:09, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > The patch looks incomplete for me. It doesn't seem to handle the
> > "next upstream" case. And the condition used looks wrong, too, as
> > it doesn't take into account what nginx actually tried to send.
> Next upstream was indeed forgotten, and is relevant for the case
> of buffered request + large request body (not tested).
> However I disagree that the condition looks wrong. So long as a
> request is sent in full, the connection remains usable - that’s
> defined solely by the data remaining in our buffers and the data
> we are waiting for from the client. There are potential
> improvements to the keepalive handling for which ‘what nginx
> actually tried to send’ is irrelevant:
> - If using chunked encoding, send a trailer sequence to place
> the upstream connection in a valid state
> - Discard the request body to keep the downstream connection
The condition you use is as follows:
+ if (u->writer.out != NULL
+ || (r->request_body && r->request_body->rest > 0))
It fails if u->writer has something unsent in it, right. This
part per se isn't enough though, as it clearly doesn't take into
account unbuffered request body. So you've added r->request_body
checks - but why r->request_body have to be considered at all if,
e.g., proxy_pass_request_body was set to off or proxy_set_body was
used? And what happens when we've read all the body, and then an
error happened in a request body filter - e.g., when we've tried
to write it to the request body file? The condition is also
likely to be wrong in case of some error while reading the request
body file from disk.
> Having a special flag for this simple case seems unnecessary. It
> is surely more robust to make a decision based on our current
> state rather than indirectly deciding that with a flag. You must
> agree or you wouldn’t dislike the quick and dirty (but otherwise
> completely functional) patch :)
Sure, I don't like the special flag too. Though the condition
proposed clearly isn't good enough either.
More information about the nginx-devel