Closing upstream keepalive connections in an invalid state

[Maxim Dounin]

Hello!

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 
> alive

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.

-- 
Maxim Dounin
http://nginx.org/