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

_______________________________________________
nginx mailing list
nginx@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx