Hello!

On Wed, Aug 22, 2012 at 09:24:03AM -0400, leki75 wrote:

> Dear Maxim,
>
> thank you for your suggestions. Analyzing the code the following turned out
> about http_write_filter:
> 1. it is able to buffer output (eg. postpone_output)
> 2. can delay response before sending bytes (limit <= 0)
> 3. delays response after sending bytes (nsent - sent)
>
> As you mentioned we delay sending the last byte of the response and only do
> millisecond calculation when sending it.
>
> $ cat limit_rate.patch
> --- src/http/ngx_http_write_filter_module.c 2012-01-18
> 16:07:43.000000000 +0100
> +++ src/http/ngx_http_write_filter_module.c 2012-08-22
> 12:44:03.862873715 +0200
> @@ -47,9 +47,10 @@
> ngx_int_t
> ngx_http_write_filter(ngx_http_request_t *r, ngx_chain_t *in)
> {
> - off_t size, sent, nsent, limit;
> + off_t size, sent, nsent, limit, nlimit;
> ngx_uint_t last, flush;
> ngx_msec_t delay;
> + ngx_time_t *t;
> ngx_chain_t *cl, *ln, **ll, *chain;
> ngx_connection_t *c;
> ngx_http_core_loc_conf_t *clcf;
> @@ -214,6 +215,23 @@
> limit = r->limit_rate * (ngx_time() - r->start_sec + 1)
> - (c->sent - clcf->limit_rate_after);
>
> + if (last && size == 1) {
> + t = ngx_timeofday();
> +
> + if (t->msec < r->start_msec) {
> + t->sec--;
> + t->msec += 1000;
> + }
> +
> + nlimit = r->limit_rate * (t->sec - r->start_sec)
> + + r->limit_rate * (t->msec - r->start_msec) / 1000
> + - (c->sent + size - clcf->limit_rate_after);
> +
> + if (nlimit <= 0) {
> + limit = nlimit;
> + }
> + }
> +
> if (limit <= 0) {
> c->write->delayed = 1;
> ngx_add_timer(c->write,
> @@ -224,6 +242,12 @@
> return NGX_AGAIN;
> }
>
> + if (last && limit > size - 1) {
> + if (size > 1) {
> + limit = size - 1;
> + }
> + }
> +
> if (clcf->sendfile_max_chunk
> && (off_t) clcf->sendfile_max_chunk < limit)
> {

One obvious problem I see that the patch won't delay last byte if
the last flag wasn't yet received by the writer filter. This may
e.g. happen when returning proxied respone as last buffer is sent
separately there, from ngx_http_upstream_finalize_request().

You may also want to avoid delay of a last byte (and a separate
syscall for it) if there is no reason to.

Otherwise it should work and looks much better than previous one.

> It also turned out that setting sendfile_max_chunk to a small enough value
> is also a solution for our problem, but this patch also works with default
> sendfile_max_chunk = 0 setting.
>
> Anyway, in nginx 1.2.3 source we found this:
> if (size == 0 && !(c->buffered & NGX_LOWLEVEL_BUFFERED)) {
> if (last) {
> r->out = NULL;
> c->buffered &= ~NGX_HTTP_WRITE_BUFFERED;
>
> return NGX_OK;
> }
>
> if (flush) {
> do {
> r->out = r->out->next;
> } while (r->out);
>
> c->buffered &= ~NGX_HTTP_WRITE_BUFFERED;
>
> return NGX_OK;
> }
>
> Instead we could use this if I am right:
> if (size == 0 && !(c->buffered & NGX_LOWLEVEL_BUFFERED)) {
> if (last || flush) {
> r->out = NULL;
> c->buffered &= ~NGX_HTTP_WRITE_BUFFERED;
>
> return NGX_OK;
> }

Yes, thank you, the loop is for sure unneeded as is. It probably
was here as an incomple code to free chain links, the following
patch should be ok (and it saves some memory in some special
cases):

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1346755968 -14400
# Node ID ab5db32b035b8cd6babc43553b21e0fd01192e7e
# Parent 190da901d41edc7e07c81b9588bd24254370c591
Http write filter: replaced unneeded loop with one to free chains.

Noted by Gabor Lekeny.

diff --git a/src/http/ngx_http_write_filter_module.c b/src/http/ngx_http_write_filter_module.c
--- a/src/http/ngx_http_write_filter_module.c
+++ b/src/http/ngx_http_write_filter_module.c
@@ -185,23 +185,19 @@ ngx_http_write_filter(ngx_http_request_t
}

if (size == 0 && !(c->buffered & NGX_LOWLEVEL_BUFFERED)) {
- if (last) {
+ if (last || flush) {
+ for (cl = r->out; cl; /* void */) {
+ ln = cl;
+ cl = cl->next;
+ ngx_free_chain(r->pool, ln);
+ }
+
r->out = NULL;
c->buffered &= ~NGX_HTTP_WRITE_BUFFERED;

return NGX_OK;
}

- if (flush) {
- do {
- r->out = r->out->next;
- } while (r->out);
-
- c->buffered &= ~NGX_HTTP_WRITE_BUFFERED;
-
- return NGX_OK;
- }
-
ngx_log_error(NGX_LOG_ALERT, c->log, 0,
"the http output chain is empty");



Maxim Dounin

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