Re: svn commit: r1779525 - /httpd/httpd/trunk/modules/http2/h2_mplx.c

[Stefan Eissing]

> Am 20.01.2017 um 21:59 schrieb Jim Jagielski :
> 
> 
>> On Jan 19, 2017, at 4:00 PM, Ruediger Pluem  wrote:
>> 
>> 
>> 
>> On 01/19/2017 09:38 PM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Thu Jan 19 20:38:50 2017
>>> New Revision: 1779525
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1779525=rev
>>> Log:
>>> On the trunk:
>>> 
>>> mod_http2: decoupling lifetime of mplx pool from h2_session which messed up 
>>> the cleanup ordering.
>>> 
>>> 
>>> Modified:
>>>   httpd/httpd/trunk/modules/http2/h2_mplx.c
>>> 
>>> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1779525=1779524=1779525=diff
>>> ==
>>> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
>>> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Jan 19 20:38:50 2017
>>> @@ -280,7 +280,7 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
>>>m->id = c->id;
>>>APR_RING_ELEM_INIT(m, link);
>>>m->c = c;
>>> -apr_pool_create_ex(>pool, parent, NULL, allocator);
>>> +apr_pool_create_ex(>pool, NULL, NULL, allocator);
>> 
>> Without further investigations: Global pools always make me worry. Are you 
>> sure we don't introduce
>> a memory leak here?
>> 
> 
> Especially untagged ones!

Has already been reverted.

Stefan Eissing

bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de

Re: svn commit: r1779525 - /httpd/httpd/trunk/modules/http2/h2_mplx.c

[Jim Jagielski]

> On Jan 19, 2017, at 4:00 PM, Ruediger Pluem  wrote:
> 
> 
> 
> On 01/19/2017 09:38 PM, ic...@apache.org wrote:
>> Author: icing
>> Date: Thu Jan 19 20:38:50 2017
>> New Revision: 1779525
>> 
>> URL: http://svn.apache.org/viewvc?rev=1779525=rev
>> Log:
>> On the trunk:
>> 
>> mod_http2: decoupling lifetime of mplx pool from h2_session which messed up 
>> the cleanup ordering.
>> 
>> 
>> Modified:
>>httpd/httpd/trunk/modules/http2/h2_mplx.c
>> 
>> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1779525=1779524=1779525=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Jan 19 20:38:50 2017
>> @@ -280,7 +280,7 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
>> m->id = c->id;
>> APR_RING_ELEM_INIT(m, link);
>> m->c = c;
>> -apr_pool_create_ex(>pool, parent, NULL, allocator);
>> +apr_pool_create_ex(>pool, NULL, NULL, allocator);
> 
> Without further investigations: Global pools always make me worry. Are you 
> sure we don't introduce
> a memory leak here?
> 

Especially untagged ones!

Re: svn commit: r1779525 - /httpd/httpd/trunk/modules/http2/h2_mplx.c

[Stefan Eissing]

> Am 19.01.2017 um 22:00 schrieb Ruediger Pluem :
> On 01/19/2017 09:38 PM, ic...@apache.org wrote:
>> Author: icing
>> Date: Thu Jan 19 20:38:50 2017
>> New Revision: 1779525
>> 
>> URL: http://svn.apache.org/viewvc?rev=1779525=rev
>> Log:
>> On the trunk:
>> 
>> mod_http2: decoupling lifetime of mplx pool from h2_session which messed up 
>> the cleanup ordering.
>> 
>> 
>> Modified:
>>httpd/httpd/trunk/modules/http2/h2_mplx.c
>> 
>> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1779525=1779524=1779525=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Jan 19 20:38:50 2017
>> @@ -280,7 +280,7 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
>> m->id = c->id;
>> APR_RING_ELEM_INIT(m, link);
>> m->c = c;
>> -apr_pool_create_ex(>pool, parent, NULL, allocator);
>> +apr_pool_create_ex(>pool, NULL, NULL, allocator);
> 
> Without further investigations: Global pools always make me worry. Are you 
> sure we don't introduce
> a memory leak here?

It's good that someone's looking at this. This pool is taken down inside a 
pre_cleanup registry of the parent. So, there is no mem leak, but it did not 
change anything either. Just double-checked that pre_cleanups run before child 
pools are destroyed. So, I will revert this. But again, thanks for checking.

> Regards
> 
> Rüdiger

Stefan Eissing

bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de

Re: svn commit: r1779525 - /httpd/httpd/trunk/modules/http2/h2_mplx.c

[Ruediger Pluem]

On 01/19/2017 09:38 PM, ic...@apache.org wrote:
> Author: icing
> Date: Thu Jan 19 20:38:50 2017
> New Revision: 1779525
> 
> URL: http://svn.apache.org/viewvc?rev=1779525=rev
> Log:
> On the trunk:
> 
> mod_http2: decoupling lifetime of mplx pool from h2_session which messed up 
> the cleanup ordering.
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> 
> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1779525=1779524=1779525=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Thu Jan 19 20:38:50 2017
> @@ -280,7 +280,7 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
>  m->id = c->id;
>  APR_RING_ELEM_INIT(m, link);
>  m->c = c;
> -apr_pool_create_ex(>pool, parent, NULL, allocator);
> +apr_pool_create_ex(>pool, NULL, NULL, allocator);

Without further investigations: Global pools always make me worry. Are you sure 
we don't introduce
a memory leak here?

Regards

Rüdiger