Feature #2924
closedSuboptimal keep-alive handling when request body is not read
Description
- A HTTP client performs a HTTP request.
- The request is eligible for keep-alive (e.g. because HTTP/1.1 was used)
- The request includes a request body (e.g. POST)
- lighttpd determines a reply without reading the request body (e.g. mod_auth returns 401)
In this case, the connection_handle_response_end_state function determines that keep-alive is not possible and initiates a connection shutdown. A remote client may be unprepared for such a shutdown, because lighttpd did not include a "Connection: close" header in its reply and attempt a second request on the same connection. This is subject to a race condition (i.e. if the shutdown is deferred compared to the response). When reproducing the issue, I hit that race at most in 0.1% of the attempts (i.e. quite unlikely). A client that is prone to hitting it is python-requests. python-requests' behaviour is suboptimal as well as is documented at https://github.com/requests/requests/issues/4664.
I propose that when lighttpd sends its response headers, it performs the same check as in connection_handle_response_end_state and adds the relevant Connection: close header. "gps" pointed out that this may be undesirable when using server.stream-request-body = 1 with mod_proxy. Requiring server.stream-request-body = 0 is a reasonable compromise to me. Is the patch a reasonable trade-off?
Files
Updated by helmut almost 6 years ago
- Not reproducible on 1.4.35
- Reproducible on 1.4.45
- Reproducible on 1.4.52
Updated by gstrauss almost 6 years ago
- Tracker changed from Bug to Feature
- Target version changed from 1.4.x to 1.4.53
A remote client may be unprepared for such a shutdown, because lighttpd did not include a "Connection: close" header in its reply and attempt a second request on the same connection.
That is a bug, but not in lighttpd.
This ticket is a feature request.
The patch as provided is too blunt. As noted, it may result in premature closure of keep-alive connections when server.stream-request-body = 1
is in use and the backend has not yet read the full request body.
The following might be a better solution, since it will trigger "Connection: close" if the response body has not been fully read and one of: there is no backend which might be streaming the request body or there is no streaming request body. The short-circuit to check for any content length in the patch below is because many requests are GET requests without a request body.
/* disable keep-alive if requested */ if (con->request_count > con->conf.max_keep_alive_requests || 0 == con->conf.max_keep_alive_idle) { con->keep_alive = 0; + } else if (0 != con->request.content_length + && con->request.content_length != con->request_content_queue->bytes_in + && (con->mode == DIRECT || 0 == con->conf.stream_request_body)) { + con->keep_alive = 0; } else { con->keep_alive_idle = con->conf.max_keep_alive_idle; }
Updated by gstrauss almost 6 years ago
- Status changed from New to Fixed
- % Done changed from 0 to 100
Applied in changeset 629b16f188173b1d4d7434fc68fb85938b2fc582.
Updated by helmut almost 6 years ago
Thank you for your review and fixing the issue with improved checks. I expect that your version fully resolves the pratical problems I'm seeing.
Also available in: Atom