Re: I think I found a fix for the memory leak issue on gRPC module
On Mon, May 27, 2024 at 02:06:58PM +0400, Roman Arutyunyan wrote:
> Hi,
> # HG changeset patch
> # User Roman Arutyunyan
> # Date 1716477338 -14400
> # Thu May 23 19:15:38 2024 +0400
> # Node ID f7d53c7f70140b1cd1eaf51ce4346a873692f879
> # Parent f58b6f6362387eeace46043a6fc0bceb56a6786a
> 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
> @@ -1231,7 +1231,7 @@ ngx_http_grpc_body_output_filter(void *d
> ngx_buf_t *b;
> ngx_int_t rc;
> ngx_uint_t next, last;
> -ngx_chain_t*cl, *out, **ll;
> +ngx_chain_t*cl, *out, *ln, **ll;
> ngx_http_upstream_t*u;
> ngx_http_grpc_ctx_t*ctx;
> ngx_http_grpc_frame_t *f;
> @@ -1459,7 +1459,10 @@ ngx_http_grpc_body_output_filter(void *d
> last = 1;
> }
>
> +ln = in;
> in = in->next;
> +
> +ngx_free_chain(r->pool, ln);
> }
Looks good now.
>
> 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;
> @@ -374,6 +379,7 @@ static ngx_int_t
> ngx_http_gunzip_filter_get_buf(ngx_http_request_t *r,
> ngx_http_gunzip_ctx_t *ctx)
> {
> +ngx_chain_t *cl;
> ngx_http_gunzip_conf_t *conf;
>
> if (ctx->zstream.avail_out) {
> @@ -383,8 +389,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,14 @@ 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, *ln;
>
> -for (cl = ctx->copied; cl; cl = cl->next) {
> -ngx_pfree(r->pool, cl->buf->start);
> +for (cl = ctx->copied; cl; /* void */) {
> +ln = cl;
> +cl = cl->next;
> +
> +ngx_pfree(r->pool, ln->buf->start);
> +ngx_free_chain(r->pool, ln);
> }
>
> 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 @@
Re: I think I found a fix for the memory leak issue on gRPC module
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 +, 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 On Behalf Of Sangmin Lee
> > Sent: Thursday, April 4, 2024 19:29
> > To: nginx-devel@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@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@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) {
> >
> > ---
Re: I think I found a fix for the memory leak issue on gRPC module
Hi,
Following an internal discussion with Sergey, here's an updated version of
the patch.
On Thu, May 23, 2024 at 01:42:24PM +0400, Roman Arutyunyan wrote:
> Hi,
>
> 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.
>
> I tested ssi with the following config.
>
> server {
> listen 8000;
>
> location / {
> root html;
>
> output_buffers 2 128;
>
> ssi on;
> ssi_types *;
> }
> }
>
> The result is similar:
>
> 6224 vs 1316912 with small buffers
> 38864 vs 43952 with the default buffers
>
> > On Thu, May 02, 2024 at 07:59:44AM +, 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 On Behalf Of Sangmin Lee
> > > Sent: Thursday, April 4, 2024 19:29
> > > To: nginx-devel@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@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@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.
> > >
> > > --
Re: I think I found a fix for the memory leak issue on gRPC module
Hi,
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.
I tested ssi with the following config.
server {
listen 8000;
location / {
root html;
output_buffers 2 128;
ssi on;
ssi_types *;
}
}
The result is similar:
6224 vs 1316912 with small buffers
38864 vs 43952 with the default buffers
> On Thu, May 02, 2024 at 07:59:44AM +, 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 On Behalf Of Sangmin Lee
> > Sent: Thursday, April 4, 2024 19:29
> > To: nginx-devel@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@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@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-&g
Re: I think I found a fix for the memory leak issue on gRPC module
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.
On Thu, May 02, 2024 at 07:59:44AM +, 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 On Behalf Of Sangmin Lee
> Sent: Thursday, April 4, 2024 19:29
> To: nginx-devel@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@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@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@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan
# 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
