After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 733927 - Dap and MtpSources Should Respect the Disposable pattern
Dap and MtpSources Should Respect the Disposable pattern
Status: RESOLVED WONTFIX
Product: banshee
Classification: Other
Component: Device (general)
git master
Other Linux
: Normal normal
: ---
Assigned To: Banshee Maintainers
Banshee Maintainers
gnome[unmaintained]
Depends on: 666696
Blocks:
 
 
Reported: 2014-07-29 15:57 UTC by Nicholas Little
Modified: 2020-03-17 09:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Respect Disposable Pattern V1 (2.34 KB, patch)
2014-07-29 16:01 UTC, Nicholas Little
none Details | Review
Ensure DapService disposes of DapSources (2.25 KB, patch)
2014-07-29 23:43 UTC, Nicholas Little
none Details | Review

Description Nicholas Little 2014-07-29 15:57:08 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.
Comment 1 Nicholas Little 2014-07-29 16:01:31 UTC
Created attachment 281966 [details] [review]
Respect Disposable Pattern V1
Comment 2 Andrés G. Aragoneses (IRC: knocte) 2014-07-29 19:05:53 UTC
@@ -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.)
Comment 3 Nicholas Little 2014-07-29 20:05:58 UTC
(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.
Comment 4 Nicholas Little 2014-07-29 23:43:53 UTC
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)
Comment 5 Andrés G. Aragoneses (IRC: knocte) 2015-03-08 13:55:17 UTC
(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?
Comment 6 Nicholas Little 2015-03-08 16:54:28 UTC
(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.
Comment 7 Andrés G. Aragoneses (IRC: knocte) 2015-03-08 20:29:32 UTC
(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?
Comment 8 Nicholas Little 2015-03-08 21:54:55 UTC
(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?
Comment 9 Andrés G. Aragoneses (IRC: knocte) 2015-03-08 22:29:21 UTC
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?
Comment 10 Andrés G. Aragoneses (IRC: knocte) 2015-03-08 22:29:45 UTC
I meant comment#6.
Comment 11 Nicholas Little 2015-03-09 19:14:14 UTC
(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?
Comment 12 Andrés G. Aragoneses (IRC: knocte) 2015-03-09 19:21:17 UTC
(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.
Comment 13 André Klapper 2020-03-17 09:53:05 UTC
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.