On Wed, 9 Jun 2004, Jim Jagielski wrote:
> On Jun 9, 2004, at 3:24 PM, Rasmus Lerdorf wrote:
> > I guess what we are agreeing on here is that the logic that sets
> > keepalive
> > to 0 is faulty and that is probably where the real fix lies.
> yeah... it's pretty inconsistent. Looking at ap_set_keepalive
> even after we know the connection will be closed, we
> set keepalive to 0, for example.
Ok, I had a closer look at the flow. There are 6 main cases I care about.
Static, Dynamic and Error requests for configs KeepAlive=Off and
KeepAlive=On. Here is what happens:
With KeepAlive On
For both static and dynamic requests the flow is similar.
We start connection->keepalive starts at 0 at the beginning of the request
process_request_internal eventually leads to an ap_send_http_header call
which calls ap_set_keepalive which determines that the config has
KeepAlive on and sets connection->keepalive to 1
For an error, like a 404, it is different. We start off with
connection->keepalive being set to 0, process_internal_request calls
ap_die on the error which calls ap_send_error_response which in turn calls
ap_send_http_header which finally sets connection_keepalive to 1. But
this happens after ap_die checks conn->keepalive to determine whether to
discard the request body or not.
With KeepAlive Off
The picture is as above, except ap_set_keepalive called from
ap_send_http_header sets connection->keepalive to 0 instead of 1. So for
the duration of the request, before and after checking whether we are on a
keepalive connection, connection->keepalive is 0.
The summary here is that checking connection->keepalive before
ap_set_keepalive() has been called really doesn't make any sense. And we
can't just call ap_set_keepalive in ap_die before the check because it
would end up getting called twice and there is no guard against that in
it. It would double-count the request in the keepalives counter. We need
to either call ap_set_keepalive earlier on, like in
process_request_internal before we hit ap_die, or we need to add a
double-call guard in it and just add a call in ap_die before the keepalive
check.
Another alternative would be to clean up this mess which has our undecided
state indistinguishable from our disabled state. Checking for 0 in ap_die
is only wrong because the check is before the ap_set_keepalive call. The
meaning of that 0 changes on that call.
-Rasmus
