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 email@example.com 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.