Using rate limitation for files smaller than the defined limit.

[Maxim Dounin]

Hello!

On Mon, Jul 30, 2012 at 08:22:42AM -0400, leki75 wrote:

> Hi!
> 
> We use nginx rate limiting function but realized that it does not work
> for files smaller than the limit specified in limit_rate directive.
> Analyzing the source code it turned out that forcing the limit is second
> based. We modified the source to be able to rate limit more precisely
> and it also works for smaller files.
> 
> Here is the patch:
> leki at szunyog:~/src/agwlimit/nginx-1.2.2$ diff -u
> src/http/ngx_http_write_filter_module.c{.old,}
> --- src/http/ngx_http_write_filter_module.c.old 2012-07-30
> 08:32:13.155354399 +0200
> +++ src/http/ngx_http_write_filter_module.c     2012-07-30
> 13:55:01.074823215 +0200
> @@ -211,8 +211,15 @@
>      }
>  
>      if (r->limit_rate) {
> -        limit = r->limit_rate * (ngx_time() - r->start_sec + 1)
> -                - (c->sent - clcf->limit_rate_after);
> +        if (ngx_cached_time->msec < r->start_msec) {

Using ngx_cached_time directly is a bad idea.  While it likely 
will work correctly right now (unless you are really happy and the 
expression will be broken in the middle by a timer signal, 
assuming timer_resolution used on linux), it's quite likely to 
break in the future.

Using ngx_timeofday() instead should be better.

> +            limit = r->limit_rate * (ngx_time() - r->start_sec - 1)
> +                    + r->limit_rate * (ngx_cached_time->msec + 1000 -
> r->start_msec) / 1000
> +                    - (c->sent - clcf->limit_rate_after);
> +        } else {
> +            limit = r->limit_rate * (ngx_time() - r->start_sec)
> +                    + r->limit_rate * (ngx_cached_time->msec -
> r->start_msec) / 1000
> +                    - (c->sent - clcf->limit_rate_after);

This basically limits to 0 at request start unless 
limit_rate_after is set to something non-zero, resulting in a 
first byte delay for all requests.

> +        }
>  
>          if (limit <= 0) {
>              c->write->delayed = 1;
> 
> As we tested the pathced nginx it worked fine. Could we use it in
> production or do you see any mistakes we made?

The above might be ok for your particular use case, though 
problems outlined above (and below) might cause suboptimal 
performance.

> Is there any reason you don't count on milliseconds?

Current code uses seconds as it's easier to handle, faster 
(implies less syscalls and less packets over network) and provides 
some inherent size for a first write.

With milliseconds and a 4k/s limit (as used in example in docs, 
see http://nginx.org/r/limit_rate) there will be 4 byte write each 
millisecond, which is quite inefficient.

Maxim Dounin