eor bucket

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

eor bucket

Stefan Eissing
Could someone help me to check my understanding of the eor bucket implementation?

If an eor bucket is ever copied, there are 2 buckets with their b->data pointing to the request_rec. Since this is local to the bucket, destroying these 2 will call apr_pool_destroy() twice on the pool. Correct?

-Stefan
Reply | Threaded
Open this post in threaded view
|

AW: eor bucket

Plüm, Rüdiger, Vodafone Group
I think your understanding is correct.

Regards

Rüdiger


C2 General

> -----Ursprüngliche Nachricht-----
> Von: Stefan Eissing <[hidden email]>
> Gesendet: Donnerstag, 9. Mai 2019 11:02
> An: [hidden email]
> Betreff: eor bucket
>
> Could someone help me to check my understanding of the eor bucket
> implementation?
>
> If an eor bucket is ever copied, there are 2 buckets with their b->data
> pointing to the request_rec. Since this is local to the bucket,
> destroying these 2 will call apr_pool_destroy() twice on the pool.
> Correct?
>
> -Stefan
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Yann Ylavic
Hmm, if r->pool gets destroyed by the first eor, the
eor_bucket_cleanup() for the copy should NULLify its b->data at the
same time, so it should be safe no?

On Thu, May 9, 2019 at 11:22 AM Plüm, Rüdiger, Vodafone Group
<[hidden email]> wrote:

>
> I think your understanding is correct.
>
> Regards
>
> Rüdiger
>
>
> C2 General
>
> > -----Ursprüngliche Nachricht-----
> > Von: Stefan Eissing <[hidden email]>
> > Gesendet: Donnerstag, 9. Mai 2019 11:02
> > An: [hidden email]
> > Betreff: eor bucket
> >
> > Could someone help me to check my understanding of the eor bucket
> > implementation?
> >
> > If an eor bucket is ever copied, there are 2 buckets with their b->data
> > pointing to the request_rec. Since this is local to the bucket,
> > destroying these 2 will call apr_pool_destroy() twice on the pool.
> > Correct?
> >
> > -Stefan
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Yann Ylavic
No it's not actually, nevermind.

On Thu, May 9, 2019 at 11:24 AM Yann Ylavic <[hidden email]> wrote:

>
> Hmm, if r->pool gets destroyed by the first eor, the
> eor_bucket_cleanup() for the copy should NULLify its b->data at the
> same time, so it should be safe no?
>
> On Thu, May 9, 2019 at 11:22 AM Plüm, Rüdiger, Vodafone Group
> <[hidden email]> wrote:
> >
> > I think your understanding is correct.
> >
> > Regards
> >
> > Rüdiger
> >
> >
> > C2 General
> >
> > > -----Ursprüngliche Nachricht-----
> > > Von: Stefan Eissing <[hidden email]>
> > > Gesendet: Donnerstag, 9. Mai 2019 11:02
> > > An: [hidden email]
> > > Betreff: eor bucket
> > >
> > > Could someone help me to check my understanding of the eor bucket
> > > implementation?
> > >
> > > If an eor bucket is ever copied, there are 2 buckets with their b->data
> > > pointing to the request_rec. Since this is local to the bucket,
> > > destroying these 2 will call apr_pool_destroy() twice on the pool.
> > > Correct?
> > >
> > > -Stefan
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Yann Ylavic
The issue is more that the hooks in eor_bucket_cleanup() will be run
multiple times, rather than a lifetime issue.

On Thu, May 9, 2019 at 11:28 AM Yann Ylavic <[hidden email]> wrote:

>
> No it's not actually, nevermind.
>
> On Thu, May 9, 2019 at 11:24 AM Yann Ylavic <[hidden email]> wrote:
> >
> > Hmm, if r->pool gets destroyed by the first eor, the
> > eor_bucket_cleanup() for the copy should NULLify its b->data at the
> > same time, so it should be safe no?
> >
> > On Thu, May 9, 2019 at 11:22 AM Plüm, Rüdiger, Vodafone Group
> > <[hidden email]> wrote:
> > >
> > > I think your understanding is correct.
> > >
> > > Regards
> > >
> > > Rüdiger
> > >
> > >
> > > C2 General
> > >
> > > > -----Ursprüngliche Nachricht-----
> > > > Von: Stefan Eissing <[hidden email]>
> > > > Gesendet: Donnerstag, 9. Mai 2019 11:02
> > > > An: [hidden email]
> > > > Betreff: eor bucket
> > > >
> > > > Could someone help me to check my understanding of the eor bucket
> > > > implementation?
> > > >
> > > > If an eor bucket is ever copied, there are 2 buckets with their b->data
> > > > pointing to the request_rec. Since this is local to the bucket,
> > > > destroying these 2 will call apr_pool_destroy() twice on the pool.
> > > > Correct?
> > > >
> > > > -Stefan
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Stefan Eissing
In reply to this post by Yann Ylavic


> Am 09.05.2019 um 11:28 schrieb Yann Ylavic <[hidden email]>:
>
> No it's not actually, nevermind.

Yeah, I was going back and forth like that myself. Therefore my question. ;)

I am seeing in an uncomitted change a rare occasion where eor_bucket_destroy() seems to destroy a pool twice. If that is related to a bucket copy, I have not found out yet. But it's one more peculiarity to keep in mind...

Thanks, guys!


> On Thu, May 9, 2019 at 11:24 AM Yann Ylavic <[hidden email]> wrote:
>>
>> Hmm, if r->pool gets destroyed by the first eor, the
>> eor_bucket_cleanup() for the copy should NULLify its b->data at the
>> same time, so it should be safe no?
>>
>> On Thu, May 9, 2019 at 11:22 AM Plüm, Rüdiger, Vodafone Group
>> <[hidden email]> wrote:
>>>
>>> I think your understanding is correct.
>>>
>>> Regards
>>>
>>> Rüdiger
>>>
>>>
>>> C2 General
>>>
>>>> -----Ursprüngliche Nachricht-----
>>>> Von: Stefan Eissing <[hidden email]>
>>>> Gesendet: Donnerstag, 9. Mai 2019 11:02
>>>> An: [hidden email]
>>>> Betreff: eor bucket
>>>>
>>>> Could someone help me to check my understanding of the eor bucket
>>>> implementation?
>>>>
>>>> If an eor bucket is ever copied, there are 2 buckets with their b->data
>>>> pointing to the request_rec. Since this is local to the bucket,
>>>> destroying these 2 will call apr_pool_destroy() twice on the pool.
>>>> Correct?
>>>>
>>>> -Stefan

Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Stefan Eissing
In reply to this post by Yann Ylavic


> Am 09.05.2019 um 11:32 schrieb Yann Ylavic <[hidden email]>:
>
> The issue is more that the hooks in eor_bucket_cleanup() will be run
> multiple times, rather than a lifetime issue.

I read it like this:
The cleanup is only registered on the first creation. The copy never registers. But the bucket_destroy calls the pool destroy each time.

>
> On Thu, May 9, 2019 at 11:28 AM Yann Ylavic <[hidden email]> wrote:
>>
>> No it's not actually, nevermind.
>>
>> On Thu, May 9, 2019 at 11:24 AM Yann Ylavic <[hidden email]> wrote:
>>>
>>> Hmm, if r->pool gets destroyed by the first eor, the
>>> eor_bucket_cleanup() for the copy should NULLify its b->data at the
>>> same time, so it should be safe no?
>>>
>>> On Thu, May 9, 2019 at 11:22 AM Plüm, Rüdiger, Vodafone Group
>>> <[hidden email]> wrote:
>>>>
>>>> I think your understanding is correct.
>>>>
>>>> Regards
>>>>
>>>> Rüdiger
>>>>
>>>>
>>>> C2 General
>>>>
>>>>> -----Ursprüngliche Nachricht-----
>>>>> Von: Stefan Eissing <[hidden email]>
>>>>> Gesendet: Donnerstag, 9. Mai 2019 11:02
>>>>> An: [hidden email]
>>>>> Betreff: eor bucket
>>>>>
>>>>> Could someone help me to check my understanding of the eor bucket
>>>>> implementation?
>>>>>
>>>>> If an eor bucket is ever copied, there are 2 buckets with their b->data
>>>>> pointing to the request_rec. Since this is local to the bucket,
>>>>> destroying these 2 will call apr_pool_destroy() twice on the pool.
>>>>> Correct?
>>>>>
>>>>> -Stefan

Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Yann Ylavic
On Thu, May 9, 2019 at 11:37 AM Stefan Eissing
<[hidden email]> wrote:
>
> > Am 09.05.2019 um 11:32 schrieb Yann Ylavic <[hidden email]>:
> >
> > The issue is more that the hooks in eor_bucket_cleanup() will be run
> > multiple times, rather than a lifetime issue.
>
> I read it like this:
> The cleanup is only registered on the first creation. The copy never registers. But the bucket_destroy calls the pool destroy each time.

Ah yes, apr_bucket_copy() (i.e. apr_bucket_simple_copy()) won't do the
right thing for eor, there should be an eor_bucket_copy() to register
the cleanup for the copy too.

We should also possibly split the eor cleanup in two, one (registered
first) to run the hooks, and the second for NULLing b->data.
Both (but mainly the first one) would do nothing is b->data is already
NULL. That way I think the eor can be safely copied.
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Yann Ylavic
On Thu, May 9, 2019 at 11:54 AM Yann Ylavic <[hidden email]> wrote:

>
> On Thu, May 9, 2019 at 11:37 AM Stefan Eissing
> <[hidden email]> wrote:
> >
> > > Am 09.05.2019 um 11:32 schrieb Yann Ylavic <[hidden email]>:
> > >
> > > The issue is more that the hooks in eor_bucket_cleanup() will be run
> > > multiple times, rather than a lifetime issue.
> >
> > I read it like this:
> > The cleanup is only registered on the first creation. The copy never registers. But the bucket_destroy calls the pool destroy each time.
>
> Ah yes, apr_bucket_copy() (i.e. apr_bucket_simple_copy()) won't do the
> right thing for eor, there should be an eor_bucket_copy() to register
> the cleanup for the copy too.
>
> We should also possibly split the eor cleanup in two, one (registered
> first) to run the hooks, and the second for NULLing b->data.
> Both (but mainly the first one) would do nothing is b->data is already
> NULL. That way I think the eor can be safely copied.

Or, use the EOR bucket code from trunk...
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Yann Ylavic
On Thu, May 9, 2019 at 12:00 PM Yann Ylavic <[hidden email]> wrote:

>
> On Thu, May 9, 2019 at 11:54 AM Yann Ylavic <[hidden email]> wrote:
> >
> > On Thu, May 9, 2019 at 11:37 AM Stefan Eissing
> > <[hidden email]> wrote:
> > >
> > > > Am 09.05.2019 um 11:32 schrieb Yann Ylavic <[hidden email]>:
> > > >
> > > > The issue is more that the hooks in eor_bucket_cleanup() will be run
> > > > multiple times, rather than a lifetime issue.
> > >
> > > I read it like this:
> > > The cleanup is only registered on the first creation. The copy never registers. But the bucket_destroy calls the pool destroy each time.
> >
> > Ah yes, apr_bucket_copy() (i.e. apr_bucket_simple_copy()) won't do the
> > right thing for eor, there should be an eor_bucket_copy() to register
> > the cleanup for the copy too.
> >
> > We should also possibly split the eor cleanup in two, one (registered
> > first) to run the hooks, and the second for NULLing b->data.
> > Both (but mainly the first one) would do nothing is b->data is already
> > NULL. That way I think the eor can be safely copied.
>
> Or, use the EOR bucket code from trunk...

That is, including r1707084 and follow ups (r1707093, r1707159 and r1707362).
Reply | Threaded
Open this post in threaded view
|

AW: eor bucket

Plüm, Rüdiger, Vodafone Group
In reply to this post by Yann Ylavic



C2 General

> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic <[hidden email]>
> Gesendet: Donnerstag, 9. Mai 2019 11:54
> An: httpd-dev <[hidden email]>
> Betreff: Re: eor bucket
>
> On Thu, May 9, 2019 at 11:37 AM Stefan Eissing
> <[hidden email]> wrote:
> >
> > > Am 09.05.2019 um 11:32 schrieb Yann Ylavic <[hidden email]>:
> > >
> > > The issue is more that the hooks in eor_bucket_cleanup() will be run
> > > multiple times, rather than a lifetime issue.
> >
> > I read it like this:
> > The cleanup is only registered on the first creation. The copy never
> registers. But the bucket_destroy calls the pool destroy each time.
>
> Ah yes, apr_bucket_copy() (i.e. apr_bucket_simple_copy()) won't do the
> right thing for eor, there should be an eor_bucket_copy() to register
> the cleanup for the copy too.
>
> We should also possibly split the eor cleanup in two, one (registered
> first) to run the hooks, and the second for NULLing b->data.
> Both (but mainly the first one) would do nothing is b->data is already
> NULL. That way I think the eor can be safely copied.

I think we should follow a similar design pattern as we do with MMAP/file buckets which use a refcount.

Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

AW: eor bucket

Plüm, Rüdiger, Vodafone Group
In reply to this post by Yann Ylavic



C2 General

> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic <[hidden email]>
> Gesendet: Donnerstag, 9. Mai 2019 12:00
> An: httpd-dev <[hidden email]>
> Betreff: Re: eor bucket
>
> On Thu, May 9, 2019 at 11:54 AM Yann Ylavic <[hidden email]>
> wrote:
> >
> > On Thu, May 9, 2019 at 11:37 AM Stefan Eissing
> > <[hidden email]> wrote:
> > >
> > > > Am 09.05.2019 um 11:32 schrieb Yann Ylavic <[hidden email]>:
> > > >
> > > > The issue is more that the hooks in eor_bucket_cleanup() will be
> run
> > > > multiple times, rather than a lifetime issue.
> > >
> > > I read it like this:
> > > The cleanup is only registered on the first creation. The copy never
> registers. But the bucket_destroy calls the pool destroy each time.
> >
> > Ah yes, apr_bucket_copy() (i.e. apr_bucket_simple_copy()) won't do the
> > right thing for eor, there should be an eor_bucket_copy() to register
> > the cleanup for the copy too.
> >
> > We should also possibly split the eor cleanup in two, one (registered
> > first) to run the hooks, and the second for NULLing b->data.
> > Both (but mainly the first one) would do nothing is b->data is already
> > NULL. That way I think the eor can be safely copied.
>
> Or, use the EOR bucket code from trunk...

Which is the MMAP/file pattern approach as I just noticed :-) Thanks for the pointer.

Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: eor bucket

Stefan Eissing
In reply to this post by Yann Ylavic
Thanks, Yann. Just proposed these for backport.

> Am 09.05.2019 um 12:04 schrieb Yann Ylavic <[hidden email]>:
>
> r1707362