mod_reqtimeout backport (was: svn commit: r897527 - /httpd/httpd/branches/2.2.x/STATUS)

[Dan Poirier <po...@pobox.com>]

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

Re: mod_reqtimeout backport

[Dan Poirier <po...@pobox.com>]

Stefan Fritsch <sf...@sfritsch.de> writes:

> In any case, we need at least three values to completely define the
> behaviour. IIRC I chose the initial timeout/maximum timeout over the
> startup time/maximum timeout approach because it was easier to
> implement. I still think it's ok, given that for normal
> configurations, there is not much difference. But the "headerinit"
> keyword is just a bit too cryptic for my taste.

> Do you agree that using RequestReadTimeout instead of RequestTimeout
> and using a single keyword with a timeout range is more descriptive
> than the current syntax?

Yes, I like that better too.

Dan

Re: mod_reqtimeout backport

[Stefan Fritsch <sf...@sfritsch.de>]

On Wed, 13 Jan 2010, Dan Poirier wrote:
>>> We might simplify the model by not exposing the internal extending of
>>> the timeout.  Just let the admin specify an overall max time,
>>> a minimum
>>> rate, or both:
>>>
>>>   HeaderTimeout: Maximum seconds to read the entire header.
>>>
>>>   HeaderMinRate: Minimum rate (bytes/second) allowed when reading the
>>>   request header.  If the rate drops below this, the request is timed
>>>   out.
>>>
>>> We'd enforce both if specified.  In that case HeaderTimeout would act
>>> like headermax.  Internally we'd probably implement HeaderMinRate by
>>> gradually extending a timeout, but we wouldn't be tied to that.
>>
>> But that would result in different behaviour, wouldn't it?
>>
>> e.g. with init timeout set to 10 max timeout set to 30 and minrate set 500
>> the client can wait for 10 seconds before sending data at a rate of 500 bytes/sec.
>> If I understand your model correctly we would cancel the request anytime if the client
>> falls below 500 bytes/sec. So if it does start only with 200 bytes/sec we would cancel
>> it immediately.
>
> Yes, my proposal probably simplifies things too much.  We could allow
> some time for things to get going.  Maybe not start enforcing the
> minimum rate until after some number of seconds, with a reasonable
> default but it could be configured:
>
>  HeaderStartupTime: time in seconds before the specified HeaderMinRate
>  starts being enforced.  Default = 10 seconds.
>
> I'm not thrilled with that though, it's inelegant.

In any case, we need at least three values to completely define the 
behaviour. IIRC I chose the initial timeout/maximum timeout over the 
startup time/maximum timeout approach because it was easier to implement. 
I still think it's ok, given that for normal configurations, there is not 
much difference. But the "headerinit" keyword is just a bit too cryptic 
for my taste.

Do you agree that using RequestReadTimeout instead of RequestTimeout and 
using a single keyword with a timeout range is more descriptive than the 
current syntax?

Re: mod_reqtimeout backport

[Dan Poirier <po...@pobox.com>]

"Plüm, Rüdiger, VF-Group" <ru...@vodafone.com> writes:

>> -----Original Message-----
>> From: news [mailto:news@ger.gmane.org] On Behalf Of Dan Poirier

>> We might simplify the model by not exposing the internal extending of
>> the timeout.  Just let the admin specify an overall max time, 
>> a minimum
>> rate, or both:
>> 
>>   HeaderTimeout: Maximum seconds to read the entire header.
>> 
>>   HeaderMinRate: Minimum rate (bytes/second) allowed when reading the
>>   request header.  If the rate drops below this, the request is timed
>>   out.
>> 
>> We'd enforce both if specified.  In that case HeaderTimeout would act
>> like headermax.  Internally we'd probably implement HeaderMinRate by
>> gradually extending a timeout, but we wouldn't be tied to that.
>
> But that would result in different behaviour, wouldn't it?
>
> e.g. with init timeout set to 10 max timeout set to 30 and minrate set 500
> the client can wait for 10 seconds before sending data at a rate of 500 bytes/sec.
> If I understand your model correctly we would cancel the request anytime if the client
> falls below 500 bytes/sec. So if it does start only with 200 bytes/sec we would cancel
> it immediately.

Yes, my proposal probably simplifies things too much.  We could allow
some time for things to get going.  Maybe not start enforcing the
minimum rate until after some number of seconds, with a reasonable
default but it could be configured:

  HeaderStartupTime: time in seconds before the specified HeaderMinRate
  starts being enforced.  Default = 10 seconds.

I'm not thrilled with that though, it's inelegant.

RE: Re: mod_reqtimeout backport

["Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>]

 

> -----Original Message-----
> From: news [mailto:news@ger.gmane.org] On Behalf Of Dan Poirier
> Sent: Mittwoch, 13. Januar 2010 15:28
> To: dev@httpd.apache.org
> Subject: Re: mod_reqtimeout backport
> 
> Stefan Fritsch <sf...@sfritsch.de> writes:
> 
> > 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.
> 
> We might simplify the model by not exposing the internal extending of
> the timeout.  Just let the admin specify an overall max time, 
> a minimum
> rate, or both:
> 
>   HeaderTimeout: Maximum seconds to read the entire header.
> 
>   HeaderMinRate: Minimum rate (bytes/second) allowed when reading the
>   request header.  If the rate drops below this, the request is timed
>   out.
> 
> We'd enforce both if specified.  In that case HeaderTimeout would act
> like headermax.  Internally we'd probably implement HeaderMinRate by
> gradually extending a timeout, but we wouldn't be tied to that.

But that would result in different behaviour, wouldn't it?

e.g. with init timeout set to 10 max timeout set to 30 and minrate set 500
the client can wait for 10 seconds before sending data at a rate of 500 bytes/sec.
If I understand your model correctly we would cancel the request anytime if the client
falls below 500 bytes/sec. So if it does start only with 200 bytes/sec we would cancel
it immediately.

Regards

Rüdiger

Re: mod_reqtimeout backport

[Dan Poirier <po...@pobox.com>]

Stefan Fritsch <sf...@sfritsch.de> writes:

> 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.

We might simplify the model by not exposing the internal extending of
the timeout.  Just let the admin specify an overall max time, a minimum
rate, or both:

  HeaderTimeout: Maximum seconds to read the entire header.

  HeaderMinRate: Minimum rate (bytes/second) allowed when reading the
  request header.  If the rate drops below this, the request is timed
  out.

We'd enforce both if specified.  In that case HeaderTimeout would act
like headermax.  Internally we'd probably implement HeaderMinRate by
gradually extending a timeout, but we wouldn't be tied to that.

Dan

Re: mod_reqtimeout backport (was: svn commit: r897527 - /httpd/httpd/branches/2.2.x/STATUS)

[Stefan Fritsch <sf...@sfritsch.de>]

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