GNOME Bugzilla – Bug 774429
gnome-music leaves temporary directories under /tmp
Last modified: 2018-01-10 14:58:24 UTC
Everytime I close gnome-music there's 5 new grilo-plugin-cache-* directories under /tmp. I can reproduce this with version 3.22.1, grilo 0.3.2 and grilo-plugins 0.3.3, all of them from Debian. I can work around the problem by blacklisting the plugins that are causing this in grilo.py: self.sources = {} self.blacklist = ['grl-filesystem', 'grl-bookmarks', 'grl-metadata-store', 'grl-podcasts'] +self.blacklist.extend(['grl-itunes-podcast', 'grl-musicbrainz-coverart', 'grl-spotify-cover', 'grl-video-title-parsing', 'grl-raitv']) self.tracker = None self.changed_media_ids = [] What I'm not sure if this is a problem in gnome-music itself (because it doesn't do the necessary cleanups on exit) or a grilo bug (see also bug 770486).
Music doesn't properly deinit grilo, this seems to be notoriously hard with python. That might be a reason. We could maybe remove all sources on exit and see if that has any effect. Patch appreciated.
Created attachment 339911 [details] [review] Unregister all sources Here's a quick and dirty patch. It unregisters all sources when the app window is destroyed. It seems to solve the problem for all BUT one of the directories, which seems to be created by 'grl-itunes-podcast'. Interestingly grl-itunes-podcast seems to be unregistered correctly, and if I actually put it in the blacklist then the problem disappears.
Created attachment 339917 [details] [review] Unregister all sources v2 Ah, there was a reference to grl-itunes-podcast left in grilo.sources This patch does the job.
Review of attachment 339917 [details] [review]: A few issues * To me this is a stop-gap solution, proper solution would be to deinit but I doubt that's attainable with python. * I prefer all the cleanup in the same place really, currently we do that in window.py (_notify_mode_disconnect) (rather odd). ::: gnomemusic/grilo.py @@ +366,3 @@ + @log + def cleanup(self): + del self.sources Why not iterate over this? These are our loaded sources. @@ +367,3 @@ + def cleanup(self): + del self.sources + for mediaSource in self.registry.get_sources(False): The false is superfluous. @@ +370,3 @@ + try: + self.registry.unregister_source(mediaSource) + except GLib.GError: GLib.Error
Created attachment 340379 [details] [review] Unregister all sources v3 (In reply to Marinus Schraal from comment #4) > * To me this is a stop-gap solution, proper solution would be to > deinit but I doubt that's attainable with python. Not sure what's the best way to do this in Python, I just went for the simplest one that I could think of. > * I prefer all the cleanup in the same place really, currently we do > that in window.py (_notify_mode_disconnect) (rather odd). Ok, I hadn't seen that one. > ::: gnomemusic/grilo.py > @@ +366,3 @@ > + @log > + def cleanup(self): > + del self.sources > > Why not iterate over this? These are our loaded sources. No, because not all sources are put in that dictionary. This new patch takes care of removing the unused ones first so we can do as you suggest.
(In reply to Alberto Garcia from comment #5) > No, because not all sources are put in that dictionary. This new patch > takes care of removing the unused ones first so we can do as you > suggest. If they aren't in that dict then we don't use them and we shouldn't keep them around in the first place. There is normally no need to del anything in python and I see no reason to do it here.
(In reply to Marinus Schraal from comment #6) > > No, because not all sources are put in that dictionary. This new patch > > takes care of removing the unused ones first so we can do as you > > suggest. > If they aren't in that dict then we don't use them and we shouldn't > keep them around in the first place. grilo.py loads all plugins (except for the blacklisted ones), but not all of them are added to the 'sources' dict. The ones that are not added to that dict are still registered by Grilo, even if gnome-music does not use them. If you don't unregister them they'll keep their temporary directories. And that's why my new patch unregisters them in _on_source_added(). > There is normally no need to del anything in python and I see no > reason to do it here. There's two things that this patch does: 1) Make sure that all sources are unregistered. This is now done in cleanup() for the ones that gnome-music uses, and in _on_source_added() for the ones that gnome-music does not use, as explained above. 2) Remove all references to the sources. This is why I'm deleting the dictionary. If I don't do that, not all temporary directories are cleaned. You can verify that very easily yourself by commenting out that line.
Review of attachment 340379 [details] [review]: ::: gnomemusic/grilo.py @@ +225,3 @@ + else: + pluginRegistry.unregister_source(mediaSource) I would add a debug message here, to track ignored sources. @@ +375,3 @@ + except GLib.Error: + logger.error("Failed to unregister %s.", mediaSource.get_id()) + del self.sources Looks okay. grl_deinit() does not work in the same way? If not, that might be a bug in Grilo.
Created attachment 340429 [details] [review] Patch using grl_deinit() (In reply to Victor Toso from comment #8) > Looks okay. grl_deinit() does not work in the same way? If not, that > might be a bug in Grilo. You're actually right, I didn't know about that function. Here's a simpler patch that uses grl_deinit(). We still need to clear the sources dict, though, I understand that it prevents all refs from being released and that's why some sources are not destroyed.
(In reply to Alberto Garcia from comment #9) > Here's a simpler patch that uses grl_deinit(). We still need to clear > the sources dict, though, I understand that it prevents all refs from > being released and that's why some sources are not destroyed. In my earlier test deinit could result in crashes, due to how python handles teardown. I'm still not convinced any of this (except deinit) should be done in music really.
I'm far from being a Python or a Grilo expert, but if there's a better way to prevent Music from filling /tmp/ with leaked directories, I'm all ears.
Review of attachment 340429 [details] [review]: I would keep the debug + unregister of sources that we don't use. In a different patch even would be great :) The reason for me is to not keep o memory stuff that will not be used. From grilo perspective, this looks okay.
Review of attachment 340429 [details] [review]: Apologies for the delay. This is the correct solution and one I looked into a while ago, however this may segfault as there is no set order for python object tear down. I used to be able to reproduce this with loading album art through grilo and closing the app while it was in progress. I think we need cancellation support in the albumart module, but I'm not even sure that would always solve the issue.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-music/issues/82.