Thank you very much for your comments, Dan.
On Tuesday 12 January 2010, Dan Poirier wrote:
> - 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 this is personal preference. For me, it is very obvious that
rate_factor has the unit
whatever_unit_apr_time_t_uses / bytes
And I don't have to know if apr_time_t uses microsecs, nanosecs, or
whatever. But YMMV, of course.
> - 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.
The AP_DEBUG_ASSERTs are in places where other modules don't do any
checks and just dereference the pointers. It return an error in the
beginning, but someone here suggested to use asserts instead. I am ok
with both ways, but I think that AP_DEBUG_ASSERT is more consistent
with other modules.
> - If headermax precedes headerinit on the RequestTimeout directive
> line, there's no check that headerinit <= headermax. Similarly
> for bodymax and bodyinit.
True. I will fix that, unless we change the syntax (see below).
> Documentation:
>
> - Should the module status still be "Experimental" if it's
> backported?
I don't know. Maybe backport it as 'experimental' first and then
promote it to 'extension' after one 2.2.x release?
> - Compatibility should show this available in 2.2.something if it's
> backported.
>
> - s/timout/timeout/g
>
Thanks, will fix.
> - 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?".
I am not particularly happy about the syntax, yet. I just had the idea
to have one keyword xxx that can optionally accept a range, instead of
two keywords xxxinit and xxxmax:
Header=30
Body=5-50 BodyMinRate=500
or
Header=5-20,minrate=500
Body=5-50,minrate=500
but the combination
Header=5,minrate=1000
is less clear. Maybe make the '-' mandatory in this case:
Header=5-,minrate=1000
Any opinions or better ideas?
One could also rename RequestTimeout into RequestReadTimeout if that
makes it easier to understand.
Cheers,
Stefan
