I think I found a fix for the memory leak issue on gRPC module

Sergey Kandaurov pluknet at nginx.com
Mon May 27 10:08:29 UTC 2024


On Wed, May 22, 2024 at 06:14:26PM +0400, Roman Arutyunyan wrote:
> Hi,
> 
> Indeed there's a problem there.  We have similar problems in other places as
> well.  Attached is a patch that fixes all I could find.
> 
> I did some testing for the sub_filter with the following config.  Small buffers
> exaggerate the problem.
> 
> http {
>     server {
>         listen 8000;
> 
>         location / {
>             root html;
> 
>             output_buffers 2 128;
> 
>             sub_filter 1 2;
>             sub_filter_types *;
>             sub_filter_once off;
>         }
>     }
> }
> 
> Retrieving a 10m file resulted in 7304 (patched) vs 1317704 (unpatched) bytes
> allocated from the request pool.  With the default output_buffers (2 32768),
> it's 79880 vs 84040.

Note that unlike in sub_filter and gzip modules,
gRPC logic is more flawed as the buffer size in user controlled.

> 
> 
> On Thu, May 02, 2024 at 07:59:44AM +0000, Pavel Pautov via nginx-devel wrote:
> > Hi Sangmin,
> > 
> > Your analysis looks correct. Client streaming RPCs can lead to unbound accumulation of ngx_chain_t objects (although, at very slow rate). Gzip module had a similar issue (https://trac.nginx.org/nginx/ticket/1046).
> > Attaching somewhat simplified patch based on yours. I was able to reproduce the issue locally and the patch fixes it.
> > 
> > From: nginx-devel <nginx-devel-bounces at nginx.org> On Behalf Of Sangmin Lee
> > Sent: Thursday, April 4, 2024 19:29
> > To: nginx-devel at nginx.org
> > Subject: I think I found a fix for the memory leak issue on gRPC module
> > 
> > CAUTION: This email has been sent from an external source. Do not click links, open attachments, or provide sensitive business information unless you can verify the sender's legitimacy.
> > 
> > I am sending this mail again because I did a mistake while I was writing a mail. I didn't know, in gmail, "Ctrl - Enter" would send a mail immediately even without a title!
> > I am sorry for that, so I am sending again.
> > 
> > Hello,
> > 
> > I think I found the main cause of the memory leak issue when using gRPC stream so I made a patch for it.
> > Please find the test scenario and details here -- This is what I wrote.:
> > https://trac.nginx.org/nginx/ticket/2614
> > After I changed the memory pool totally on nginx and tested our workload -- long-lived gRPC streams with many connections -- using Valgrind and massif, I was able to find what brought up the memory leak issue.
> > like the picture below.
> > 
> > [cid:image001.png at 01DA9C29.C792CD90]
> > ( I am not sure whether this picture will be sent properly )
> > 
> > After I patched one part, it seems okay now I have tested it for 1 week with out workload.
> > 
> > [cid:image002.png at 01DA9C29.C792CD90]
> > ( I am not sure whether this picture will be sent properly )
> > 
> > But because I am not familiar with Mercurial, I couldn't find a way to create PR like on github. I guess this mailing list is for this patch.
> > From my point of view, it is more like a workaround and I think the way of using ngx_chain_add_copy() or itself needs to be changed because it allocates a ngx_chain_t structure using ngx_alloc_chain_link() but inside of that, it just copies pointer, like cl->buf = in->buf;
> > so this ngx_chain_t instance should be dealt with differently unlike other ngx_chain_t instances.
> > But I am quite new to nginx codes so my view might be wrong.
> > Anyhow, please go over this patch and I would like to further talk here.
> > 
> > --------------------------------------------------------------------------------------------------------------------------------------------
> > 
> > diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c
> > index dfe49c586..1db67bd0a 100644
> > --- a/src/http/modules/ngx_http_grpc_module.c
> > +++ b/src/http/modules/ngx_http_grpc_module.c
> > @@ -1462,6 +1462,12 @@ ngx_http_grpc_body_output_filter(void *data, ngx_chain_t *in)
> >          in = in->next;
> >      }
> > 
> > +       ngx_chain_t *nl;
> > +       for (ngx_chain_t *dl = ctx->in; dl != in; dl = nl ) {
> > +               nl = dl->next;
> > +               ngx_free_chain(r->pool, dl);
> > +       }
> > +
> >      ctx->in = in;
> > 
> >      if (last) {
> > 
> > --------------------------------------------------------------------------------------------------------------------------------------------
> > 
> > Best regards,
> > Sangmin
> 
> 
> 
> 
> 
> > _______________________________________________
> > nginx-devel mailing list
> > nginx-devel at nginx.org
> > https://mailman.nginx.org/mailman/listinfo/nginx-devel
> 
> 
> --
> Roman Arutyunyan

> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # Date 1716386385 -14400
> #      Wed May 22 17:59:45 2024 +0400
> # Node ID 95af5fe4921294b48e634defc03b6b0f0201d6f7
> # Parent  e60377bdee3d0f8dbd237b5ae8a5deab2af785ef
> Optimized chain link usage (ticket #2614).
> 
> Previously chain links could sometimes be dropped instead of being reused,
> which could result in increased memory consumption during long requests.
> 
> A similar chain link issue in ngx_http_gzip_filter_module was fixed in
> da46bfc484ef (1.11.10).
> 
> Based on a patch by Sangmin Lee.
> 
> diff --git a/src/core/ngx_output_chain.c b/src/core/ngx_output_chain.c
> --- a/src/core/ngx_output_chain.c
> +++ b/src/core/ngx_output_chain.c
> @@ -117,7 +117,10 @@ ngx_output_chain(ngx_output_chain_ctx_t 
>  
>                  ngx_debug_point();
>  
> -                ctx->in = ctx->in->next;
> +                cl = ctx->in;
> +                ctx->in = cl->next;
> +
> +                ngx_free_chain(ctx->pool, cl);
>  
>                  continue;
>              }
> @@ -203,7 +206,10 @@ ngx_output_chain(ngx_output_chain_ctx_t 
>              /* delete the completed buf from the ctx->in chain */
>  
>              if (ngx_buf_size(ctx->in->buf) == 0) {
> -                ctx->in = ctx->in->next;
> +                cl = ctx->in;
> +                ctx->in = cl->next;
> +
> +                ngx_free_chain(ctx->pool, cl);
>              }
>  
>              cl = ngx_alloc_chain_link(ctx->pool);
> diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c
> --- a/src/http/modules/ngx_http_grpc_module.c
> +++ b/src/http/modules/ngx_http_grpc_module.c
> @@ -1459,7 +1459,10 @@ ngx_http_grpc_body_output_filter(void *d
>              last = 1;
>          }
>  
> +        cl = in;
>          in = in->next;
> +
> +        ngx_free_chain(r->pool, cl);

cl is used later in this function, this change makes it clobbered

>      }
>  
>      ctx->in = in;
> diff --git a/src/http/modules/ngx_http_gunzip_filter_module.c b/src/http/modules/ngx_http_gunzip_filter_module.c
> --- a/src/http/modules/ngx_http_gunzip_filter_module.c
> +++ b/src/http/modules/ngx_http_gunzip_filter_module.c
> @@ -333,6 +333,8 @@ static ngx_int_t
>  ngx_http_gunzip_filter_add_data(ngx_http_request_t *r,
>      ngx_http_gunzip_ctx_t *ctx)
>  {
> +    ngx_chain_t  *cl;
> +
>      if (ctx->zstream.avail_in || ctx->flush != Z_NO_FLUSH || ctx->redo) {
>          return NGX_OK;
>      }
> @@ -344,8 +346,11 @@ ngx_http_gunzip_filter_add_data(ngx_http
>          return NGX_DECLINED;
>      }
>  
> -    ctx->in_buf = ctx->in->buf;
> -    ctx->in = ctx->in->next;
> +    cl = ctx->in;
> +    ctx->in_buf = cl->buf;
> +    ctx->in = cl->next;
> +
> +    ngx_free_chain(r->pool, cl);
>  
>      ctx->zstream.next_in = ctx->in_buf->pos;
>      ctx->zstream.avail_in = ctx->in_buf->last - ctx->in_buf->pos;
> @@ -383,8 +388,12 @@ ngx_http_gunzip_filter_get_buf(ngx_http_
>      conf = ngx_http_get_module_loc_conf(r, ngx_http_gunzip_filter_module);
>  
>      if (ctx->free) {
> -        ctx->out_buf = ctx->free->buf;
> -        ctx->free = ctx->free->next;
> +
> +        cl = ctx->free;
> +        ctx->out_buf = cl->buf;
> +        ctx->free = cl->next;
> +
> +        ngx_free_chain(r->pool, cl);
>  
>          ctx->out_buf->flush = 0;
>  
> diff --git a/src/http/modules/ngx_http_gzip_filter_module.c b/src/http/modules/ngx_http_gzip_filter_module.c
> --- a/src/http/modules/ngx_http_gzip_filter_module.c
> +++ b/src/http/modules/ngx_http_gzip_filter_module.c
> @@ -985,10 +985,12 @@ static void
>  ngx_http_gzip_filter_free_copy_buf(ngx_http_request_t *r,
>      ngx_http_gzip_ctx_t *ctx)
>  {
> -    ngx_chain_t  *cl;
> +    ngx_chain_t  *cl, *next;
>  
> -    for (cl = ctx->copied; cl; cl = cl->next) {
> +    for (cl = ctx->copied; cl; cl = next) {
>          ngx_pfree(r->pool, cl->buf->start);
> +        next = cl->next;
> +        ngx_free_chain(r->pool, cl);
>      }
>  
>      ctx->copied = NULL;
> diff --git a/src/http/modules/ngx_http_ssi_filter_module.c b/src/http/modules/ngx_http_ssi_filter_module.c
> --- a/src/http/modules/ngx_http_ssi_filter_module.c
> +++ b/src/http/modules/ngx_http_ssi_filter_module.c
> @@ -482,9 +482,13 @@ ngx_http_ssi_body_filter(ngx_http_reques
>      while (ctx->in || ctx->buf) {
>  
>          if (ctx->buf == NULL) {
> -            ctx->buf = ctx->in->buf;
> -            ctx->in = ctx->in->next;
> +
> +            cl = ctx->in;
> +            ctx->buf = cl->buf;
> +            ctx->in = cl->next;
>              ctx->pos = ctx->buf->pos;
> +
> +            ngx_free_chain(r->pool, cl);
>          }
>  
>          if (ctx->state == ssi_start_state) {
> diff --git a/src/http/modules/ngx_http_sub_filter_module.c b/src/http/modules/ngx_http_sub_filter_module.c
> --- a/src/http/modules/ngx_http_sub_filter_module.c
> +++ b/src/http/modules/ngx_http_sub_filter_module.c
> @@ -335,9 +335,13 @@ ngx_http_sub_body_filter(ngx_http_reques
>      while (ctx->in || ctx->buf) {
>  
>          if (ctx->buf == NULL) {
> -            ctx->buf = ctx->in->buf;
> -            ctx->in = ctx->in->next;
> +
> +            cl = ctx->in;
> +            ctx->buf = cl->buf;
> +            ctx->in = cl->next;
>              ctx->pos = ctx->buf->pos;
> +
> +            ngx_free_chain(r->pool, cl);
>          }
>  
>          if (ctx->buf->flush || ctx->buf->recycled) {


More information about the nginx-devel mailing list