I'm fine going ahead with Gordon's fix. I don't have a lot of time to dig into the refcounting issue personally right now, but I'd at least leave the bug open until we have made it through a bit more testing. I have an uneasy feeling it (or something closely related) may pop up again if we push harder on testing.
--Rafael On Mon, Jul 20, 2015 at 10:43 AM, Ken Giusti <[email protected]> wrote: > +1 for using Gordon's fix for now - we can cut a beta and see if it holds > up. > > Since there's some unanswered questions regarding the patch's behavior, > I'll leave the bug open - drop the blocker status - and assign it over to > Rafi. He's better cerebrally equipped to understand the reference counting > implementation than I certainly am. > > > > ----- Original Message ----- > > From: "Robbie Gemmell" <[email protected]> > > To: [email protected], [email protected] > > Sent: Monday, July 20, 2015 12:03:06 PM > > Subject: Re: proton 0.10 blocker > > > > On 17 July 2015 at 23:32, Gordon Sim <[email protected]> wrote: > > > On 07/17/2015 10:04 PM, Rafael Schloming wrote: > > >> > > >> On Fri, Jul 17, 2015 at 12:47 PM, Gordon Sim <[email protected]> wrote: > > >> > > >>> On 07/17/2015 08:15 PM, Rafael Schloming wrote: > > >>> > > >>>> On Fri, Jul 17, 2015 at 11:56 AM, Gordon Sim <[email protected]> > wrote: > > >>>> > > >>>> On 07/17/2015 07:17 PM, Rafael Schloming wrote: > > >>>>> > > >>>>> > > >>>>> Given this I believe the incref/decref pair is indeed running > into > > >>>>>> > > >>>>>> problems > > >>>>>> when being invoked from inside a finalizer. I'd be curious if an > > >>>>>> alternative fix would work. I suspect you could replace the > additional > > >>>>>> conditions you added to the if predicate with this: > > >>>>>> > > >>>>>> pn_refcount(endpoint) > 0 > > >>>>>> > > >>>>>> > > >>>>> If the refcount is *not* 0, what does the incref/decref sequence > > >>>>> accomplish? > > >>>>> > > >>>> > > >>>> > > >>>> I believe the answer to this is the same as the answer I just > posted on > > >>>> the > > >>>> other thread, i.e. the incref may trigger the incref hook (see > > >>>> pn_xxx_incref in engine.c), and this in turn may update the endpoint > > >>>> state > > >>>> and adjust the refcount accordingly. The decref may then end up > > >>>> finalizing > > >>>> the object. > > >>>> > > >>> > > >>> Right, understood now. > > >>> > > >>> Unfortunately replacing the additional conditions with just that > check on > > >>> the refcount doesn't prevent the crash though. > > >>> > > >> > > >> Doh, not the result I was hoping for. Does it yield the same stack > trace > > >> as > > >> before? > > > > > > > > > Yes > > > > > > > Given that and all who looked thinking the earlier proposal was safe, > > is it worth going with that change at least for now? Knocking things > > off the blocker list and getting an RC (or even just a beta) out would > > be really really good. > > > > Robbie > > > > -- > -K >
