ATF safety

[Jeff Mitchell]

On Thursday 17 August 2006 02:08, Martin Aumueller wrote:
> If such a data corruption as some ATF users experienced happened to me, I'd
> be pissed. If turning this feature on would have been recommended by the
> program that caused the corruption, then doubly so. And even more so, if
> its developers continuously insist on it being safe despite I having proof
> of the opposite.
>
> Thus I suggest that we:
> - leave ATF disabled by default (probably forever)

Already done -- it was always off by default

> - remove any ATF recommendation in Amarok's config dialog

Also already done -- it just says "see Information" which has suitably obvious 
warnings when clicked, with a PLEASE NOTE in big red letters.

> - stop insisting on its safety

Rest assured that I do take this corruption issue seriously.  However, out of 
all the various issues people have tried to pin on ATF there has been one 
single issue that has been its fault -- this possible corner case that can 
cause corruption, and the semantics of it are such that it is/was not likely 
to happen to many people.  I didn't plan for it because I thought taglib 
would (as you recommended below) do file locking itself when it was writing 
to a file. (Scott will be including a warning in the API docs of the next 
version of TagLib explicitly stating that this is not the case).

All the other problems have been either taglib problems or someone else 
changing my code and breaking it.  Statistically speaking, ~jefferai is still 
true :-)

> - 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.

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 case 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?

>   * avoid code that needs triple WARNINGs in the comments (some setUniqueId
> that we have right now) - the code should be easily understandable by all
> developers

Yes.  The warning is very easy to understand.  It says, don't set the uniqueid 
manually.  Because you shouldn't be doing it unless you have a good reason -- 
you should be letting the scanner and database handle this work.  This is not 
a dangerous case, it's just not something you want to be doing unless you are 
sure you are associating the correct UID with the correct MetaBundle.  Parts 
of the code that do this use this function.

>   * 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.

>     + 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.

--Jeff