GNOME Bugzilla – Bug 666981
[LibraryWatcher] Banshee crashes when importing a CD
Last modified: 2012-02-22 20:16:23 UTC
Created attachment 204313 [details] Log file of the execution. When I try to import a CD, Banshee crashes. Apparently the cause of this is that Banshee creates a new empty file for the first track, and then tries to import it to the collection. The file wich is being imported is empty, so it causes an exception and Banshee exits.
Thank you for your bug report. The log files show that the Library Watcher extension is enabled. It sometimes causes problems when importing tracks. In your case, it maybe tries to import the track before it is actually converted from the CD. Could you disable it and try to reproduce the problem ?
Of course, disabling the Library Watcher it works correctly. Thank you very much.
*** Bug 667019 has been marked as a duplicate of this bug. ***
Created attachment 206747 [details] stdout when fault occurs - different environment, same fault. Errors shown in English, and only for faulting thread. Banshee 2.3.4 running on Ubuntu 11.10
I've been running into same fault in recent weeks - see the attached log. It's very annoying. If a track file is empty, just ignore it!
Apparently this bug has been exposed by a new feature in 2.3.x development series. If you wanted more stability, you should have sticked maybe with 2.2.x stable series. If you didn't, thanks for testing. So currently you can either disable the LibraryWatcher or stick with Banshee 2.2.x, or wait until we fix it (hopefully I'll have some time to look at it in the next weeks, but help welcome!). Confirming due to having more than 1 person reproducing it.
I've just committed a fix so that we don't crash when there's an exception while the LibraryWatcher processes tracks : http://git.gnome.org/browse/banshee/commit/?id=7f70949db This doesn't fix the root of the problem (the interaction between importing a CD and the LibraryWatcher), so I'm leaving this bug open.
Thanks Bertrand! I have a fix in the works (haven't tested it yet). Hopefully I'll have time to finish it before the 2.3.6 release.
Created attachment 208009 [details] [review] proposed patch As described in the commit message, this patch is likely to fix bug 655752 as well.
Review of attachment 208009 [details] [review]: Thanks for the patch ! I think its a good approach, but I'm wondering if 10 seconds is enough (we might be waiting for a big file to be transcoded). But with a longer delay the user could get impatient and wonder why his files are not showing up... Maybe the SourceWatcher could figure out that some import operation is ongoing and suspend itself until it's finished ? Especially since in the case of importing a CD, the Watcher doesn't need to do anything, the files will be imported anyway. Not sure if that's a reasonable or feasible idea... ::: src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/SourceWatcher.cs @@ +100,3 @@ watcher = new FileSystemWatcher (path); watcher.IncludeSubdirectories = true; + watcher.Changed += OnModification; I agree on the name change to reduce confusion, but event handlers should be named with a verb: "OnModified" Blame the "Framework Design Guidelines" book ;) @@ +151,3 @@ + var timer = CreateTimer (fullpath); + lock (created_items_lock) { + timer.Elapsed += (sender, e) => TimeUpForChangedEvent (fullpath); Is it necessary to use a specific object for the lock ? I think it would be simpler to just use created_items_bag for that, as you've done in TimeUpForChangedEvent (probably by mistake). @@ +163,3 @@ + timer.Stop (); + timer.Start (); + var timer = CreateTimer (fullpath); That return statement is not necessary. @@ -187,3 @@ - change_types |= item.ChangeType; - } catch (Exception e) { - RemoveTrack (item.FullPath); Is it a good idea to remove the try/catch clause ? I added it so that a problem when importing or updating a track wouldn't crash the whole app. Better for a change to be ignored than to crash...
Created attachment 208078 [details] [review] proposed patch, v2 (In reply to comment #10) > Review of attachment 208009 [details] [review]: > > Thanks for the patch ! Thanks for the prompt review! > I think its a good approach, but I'm wondering if 10 seconds is enough (we > might be waiting for a big file to be transcoded). But with a longer delay the > user could get impatient and wonder why his files are not showing up... If you ask me this, I'm not sure you understood completely the approach :) 10 seconds would be the maximum to consider between the changed events. I've tested this and I get normally around 10 events per second, so I'm already being quite generous. Take in account that every time a Changed event happens, the timer gets restarted (it starts to count from 0 again) so literally reaching 10 seconds between a Changed event and the next one is almost impossible, and still a reasonable delay for the user to see a new file appearing on their library after a change happens. > Maybe the SourceWatcher could figure out that some import operation is ongoing > and suspend itself until it's finished ? Especially since in the case of > importing a CD, the Watcher doesn't need to do anything, the files will be > imported anyway. Not sure if that's a reasonable or feasible idea... I guess we can discard this idea then? > ::: > src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/SourceWatcher.cs > @@ +100,3 @@ > watcher = new FileSystemWatcher (path); > watcher.IncludeSubdirectories = true; > + watcher.Changed += OnModification; > > I agree on the name change to reduce confusion, but event handlers should be > named with a verb: "OnModified" > Blame the "Framework Design Guidelines" book ;) Good catch. I've actually thought about it and there's no point in mixing these changes, so I've already pushed to master this renaming. The patch I'm posting now is based on this (after this commit). > > @@ +151,3 @@ > + var timer = CreateTimer (fullpath); > + lock (created_items_lock) { > + timer.Elapsed += (sender, e) => TimeUpForChangedEvent (fullpath); > > Is it necessary to use a specific object for the lock ? > I think it would be simpler to just use created_items_bag for that, as you've > done in TimeUpForChangedEvent (probably by mistake). Oh, yes, I didn't mean to use it in TimeUpForChangedEvent hehe. Well, it's always a good practice to use a different object (because it's always safe). But in the case that the collection is not public/internal it's not really necessary, so I've changed it to use the collection instead. > @@ +163,3 @@ > + timer.Stop (); > + timer.Start (); > + var timer = CreateTimer (fullpath); > > That return statement is not necessary. Removed. > > @@ -187,3 @@ > - change_types |= item.ChangeType; > - } catch (Exception e) { > - RemoveTrack (item.FullPath); > > Is it a good idea to remove the try/catch clause ? > I added it so that a problem when importing or updating a track wouldn't crash > the whole app. Better for a change to be ignored than to crash... Mmm, I guess it's fine to leave it there. You know, as our "Improva Banshee via reporting anonymous statistical usage" feature doesn't catch yet every uncaught exception (would be nice to have no?) I'm always kind of reluctant to hide unexpected stuff in logs. If we had caught this exception in the past, users would not have got a crash and would not have this bug report ;)
s/Improva/Improve s/not have this bug report/not have *sent* this bug report/
(In reply to comment #11) > (In reply to comment #10) > > Review of attachment 208009 [details] [review] [details]: > > > > Thanks for the patch ! > > Thanks for the prompt review! > > > I think its a good approach, but I'm wondering if 10 seconds is enough (we > > might be waiting for a big file to be transcoded). But with a longer delay the > > user could get impatient and wonder why his files are not showing up... > > If you ask me this, I'm not sure you understood completely the approach :) > > 10 seconds would be the maximum to consider between the changed events. I've > tested this and I get normally around 10 events per second, so I'm already > being quite generous. Take in account that every time a Changed event happens, > the timer gets restarted (it starts to count from 0 again) so literally > reaching 10 seconds between a Changed event and the next one is almost > impossible, and still a reasonable delay for the user to see a new file > appearing on their library after a change happens. For some unknown reason I assumed we'd only get an event for the creation, and then a "changed" event when the file was completely transcoded. This is wrong, so forget my remark.
Comment on attachment 208078 [details] [review] proposed patch, v2 I've committed this, with a few minor changes: http://git.gnome.org/browse/banshee/commit/?id=cf2346be3
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.