ATF safety

[Martin Aumueller]

On Thu August 17 2006 14:44, Jeff Mitchell wrote:
[...]
> > - and please try to make the code easier to understand more safe
>
> ?  I don't think the code is that hard to understand.  The process in
> collectiondb for instance is more well documented than the much of the code
> in amarok (see playlist.cpp, for instance), and the stuff in MetaBundle is
> fairly straighforward.  It's the algorithms that are hard to understand. 
> And we're always trying to make Amarok code more safe, aren't we...don't
> think I haven't seen all your commits with the DeepCopys :-)
>
> >   * we should properly lock files that any Amarok process writes to
> > (imagine a writable collection shared by several users - this is not only
> > ATF related)
>
> You can't do this, really.  Scott and I had a very long discussion about
> it. The problem is that flock and fcntl either don't work at all or are
> very broken with files on network file systems, including NFS and Samba --
> and many, many people fall into this realm.  So you have to get around
> this.  You can either try per-file lockfiles, which have lots of problems,
> or a global lockfile, which also has problems, and doesn't allow for tag
> edits to be done during a scan.  I've discussed these options at length
> with Ian.

I didn't see anybody try flock or fcntl. Both of these mechanisms were created to handle cases as ours. So I think it's worth trying before resorting to other mechanisms that involves changes in many places.
>
> If you've seen my recent commits, you'll see I'm taking another approach --
> adding the ability to a) find the instance of a running ScanController, and
> b) pause the collectionscanner.  So if you want to do tag editing, instead
> of calling save() on the TagLib::File, you pass it into a function which
> checks to see if a scanner is running.  If so, you pause it, *wait for
> confirmation*, do the write, then unpause it.  If confirmation never comes,
> you abort the tag writing.  In addition, make sure when
> amarokcollectionscanner is starting that no other processes are still
> existing/running -- if so kill them, and if you can't (owned by another
> user, or something else) then abort the scany.  Crashing amarokapp with a
> running scanner, then starting again was one reason the earlier problem
> could occur.
>
> It is true that this won't handle the case of a networked filesystem with
> multiple users all scanning at the same time, all with ATF turned on. 
> There really isn't a good way to handle this that I've seen, and it's not
> even limited to ATF: you can run into the same problem if you and someone
> else both try to update the tags of a file at the same time, even with ATF
> off. Again, it's just a casle where ATF makes it more likely because it's
> dealing with so many files that first scan.  So perhaps putting a warning
> in the Information dialog about this case in the big red PLEASE NOTE
> section would be good, telling users to scan from a single computer first
> (and then possibly make the collection read-only for most users -- this is
> a good idea even without ATF), or use a single computer's database, or etc.
>
> > * do not make the behaviour of MetaBundle dependent on a
> > process name (this is not very transparent to the caller, and some day,
> > the process names might change and ATF will stop working)
>
> And you suggest what, instead?

Please see below.
[...]
> >   * limit the locations where meta data writes are triggered to as few
> > places as possible and make it more clear when this happens
>
> It already only happens in a few places.  We just need to update all the
> file->save() commands to use the new function.
>
> >   * make the ATF related parts of the MetaBundle API behave just as the
> > rest of MetaBundle - Amarok developers are already familiar with this
> > pattern: + a setUniqueId(QString) call that sets the unique id *only in
> > memory* + a uniqueId() call for querying the unique id
>
> That's already the current behavior.

There is still this setUniqueId call that does the actual writing. Which is confusing, as this call has the same name as the others, and it behaves differently. But I see, the situation has improved a bit: the other setUniqueId calls don't call it. But the one w/o parameters is also not in line with the general MetaBundle API pattern: all other setters accept a parameter.
>
> >     + write the unique id together with the other tags in
> > MetaBundle::save() + perhaps provide a saveUniqueId() as an optimization
> > for only writing the unique id to the file
>
> Won't be necessary once the new save function is implemented -- everything
> will use that.
>
> >   * with this changed API, the collection scanner could just call
> > bundle->setUniqueId(somerandomid); bundle->saveUniqueId() - no need for
> > special casing MetaBundle for processes having a certain name
>
> No, you can't do that, first because the scanner has to use an ID if it
> already exists, which requires a lot of various logic and taglib stuff, and
> second because there are other times when UIDs can be set (or unset -- you
> have to allow for stripping as well).  These functions are implemented
> outside the scanner, for instance through DCOP.

I still think you can: of course my pseudo code was not complete. Do something like this when scanning a file:
- read the tags, including a unique id if available (but don't yet do anything except for just reading the tags)
- if no unique id is there, then invent a new one and write it to the file

This would mean changing the current code from

MetaBundle bundle( path ); // this is scary, might write to the file if no unique id is there!

to

MetaBundle bundle( path ); // would just read tags
if( bundle.uniqueId.isEmpty() )
{
   bundle.setUniqueId( createUniqueId() );
   bundle.save();
}

The advantage of this is:
- it's obvious when the file gets written
- MetaBundle remains a general purpose class with no magic behaviour depending on the processs name
- only one setUniqueId method in MetaBundle should be sufficient then - just as with e.g. setArtist
- less logic in MetaBundle: no need for processesing 'recreate' or 'strip' options - just do a setUnqueId( createUniqueId() ) or setUniqueId( QString::null ) followed by saving the bundle - the other cases you mentioned would work with this scheme easily

In my opinion, removing the special logic for unque ids from MetaBundle and doing it where it is necessary, would really make it easier to understand what's going on (e.g. I just had a look at collectionscanner.cpp for writing this mail and I was thinking, where the hell do the unique ids get written, until I found that it happens in readTags!)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok/attachments/20060818/db283239/attachment.html>