[Gnucash-changes] r13152 - gnucash/trunk/lib/libqof/qof - Don't allow the event handler list to shrink while we're traversing it.

[Chris Shoemaker]

On Fri, Feb 10, 2006 at 09:24:48PM +0000, Neil Williams wrote:
> On Wednesday 08 February 2006 6:04 am
> > Date: 2006-02-08 01:04:51 -0500 (Wed, 08 Feb 2006)
> > New Revision: 13152
> > Trac: http://svn.gnucash.org/trac/changeset/13152
> 
> I've been wondering about this ever since it went in, I'm not sure if it's the 
> best solution. It seems a little too simplistic.
> 
> Is there a test case I can use? Was the crash described below part of a bug 
> report? How can I reproduce it?

There's no bug report - I hit the bug myself.  To reproduce it you can
revert the fix (locally, of course) and just keep testing until you
get unlucky, or you can write a test case that adds two handlers and
then tries to unregister the second from within the first.  I think I
was closing the main account-tree when I hit it.

> 
> > Modified:
> >    gnucash/trunk/lib/libqof/qof/gnc-event.c
> > Log:
> >    Don't allow the event handler list to shrink while we're traversing it.
> 
> What about making this stage conditional? (It would at least limit the 
> inexorable growth of the list.)
> 
> > +    /* Normally, we could actually remove the handler's node from the
> > +       list, but we may be unregistering the event handler as a result
> > +       of a generated event, such as GNC_EVENT_DESTROY.  In that case,
> > +       we're in the middle of walking the GList and it is wrong to
> > +       modify the list. So, instead, we just NULL the handler. */
> 
> If GNCEngineEventType != GNC_EVENT_DESTROY, remove the handler node?
> 
> >    This change isn't ideal in the sense that the handler list is now a
> >    monotonically increasing resource,
> 
> This worries me - consider if the events are used in a situation where 
> resources are far more limited than in a desktop computer. How big does this 
> list now become if the process is in use continually?

equal to the number of callbacks registered during the life of the program.

> >    but it's better than crashing when 
> > the handler in node N+1 happens to be deleted while servicing the handler
> > in node N.
> 
> Isn't there some way we can test for N+1 != NULL?
> (we probably should be doing that anyway.)

No.  There's no safe way to generally modify a GList while you walk it
- no matter how many checks you make.

> Perhaps we need a hypothetical function on the lines of: static void
> qofevent_resort_list(void) that is called internally before events
> are resumed (if suspend_counter == 0 after being decremented) or at
> some other point when it would be safe to do so?

There's no place in the code right now where this is safe to do.  You
would need a recursion counter, and then you would know when it was
safe.

> It could iterate over the list, creating a new list that had no gaps, 
> before using that to replace the original. I'd rather not have to iterate so 
> maybe instead of setting the handler to NULL, we can store it somewhere and 
> remove specific handlers when it is safe to do so? (at the end of the loop?)
> 
> This isn't an API issue, it's a technical issue about how best to prevent this 
> crash without causing another elsewhere by consuming resources on a much more 
> limited platform.

Personally, I think the long-term solution to this problem is GHook.

-chris