GNOME Bugzilla – Bug 733927
Dap and MtpSources Should Respect the Disposable pattern
Last modified: 2020-03-17 09:53:05 UTC
Currently, DapSource does not call it's superclass's Dispose method. It should definitely do this. In addition, DapSource's Dispose method calls Flush, in the case where a device is unplugged this is harmful (as the user may not have caused a Flush to occur during the session) and the source of an entirely unnecessary error. Furthermore, in an effort to ensure a Flush, MtpSource attempts to 'unpeel the onion from the inside out' during its Dispose implementation which is obviously a mistake. A final consequence of the break in the Disposable chain is that MtpSource also requires a call to ServiceManager.SourceManager.RemoveSource in its Dispose implementation.
Created attachment 281966 [details] [review] Respect Disposable Pattern V1
@@ -48,10 +48,6 @@ namespace Banshee.Dap.Mtp { public class MtpSource : DapSource { - - protected override object InternalLock { - get { return mtp_device; } - } private MtpDevice mtp_device; //private bool supports_jpegs = false; Why the change above? >- Flush (); >removes the error prone Flush from Dispose; Why is it error prone? I have the feeling that if you remove the call to Flush() here, you will cause bug 666696 again. (I don't see the Dispose() method of base clases calling Flush, which is necessary to not cause this bug.)
(In reply to comment #2) > @@ -48,10 +48,6 @@ namespace Banshee.Dap.Mtp > { > public class MtpSource : DapSource > { > - > - protected override object InternalLock { > - get { return mtp_device; } > - } > private MtpDevice mtp_device; > > //private bool supports_jpegs = false; > > Why the change above? That property shouldn't really be virtual, though I stopped myself from adding that change to the patch in the interests of keeping it small. It shouldn't be virtual because DapSource expects to be able to lock on it, but if it's a device lock (using the actual device, as in MTP) then when DapSource. Dispose runs it's set to null. > >- Flush (); > >removes the error prone Flush from Dispose; > > Why is it error prone? I have the feeling that if you remove the call to > Flush() here, you will cause bug 666696 again. (I don't see the Dispose() > method of base clases calling Flush, which is necessary to not cause this bug.) Sorry, I meant it's error prone to call Flush during Dispose when we're not certain what state the device is in. Good call, I had concentrated on the patches you'd produced and neglected the actual issue. I'll follow up with a patch that addresses both.
Created attachment 281984 [details] [review] Ensure DapService disposes of DapSources (In reply to comment #2) <snip /> > Why is it error prone? I have the feeling that if you remove the call to > Flush() here, you will cause bug 666696 again. (I don't see the Dispose() > method of base clases calling Flush, which is necessary to not cause this bug.) ok, looks like 666696 is in fact occurring again anyway. Closing Banshee doesn't cause my MTP playlists to sync to my device... The commit message for this one is quite in depth, in short we're causing a live lock which is preventing DapService.UnmapDevice to succeed (in all cases)
(In reply to Nicholas Little from comment #4) > ok, looks like bug 666696 is in fact occurring again anyway. Closing Banshee > doesn't cause my MTP playlists to sync to my device... Do you mean, that this last patch still makes bug 666696 regress, or not?
(In reply to Andrés G. Aragoneses (IRC: knocte) from comment #5) > (In reply to Nicholas Little from comment #4) > > ok, looks like bug 666696 is in fact occurring again anyway. Closing Banshee > > doesn't cause my MTP playlists to sync to my device... > > Do you mean, that this last patch still makes bug 666696 regress, or not? No, last time I tested it had already regressed.
(In reply to Nicholas Little from comment #6) > (In reply to Andrés G. Aragoneses (IRC: knocte) from comment #5) > > (In reply to Nicholas Little from comment #4) > > > ok, looks like bug 666696 is in fact occurring again anyway. Closing Banshee > > > doesn't cause my MTP playlists to sync to my device... > > > > Do you mean, that this last patch still makes bug 666696 regress, or not? > > No, last time I tested it had already regressed. So, I should mark this patch with the needs-work flag?
(In reply to Andrés G. Aragoneses (IRC: knocte) from comment #7) > (In reply to Nicholas Little from comment #6) > > (In reply to Andrés G. Aragoneses (IRC: knocte) from comment #5) > > > (In reply to Nicholas Little from comment #4) > > > > ok, looks like bug 666696 is in fact occurring again anyway. Closing Banshee > > > > doesn't cause my MTP playlists to sync to my device... > > > > > > Do you mean, that this last patch still makes bug 666696 regress, or not? > > > > No, last time I tested it had already regressed. > > So, I should mark this patch with the needs-work flag? What makes you say that?
Sorry, I'm having trouble understanding your comments, somehow I always find ambiguity in them. Does your comment#7 mean that we should reopen bug 666696 already?
I meant comment#6.
(In reply to Andrés G. Aragoneses (IRC: knocte) from comment #9) > Sorry, I'm having trouble understanding your comments, somehow I always find > ambiguity in them. Does your comment#7 mean that we should reopen bug 666696 > already? Yep, bug 666696 has regressed so we should consider re-opening it - however I think the fix should take a slightly different form. Unfortunately this patch doesn't correct any bugs in banshee which we could point at and say 'this fixed that'. It's simply about correcting our use of IDisposable. This patch is good if you agree with the statement 'it's error prone to call Flush during Dispose when we're not certain what state the device is in', otherwise I need to understand why it's a good idea to try writing to a device which we aren't certain is plugged in?
(In reply to Nicholas Little from comment #11) > (In reply to Andrés G. Aragoneses (IRC: knocte) from comment #9) > > Sorry, I'm having trouble understanding your comments, somehow I always find > > ambiguity in them. Does your comment#7 mean that we should reopen bug 666696 > > already? > > Yep, bug 666696 has regressed so we should consider re-opening it Ok, feel free to do it. > however I think the fix should take a slightly different form. That's fine, whenever you have time please have a look! > Unfortunately this patch doesn't correct any bugs in banshee which we could > point at and say 'this fixed that'. It's simply about correcting our use of > IDisposable. No problem. But with the desire of not making bug 666696 more difficult to fix, I would like that we re-fix it before fixing this one. So I'm going to mark it as a dependency.
Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the responsibility for active development again. See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.