httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fritsch ...@sfritsch.de>
Subject Re: mod_reqtimeout backport (was: svn commit: r897527 - /httpd/httpd/branches/2.2.x/STATUS)
Date Tue, 12 Jan 2010 23:30:37 GMT
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

Mime
View raw message