Some comments based on
<http://people.apache.org/~sf/mod_reqtimeout.2.2.patch>:
Code:
- The units are confusing in the computation and use of the rate_factor
values. rate_factor is computed as
apr_time_from_sec(1)/min_rate
where min_rate's units are bytes/second, so the units for the
rate_factor are seconds^2/(10^6 * bytes). Then in extend_timeout(),
that gets multipled by a length in bytes, so we end up with units of
seconds^2/10^6 where we really want microseconds.
I think it would be clearer to write the computation of the
rate_factor as
rate_factor = APR_USEC_PER_SEC / min_rate
so the units work out as usec/byte and the timeout ends up
in usec.
(Yes, you end up with the same numbers, but it took me quite a while
to prove that to myself, given that the units weren't right.)
- I'd feel better if most of the AP_DEBUG_ASSERTs were replaced by code
that would fail the request. In production this could result in
crashes at runtime if any of these bad return values actually happen.
- If headermax precedes headerinit on the RequestTimeout directive line,
there's no check that headerinit <= headermax. Similarly for bodymax
and bodyinit.
Documentation:
- Should the module status still be "Experimental" if it's backported?
- Compatibility should show this available in 2.2.something if it's
backported.
- s/timout/timeout/g
- Maybe an explanatory paragraph should be added, about xxxinit and
xxxminrate and xxxmax and how they relate to each other and affect the
timeout of requests. That would resolve some questions I had as I was
reading through this, like "why is this called headerinit instead of
headerread?".
Dan
