GNOME Bugzilla – Bug 313833
Monitoring code doesn't reload menu completely
Last modified: 2005-10-03 14:26:18 UTC
Distribution/Version: Mandriva cooker Using latest CVS snapshot (with all the fixes on entry-directories.c), I'm seeing incomplete menu reload when menu are changed on disk. To test, use the attached testcase the following way : -kill all instance of gnome-panel -tar xf xdg1-testcase.tar.bz2 -run gnome-panel -load menu (it will probably look strange since you might be missing icons and applications) -rm -rf ~/.local/share/applications/* .config/menus/* && tar xf xdg1-testcase.tar.bz2 -open menu : you will notice it lacks some topdir. Doing this testcase many time in a same instance of gnome-panel will result in various output of menu. Menu reload after removal got broken with the change in entry-directories.c (it was not a problem before those changes). -
Created attachment 50910 [details] huge testcase
Note to anyone who's username is not "a" - you need to change the path in the <AppDir> and <DirectoryDir> in .config/menus/applications.menu
Okay, I can't really do anything useful with such a huge test case - if you could narrow it down to something more manageable, that would be good. Also, we shouldn't rule out the possibility of gamin being someway at fault here - e.g. make sure we're getting the notifications we should be getting etc.
ok, after a lot of analysis, I think I figured what are the exact problems : -first, it is not a gamin bug nor a inotify bug. I've been able to reproduce this bug using fam instead of gamin, as well, as using gamin in dnotify or inotify mode. Bug is harder to trigger with gamin/dnotify because of boost mode protection in gamin but it can still be triggered (more on this below). -first, in menu-monitor.c, queue_fam_event is trying to "optimize" events for each path, to only deliver the last received event. While it can be ok when several events of the same type (Changed, for instance) are received, it is problematic if, for instanced, Created and Changed are received in this order, for a directory. In this case, Changed event will drop Created event, and since handle_cached_dir_changed doesn't do anything on Changed event for directory, the new directory won't appear at all in the menus. I tried to fix this bug by only "optimizing" (ie dropping) old event of the same type (only Changed events can drop previous changed event, etc..), but unfortunately, it uncovered another bug in the monitoring code, which is way harder to fix (I'm not sure how to fix it cleanly) : current menu monitor code doesn't handle burst of events for a specific path correctly, because events are queue for each monitors and processed later. Here is an example for this problem : we have menu like this : <menu_path>/applications/A/B/C.desktop if we delete applications/* and untar the same hierachie after deletion, we get the following events : Delete C.desktop Delete B Delete A Create A Create B Changed A Create C.desktop Changed B received events will end in separate queues, one for each elements queue A : Delete A, Create A, Changed A queue B : Delete B, Create B, Changed B queue C.desktop : Delete C.desktop, Create C.desktop then, in emit_event_in_idle, events are regrouped, in an order which can be random since it is based on the order in menu_registry hash table, but events for a monitor are kept in order. So, you can end with Delete A, Create A, Changed A, Delete B, Create B, Changed B, Delete C.desktop, Create C.desktop or Delete B, Create B, Changed B, Delete A, Create A, Changed A, Delete C.desktop, Create C.desktop or other combinations. If we end in the first set, when 'Delete A' event is processed, menu monitor B and C are scheduled to be removed (because they are no longer relevant to the menu tree). Unfortunately, since Create B and Create C.desktop event are already in their associated monitors which are going to be removed, processing these events will add items to part of the tree which are not going to be displayed on the tree. Looking at this simple example can look a little absurd, but when untaring a big tree with a lot of events being generated very quickly (this is why inotify is triggering the bug more easily than dnotify with gamin), menu monitor can accumulate creation events for item they should not monitor anymore. I'm not sure how to fix this cleanly. We could store pending events based on their path (and not on the target monitor) and only search for relevant monitor when processing events. Or keep menu monitor lifetime even after a menu is no longer monitored at all but if event are still pending for this menu. We might also want to try to process events in the exact order they were received.
Created attachment 52022 [details] [review] patch to not replace Created event with Changed event
Created attachment 52124 [details] [review] fix menu monitoring issue the attached patch fixes completely the problem. I've dropped queues for each menu monitoring, using a single queue for the entire application (so there is no longer any problem with event ordering) and fam event are using string as parameter for fam event.
Nice analysis, thanks I think your last attachment doesn't contain all your changes, though, right?
Created attachment 52854 [details] [review] menu-monitor-event-ordering.patch Committed this to HEAD
2005-09-30 Mark McLoughlin <mark@skynet.ie> Hopefully fix issue where menus wouldn't be completely reloaded after a spew of file change events (#313833) Detailed analysis from Frederic Crozat <fcrozat@mandriva.com> * libmenu/menu-monitor.c: make the pending_events list global rather than per-monitor and don't try and coalesce events for the same path so that we can guarantee that we emit all the events in the same order as we receive them from FAM.
Created attachment 52975 [details] [review] correct patch this time Arg, I attached the wrong patch