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 _______________________________________________ gnucash-devel mailing list [email protected] https://lists.gnucash.org/mailman/listinfo/gnucash-devel
