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

[Neil Williams]

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?

> 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?

>    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.)

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? 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.

-- 

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20060210/cd74082c/attachment.bin