10
\$\begingroup\$

I just found out about the existence of this forum and it's exactly what I needed. I just asked this question on Stack Overflow and after reading the answers I came up with a code (not the one I posted there but the one I'm posting here).

First, this is my messed up code:

string tempFile = Path.GetTempFileName();

using (Stream tempFileStream = File.Open(tempFile, FileMode.Truncate))
{
    SafeXmlSerializer xmlFormatter = new SafeXmlSerializer(typeof(Project));
    xmlFormatter.Serialize(tempFileStream, Project);
}

if (File.Exists(fileName)) File.Delete(fileName);
File.Move(tempFile, fileName);
if (File.Exists(tempFile)) File.Delete(tempFile);

When saving to an existing file that is in my Dropbox, it will, sometimes, delete the original file and fail to move the temporary file to the original location in File.Move(tempFile, fileName);. So here is my new code that I think it should never, unless the OS became self-conscious and evil, delete the original file without saving the new one. The worst it can happens is that the original will be renamed and will stay like that and I will have to let the user know (see the MessageBox at the bottom):

private string GetTempFileName(string dir)
{
    string name = null;
    int attempts = 0;
    do
    {
        name = "temp_" + Player.Math.RandomDigits(10) + ".hsp";
        attempts++;
        if (attempts > 10) throw new Exception("Could not create temporary file.");
    }
    while (File.Exists(Path.Combine(dir, name)));

    return name;
}

private void TryToDelete(string path)
{
    try { File.Delete(path); }
    catch { }
}

private void SaveProject(string fileName)
{
    bool originalRenamed = false;
    string tempNewFile = null;
    string oldFileTempName = null;
    Exception exception = null;

    try
    {
        tempNewFile = GetTempFileName(Path.GetDirectoryName(fileName));

        using (Stream tempNewFileStream = File.Open(tempNewFile, FileMode.CreateNew))
        {
            SafeXmlSerializer xmlFormatter = new SafeXmlSerializer(typeof(Project));
            xmlFormatter.Serialize(tempNewFileStream, Project);
        }

        if (File.Exists(fileName))
        {
            oldFileTempName = GetTempFileName(Path.GetDirectoryName(fileName));
            File.Move(fileName, oldFileTempName);
            originalRenamed = true;
        }

        File.Move(tempNewFile, fileName);
        originalRenamed = false;

        CurrentProjectPath = fileName;
    }
    catch (Exception ex)
    {
        exception = ex;
    }
    finally
    {
        if (tempNewFile != null) TryToDelete(tempNewFile);

        if (originalRenamed)
        {
            try
            {
                File.Move(oldFileTempName, fileName);
                originalRenamed = false;
            }
            catch { }
        }

        if (exception != null) MessageBox.Show(exception.Message);

        if (originalRenamed)
        {
            MessageBox.Show("'" + fileName + "'" +
                " have been corrupted or deleted in this operation.\n" +
                "A backup copy have been created at '" + oldFileTempName + "'");
        }
        else if (oldFileTempName != null) TryToDelete(oldFileTempName);
    }
}

Player.Math.RandomDigits is just a function that returns a string with random digits. Do you think my code is safe as I think it is or am I missing something? It's hard for me to test every possible exception.

\$\endgroup\$
4
\$\begingroup\$
  • I don't like discarding all exceptions from File.Delete(). I think it's fine to discard a DirectoryNotFoundException here, but I think the others should be allowed to propagate up and be shown to the user. I find that hiding the fact that the user does not have write access or that another process has the file locked generally tends to cause more confusing errors later on.

  • You serialise the new content inside the big try-catch. If the serialisation fails then you tell the user that the file has been corrupted or deleted and they should restore from the backup provided, even though you have not reached the point where the backup is made.

  • The idea of having one big try-catch and then using flags to identify what stage you were up-to makes me a little uneasy. You can replace the originalRenamed flag with a try-catch around File.Move(tempNewFile, fileName) since this is the only call made while the flag is set to true.

    try
    {
        // ...
    
        try
        {
            File.Move(tempNewFile, fileName);
        }
        catch
        {
            if (!string.IsNullOrEmpty(oldFileTempName) && File.Exists(oldFileTempName))
            {
                File.Move(oldFileTempName, fileName);
            }
    
            MessageBox.Show("...");
    
            throw;
        }
    
        // ...
    
        if (!string.IsNullOrEmpty(oldFileTempName) && File.Exists(oldFileTempName))
        {
            TryToDelete(oldFileTempName);
        }
    }
    catch(Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
    

    This may be inconvenient if you want the message boxes to appear in the specific order but I don't think it should be too much of an issue.

\$\endgroup\$
  • \$\begingroup\$ Wait, if serialization fails I won't tell the user that the file have been corrupted because the originalRenamed flag won't be set. Also, the reason I use a flag instead of putting the File.Move(tempNewFile, fileName) into a try-catch is because I don't wan't to show the file corrupted message if I didn't previously call File.Move(fileName, oldFileTempName);, instead, just delete the tempNewFile and show the IOException. Does it make sense? \$\endgroup\$ – Juan Mar 24 '11 at 18:03
  • \$\begingroup\$ But I do agree with the discarding all the File.Delete exceptions issue. \$\endgroup\$ – Juan Mar 24 '11 at 18:13
  • \$\begingroup\$ Yes, you are right about the second point. Though I still maintain however, that it would be better to have a more localised try-catch block than using flags. In the example I provided, it checks if the backup exists so that the "corrupted message" will not be shown if no backup was made. \$\endgroup\$ – Brian Reichle Mar 25 '11 at 1:53
5
\$\begingroup\$

Using File.Replace() still seems the best option to me. As you mentioned on SO, the following might give a problem:

If the sourceFileName and destinationFileName are on different volumes, this method will raise an exception. If the destinationBackupFileName is on a different volume from the source file, the backup file will be deleted.

To prevent this exception from being thrown, can't you first check whether you can serialize the temp file directly to the desired volume? If this isn't possible, further processing will fail as well, so you can already show a message that the volume isn't writeable.

Afterwards, call the Replace function, creating a backup file. When it fails, handle appropriately and check in which state your files are, indicate to the user when something is 'corrupt' (so point to the backup file). In the finally check whether the backup file exists and remove.

\$\endgroup\$
  • \$\begingroup\$ This sounds good. The only problem is the "chech in which state your files are" part. What if something fails when checking that? File.Exists is known to be unreliable. My code, though, knows exactly on which state the files are (only thing it doesn't really know is whether the temporary files have been deleted or not, but that's not as relevant as knowing whether the original file have been renamed or not). \$\endgroup\$ – Juan Mar 24 '11 at 18:29
  • \$\begingroup\$ @jsoldi: I don't know of any unreliabilities of File.Exists. \$\endgroup\$ – Steven Jeuris Mar 24 '11 at 19:25
  • \$\begingroup\$ I've seen it myself. Sometimes it will return false when the file does exist. Something to do with files virtualization. \$\endgroup\$ – Juan Mar 24 '11 at 19:33

protected by Jamal Sep 15 '18 at 4:40

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.