XmlElement::writeToFile()


#1

Is it normal that the temporary file that gets created by XmlElement::writeToFile() does not get deleted?
In my directory I now see 40 times the same temp file with ending (1), (2), (3) …


#2

Well no, it’s not normal - I use that all method the time and have never had trouble with it.

As you can see in the XmlElement code, it always deletes the file. What directory are you writing to? Has it got some kind of strange permissions set on it?


#3

Ehm, no special directory. Just c:\ at the moment. I’ll try to find out what is going on and post it here.


#4

Thanks, I’d be keen to know if there’s a problem with deletion.


#5

After deleting the 38 AudioPrefs(xxx).xml files, the problem never reoccured. I have no clue why it happened. Anyway, I’m happy because it works now. :smiley:


#6

I wonder if it was something like a virus-checker grabbing those files while you were trying to delete them, so stopping it working…


#7

I’ve run into a similar problem, where XmlElement::writeToFile leaves the temp file (e.g. foo(2).xml) around. I think the code as it is correct, but it’s possible that the tempFile.deleteFile(); statement can fail. I’ve had this happen when two processes are trying to write to this file “at the same time.”

The reality that it’s possible for the temp files to stay around means to me that I’ve got to teach the uninstaller for my software to remove these files. This is a bit of a struggle because File::getNonexistentChildFile has no upper bound on the number it puts in parentheses. This means I’ve got do a similar thing looking for files to delete an uninstall time. It’d feel safer if we gave File::getNonexistentChildFile an optional max after which it would fail. It may mean a signature change beyond adding the new defaulted arg (or maybe a new function so existing code could remain unchanged), but it would help.

Beyond that, it also seems that we could teach XmlElement::writeToFile to clean up any temp files after a successful save since we know it’s safe to remove the temp files in that case. Instead of immediately returning true after tempFile.moveFileTo(f) succeeds, it could remove any lingering temp files. This operation also seems safer with a max to the number in parens.

Thoughts?


#8

How could it possibly know the difference between temporary files and ones that are needed?

All I can think of to make it more robust would be something like this:

[code] int i;
for (i = 5; --i >= 0;)
{
if (tempFile.moveFileTo (f))
return true;

            Thread::sleep (100);
        }

        for (i = 5; --i >= 0;)
        {
            if (tempFile.deleteFile())
                break;

            Thread::sleep (100);
        }[/code]

…which might be good enough to work around the file being temporarily grabbed by another process.


#9

I may be missing something but isn’t it easy to tell the difference between temporary files and the one file that’s needed? Temporary files always have (#) in them, and the file that’s needed doesn’t have the parens/number at the end. Is there any file reading code that uses the numbered filenames? I’m using this via PropertiesFile which doesn’t. I suppose it could, but then the app would need to learn the numbered filename somehow so it could pass it to the PropertiesFile constructor.

What I’m proposing is something like:

            if (tempFile.moveFileTo (f))
            {
                // we successfully renamed the temp file.
                // Since we know the "properly" named file
                // exists and has the correct/up-to-date
                // contents, it's OK to blow away any
                // remaining temp files that may have been
                // left over from earlier failures.
                for (i = 0;(i < max_temp_number);i++)
                {
                    File        oldTemp(i);
                    oldTemp.deleteFile();
                }
                return true;
            }

Where oldTemp(i) doesn’t quite work like that, but constructs a File object with the numbered name. Perhaps some part of File::getNonexistentChildFile could become a separate utility function to make this easier.

And maybe it makes sense to check oldTemp.exists() before calling oldTemp.deleteFile().


#10

And what happens if another thread or process is also using a temp file in the same directory?


#11

Trouble. A few options come to mind:

  1. make this extra code optional, though to be useful for me I’d need to get control in PropertiesFile as well. Apps turn this on when they know they’re the only process manipulating these files at a time.

  2. in case multiple processes do need to manipulate these files at the same time, create a mutex based on the absolute path to the file and lock it before writing/renaming/cleaning up. Looks like InterProcesLock could do this, especially if there’s a portable way to get the mutex name to start with “Global” on Windows > NT 4.0. Again, the protection could be optional. Seems like this protection might be useful whether or not the code in #1 makes it in.

  3. leave it the way it is but provide an upper bound to File::getNonexistentChildFile so it’s easier for apps or uninstallers to clean up.

Pondering #2 for a minute, if this is too much to put into XmlElement::writeToFile, how would an application handle this in juce? I guess this is really a question about the Global\ prefix with InterProcessLock under Windows.


#12

I’ve added my suggestion for making it have a few attempts at deleting the file, but I’m not going to write reams of complicated code to deal with this.

If you think it’s a big problem for you, just write your own version of that method, but use a temp filename with a known pattern. Then, when your app exits, do a quick check for any matching files and delete them.


#13

Adding an InterProcessLock doesn’t seem like reams of extra code. I could see making it optional qualifying for that though. What do you think about adding InterProcessLock to the code unconditionally?


#14

If you need a bullet-proof temp file, shouldn’t you be asking for a BulletProofTempFile class that handles all these eventualities? Or a TempFileManager class than keeps a record of all the files you’ve used and cleans them up on shutdown? Or a TemporaryFileOutputStream that would stream to a temporary file and swap it over when you delete it… Any of those would have to be able to handle eventualities like this, but it doen’t belong in an XML method!!

But first I’d rather know why these files can’t be deleted! Is it a virus checker grabbing them? Some kind of permissions problem? The machine’s search task indexing them?..


#15

That could be what I’m asking for, yes.

I haven’t nailed down exactly what’s going on when this fails for me. I’ve got two processes that I wrote both calling PropertiesFile::save at the same time. I don’t know what part of tempFile.moveFileTo is failing (the delete or the move), nor what’s causing the subsequent tempFile.deleteFile to fail. A slightly off-topic nit, but since neither juce_deleteFile nor juce_moveFile on windows log GetLastError it’s hard to know exactly why the temp file sticks around.

But, since I’ve got two processes fighting over the file at the same time, an InterProcessLock seems like just the thing. You’re right though that sticking that in XmlElement::writeToFile can’t be right, and that a separate class for managing the temp files is a better way to go – and a way that qualifies as reams of code.


#16

Ah, well you never said it was your own processes trying to write at the same time! Why not just use your own interprocesslock, then?


#17

I guess I can find a place to stick the InterProcessLock and the temp file cleanup stuff. Just trying to come up with a general solution so I don’t need to maintain any diffs from the public source tree.

PS. What are the chances of addingLogger::writeToLog("DeleteFile(" + fileName + ") failed (" + String(GetLastError()) + ")");or similar to juce_deleteFile, juce_moveFile, etc. in case of failure?


#18

I’d maybe add it as a DBG, though not sure what you’re going to learn from it. If, as you say, you’ve got two of your own processes writing at the same time, then surely all you’d need to do is put a lock around the writer.


#19

I’m finally getting around to writing the code to use InterProcessLock. While testing this, I can fairly easily make the assert on line 101 of juce_TemporaryFile.cpp fire. All it takes is 5 consecutive fails of moveFileTo.

Removing this assert (or at least moving it) seems OK since overwriteTargetFileWithTemporary communicates what happened via its return value.

From looking through the code, all the places that call overwriteTargetFileWithTemporary check its return value.

I haven’t checked all of those places in detail, but there’s definitely one place up the call stack that does ignore a return value – the PropertiesFile destructor ignores saveIfNeeded’s return value, which could fail due to a failure in overwriteTargetFileWithTemporary. To me, it seems more correct to check the return value there and assert if saveIfNeeded fails. What do you think?

VS2008 doesn’t warn me that there’s code that’s ignoring return values. There’s a way to get gcc to do it, but it may involve lots of warn_unused_result attributes.


#20

Yes, I guess that would be a better place for an assertion.