[Development] QFile: writing via a temporary file

[David Faure]

On Friday 06 January 2012 11:41:05 =?ISO-8859-1?Q?Jo=E3o?= Abecasis wrote:
> > Solution 2: how about making this functionality part of QFile itself?
> > No need to "port to another class" anymore, just enable the safety
> > feature by calling file.setUseTemporaryFile(true).  This is what I've
> > started doing in the attached patch, but I'd like to gather feedback
> > before polishing it up.  One issue is that after doing
> > file.setUseTemporaryFile(true) and file.open(), all the operations on
> > the file object are no longer operating on the given fileName, but on
> > the "internal" temporary file. That's what we want for writing, but
> > maybe it could confuse people that remove() or rename() leaves the
> > existing file untouched? I think it would simply have to be
> > documented: when enabling the feature, all that happens between open()
> > and close(), happens on the temp file.
> 
> I don't support putting this in QFile as has been suggested as, from my
> experience with it, this will open a can of worms in maintenance and
> subtle issues cropping up in user code such as the ones being discussed:
> what's fileName()?  Does it exists() before commit? What do remove() and
> rename(newName) do?

If we go towards the idea that the temp file is really internal (as the idea of 
a mode flag would point to), then fileName() can keep being the target file,
exists() will only be true if it already existed before starting to write.

If it didn't exist before, then f.open(); f.write(); f.exists() is very 
fragile anyway, isn't it? It sounds like the result depends on whether OS 
flushed it to disk or not... So this doesn't seem like a valid use case anyway.

When it comes to remove and rename, they are already documented to close the 
file first, so this would finish the writing operation, and no surprises will 
happen to the user calling them. No issue of "which file is it about", there is 
only one file after close() anyway.

I think if we think of the tempfile as "an internal buffering mechanism" rather 
than "a second file exposed via the qfile api", then all these issues of "which 
file is this about now" don't happen at all.

> I think we would be well served by an API that exposes a QIODevice
> interface plus additional interface for commit/rollback.

This represents more porting effort for the application developer.
I have seen it in KDE in the past, where having to manage one more object, but 
only in writing mode, not in reading mode, and making sure to call 
commit/rollback on it, was a lot of trouble, compared to the initial code that 
was simply using a QFile for all that. Enabling a flag on that QFile would have 
been soooo much easier.

> > The other question is, would one have to call commit/rollback
> > explicitely, or should QFile::close() (and the dtor) do the
> > committing?
> 
> I think commit should require explicit action, for instance, by
> explicitly calling close(). I don't like having the destructor
> automatically commit (even if it calls close) though.

QFile's destructor has always called close(), that's in everyone's mind, so I 
don't think we want explicit and implicit close to work differently, that would 
be very confusing.

When writing to a file, you normally don't have to confirm "yes I want to commit 
my changes". So the safer use of a temp file shouldn't require this either, 
others have convinced me in this thread.

> An exception being
> thrown in the middle of a save operation could potentially unwind the
> stack, call the destructor and commit a transaction half-way through.

Exception handling is a new argument though. But doesn't the current QFile 
have the exact same issue then? You start writing, throw an exception, dtor 
calls close, there you go. The use of an internal temporary file doesn't change 
this at all, so I don't see why it should behave differently.

If one can throw exceptions in the middle of a save operation, he'll need a 
RAII class that calls rollback, I would think.

> What happens to writes happening after a commit, though?

This problem doesn't happen if commit == close(). You can't write after 
closing, that much is obvious to everyone.

> > And how should one rollback? Explicit file.rollback(), or
> > in the existing file.remove()?  Oswald suggested doing that in
> > close()/remove() directly, and getting rid of commit()/rollback().
> > Opinions?
> 
> I wouldn't like remove() to be doing anything different from what it
> does now: delete the file pointed to by fileName.

Here, I agree.

remove() is close()+remove(fileName), so in all cases (tempfile or not), it will 
remove the target file completely.

Rolling back should be a different method, I'm convinced of that now. Not only 
is it much clearer, it also doesn't break the above rule.

> > Solution 1: using a separate class for handling the QTemporaryFile and
> > the rename call. KDE's KSaveFile and QtCreator's SaveFile do that. The
> > code of such a class is quite easy. The downside is that from an API
> > point of view, it's a bit weird. You have to use this "save file"
> > wrapper class, which is hard to picture in one's mind, and to name
> > properly for what it does.  API-wise, such a class has methods like
> > commit() and rollback(), to decide what happens when we're done. If
> > you forget to call either one, the destructor will decide for you.
> > Funny, in KDE it commits, in QtCreator it rolls back...
> 
> And still those are working examples from the real-world where exactly
> such solutions were preferred over extending QFile.

We had no other choice back then! It was pretty much impossible to contribute 
to Qt, this is why we all had to have our own additional classes. This 
definitely doesn't mean it was the best way to solve the problem...

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5