Sharing Violation on Path when no stream is open

Question

I'm developing a game in C# in which each "District" will have data about it stored in a save file periodically. For testing, the "SaveAll" method is called once at the beginning of the level.

The code for the file operations is as follows:

using UnityEngine;
using UnityEngine.UI;
using System.Collections;
using System.IO;

public class DistrictSaveData : KeepAwake {

    private string saveDirectoryPath = string.Concat(
        System.Environment.GetFolderPath(System.Environment.SpecialFolder.MyDocuments),
        "\\My Games\\District\\Districts");
    private string saveFilePath = string.Concat(
        System.Environment.GetFolderPath(System.Environment.SpecialFolder.MyDocuments),
        "\\My Games\\District\\Districts\\District-x.dat");
    private StreamReader saveFileReader;

    public void SaveAll() {
        foreach (GameObject gO in GameObject.FindGameObjectsWithTag("District")) {
            District district = gO.GetComponent<District>();
            saveFilePath = string.Concat(
                System.Environment.GetFolderPath(System.Environment.SpecialFolder.MyDocuments),
                "\\My Games\\District\\Districts\\District-", district.id , ".dat");
            if (!Directory.Exists(saveDirectoryPath)) {
                Directory.CreateDirectory(saveDirectoryPath);
            }
            try {
            File.Delete(saveFilePath);
            } catch {}
            File.Create(saveFilePath);
            File.WriteAllText(saveFilePath, district.SendSaveData());
        }
    }

    public void LoadAll() {
        foreach (GameObject gO in GameObject.FindGameObjectsWithTag("District")) {
            District district = gO.GetComponent<District>();
            saveFilePath = string.Concat(
                System.Environment.GetFolderPath(System.Environment.SpecialFolder.MyDocuments),
                "\\My Games\\District\\Districts\\District-", district.id , ".dat");
            if(File.Exists(saveFilePath)) {
                OpenFileForReading();
                district.isHQ = bool.Parse(saveFileReader.ReadLine());
                district.controllingFaction = StringToFaction(saveFileReader.ReadLine());
                district.agricultureSpecialisation = StringToAgricultureSpecialisation(saveFileReader.ReadLine());
                district.technologySpecialisation = StringToTechnologySpecialisation(saveFileReader.ReadLine());
                district.militarySpecialisation = StringToMilitarySpecialisation(saveFileReader.ReadLine());
                CloseFileAfterReading();
            } else
                break;
        }
    }

    /// <summary>
    /// Opens the save file for reading.
    /// </summary>
    private void OpenFileForReading() {
        saveFileReader = File.OpenText(saveFilePath);
    }

    /// <summary>
    /// Closes the save file after reading.
    /// </summary>
    private void CloseFileAfterReading() {
        saveFileReader.Close();
    }

    private Faction StringToFaction(string stringToConvert) {
        switch (stringToConvert) {
        case "TheCrimsonLegion":
            return Faction.TheCrimsonLegion;
        case "TheVanguardsOfChaos":
            return Faction.TheVanguardsOfChaos;
        case "TheEmeraldFoxes":
            return Faction.TheEmeraldFoxes;
        case "TheSyndicate":
            return Faction.TheSyndicate;
        case "TheKeepersOfTheTome":
            return Faction.TheKeepersOfTheTome;
        case "TheArchitectsOfThought":
            return Faction.TheArchitectsOfThought;
        default:
            return Faction.None;
        }
    }

    private AgricultureSpecialisation StringToAgricultureSpecialisation(string stringToConvert) {
        switch (stringToConvert) {
        case "Farm":
            return AgricultureSpecialisation.Farm;
        case "Plantation":
            return AgricultureSpecialisation.Plantation;
        case "Biodome":
            return AgricultureSpecialisation.Biodome;
        default:
            return AgricultureSpecialisation.None;
        }
    }

    private TechnologySpecialisation StringToTechnologySpecialisation(string stringToConvert) {
        switch (stringToConvert) {
        case "Laboratory":
            return TechnologySpecialisation.Laboratory;
        case "University":
            return TechnologySpecialisation.University;
        case "GreatTechnologicalInstitution":
            return TechnologySpecialisation.GreatTechnologicalInstitution;
        default:
            return TechnologySpecialisation.None;
        }
    }

    private MilitarySpecialisation StringToMilitarySpecialisation(string stringToConvert) {
        switch (stringToConvert) {
        case "Outpost":
            return MilitarySpecialisation.Outpost;
        case "Barracks":
            return MilitarySpecialisation.Barracks;
        case "Fortress":
            return MilitarySpecialisation.Fortress;
        default:
            return MilitarySpecialisation.None;
        }
    }
}

The exception that is thrown (several times) reads:

IOException: Sharing violation on path C:\users\samdy1\My Documents\My Games\District\Districts\District-0.dat
System.IO.FileStream..ctor (System.String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, Boolean anonymous, FileOptions options) (at /Users/builduser/buildslave/mono-runtime-and-classlibs/build/mcs/class/corlib/System.IO/FileStream.cs:320)
System.IO.FileStream..ctor (System.String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize)
(wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare,int)
System.IO.File.Create (System.String path, Int32 bufferSize) (at /Users/builduser/buildslave/mono-runtime-and-classlibs/build/mcs/class/corlib/System.IO/File.cs:135)
System.IO.File.Create (System.String path) (at /Users/builduser/buildslave/mono-runtime-and-classlibs/build/mcs/class/corlib/System.IO/File.cs:130)
DistrictSaveData+<SaveAll>c__Iterator3.MoveNext () (at Assets/Scripts/_Core/SaveData/DistrictSaveData.cs:29)

This exception is thrown on lines 28 and 29 of the process in the SaveAll method. However, the SaveAll method uses no streams so I fail to see how one can be left open. In fact, at this point in the level, the reading stream hasn't been opened at all.

Have I missed something obvious?

Side note: consider `Path.Combine` rather than `string.Concat`. It automatically ensures path separators are properly handled. — Eric J., Sep 03 '15 at 21:57
Those file system readers etc. are ``IDisposable``. Instead of having an instance as member, I think it would preferable to apply the ``using reader = new FileReader(path) { ... }`` approach. — BitTickler, Sep 03 '15 at 21:57
The reason being, if an exception is thrown while reading the files, you do not close them. — Eric J., Sep 03 '15 at 21:58
The second reason being that if you happen to have multiple threads working on DIFFERENT files, you have fewer things to worry about, too. — BitTickler, Sep 03 '15 at 21:59
@EricJ. The code in LoadAll() has not been called at this point, so no exception could be thrown reading them. Regardless, I tried closing the file before the File.Create() and File.WriteAllText() lines that cause the issue and still got the same errors. — notquiteamonad, Sep 04 '15 at 06:34
I must apologise for not adding enough detail. The error is definitely being caused in the SaveAll() method. I am now attempting to isolate the issue. — notquiteamonad, Sep 06 '15 at 10:20

Eric J. · Accepted Answer · 2015-09-06T16:46:46.550

There's not enough information to know for sure, but if you look at this code:

OpenFileForReading();
district.isHQ = bool.Parse(saveFileReader.ReadLine());
district.controllingFaction = StringToFaction(saveFileReader.ReadLine());
district.agricultureSpecialisation = StringToAgricultureSpecialisation(saveFileReader.ReadLine());
district.technologySpecialisation = StringToTechnologySpecialisation(saveFileReader.ReadLine());
district.militarySpecialisation = StringToMilitarySpecialisation(saveFileReader.ReadLine());
CloseFileAfterReading();

If an exception is thrown after OpenFileForReading() but before CloseFileAfterReading(), the file will not be closed and you will see the behavior your describe.

At a minimum, rewrite the code as

OpenFileForReading();
try
{
    district.isHQ = bool.Parse(saveFileReader.ReadLine());
    district.controllingFaction = StringToFaction(saveFileReader.ReadLine());
    district.agricultureSpecialisation = StringToAgricultureSpecialisation(saveFileReader.ReadLine());
    district.technologySpecialisation = StringToTechnologySpecialisation(saveFileReader.ReadLine());
    district.militarySpecialisation = StringToMilitarySpecialisation(saveFileReader.ReadLine());
}
finally
{
    CloseFileAfterReading();
}

You would be better off though with a using block rather than separate method calls to open/close the file.

+1 for identifying the very same code section as a likely candidate which also raised my attention. This C-style approach (open,use,close) is not as trustworthy as a using block. — BitTickler, Sep 03 '15 at 22:03

binki · Answer 2 · 2018-05-28T13:19:35.987

You say that the exception is thrown on lines 28 and 29.

Line 29

This line refers to the File.WriteAllText() call.

For line 29, I can very easily see what’s happening. You are failing to call .Dispose() on the FileStream returned by File.Create(). File.Create(path) is a shorthand for new FileStream(path, FileMode.Create). Since it is unspecified, the default value of FileShare.None is used. This means that the subsequent call to File.WriteAllText() will fail.

Instead of this pattern, you should remove File.Create(). File.WriteAllText() will already create or truncate the file, so there is no need to precreate it or truncate it manually. If you insist on calling File.Create() (which is nonsensical), you should call it in using like the following to ensure that its handle is closed prior to calling File.WriteAllText():

using (File.Create(saveFilePath)) {
}
File.WriteAllText(saveFilePath, district.SendSaveData());

It is possible, but unlikely, that garbage collection occurs after File.Create() returns and before File.WriteAllText() attempts to open the file. This might happen, for example, if district.SendSaveData() creates a lot of objects and uses lots of memory. Such behavior may (but is not guaranteed to) result in garbage collection happening. Additionally, all of the arguments to a method are always evaluated prior to the actual method call, so File.WriteAllText() will not run until that method exits. If garbage collection does happen in this interval, it is possible that the finalizer of the FileStream returned from File.Create() runs prior to File.WriteAllText() opening the file. And if that happens, then you will not see an exception on line 29.

Line 28

This line refers to the File.Create() call.

For exceptions that happen on line 28, I can only guess at what is happening. It is unclear from your code which order the public methods of your class will be called in. However, there there is one possibility that comes to mind.

If your call to SaveAll() from code which you omitted traps the exception from line 29 and then calls SaveAll() again later without garbage collection and finalizers running, then line 28 should throw the sharing violation exception. This is the sequence of events:

Line 28 runs for a particular district for the first time and returns a FileStream. Let’s name this guy FileStream A.
Line 29 throws an exception due to FileStream A holding an open file handle to the file in question.
The exception from line 29 is trapped by external code.
The external code calls SaveAll() again.
Line 28 tries to open a second FileStream for the same district again and throws an exception due to FileStream A still holding an open file handle to the file in question.
Eventually the garbage collector runs and calls the finalizer for FileStream A. After this happens, the code will get past line 28 for this district (but will likely fail at line 29).

Another possibility is the file being opened in any other process or opened by another part of your program.

Advice

I do not believe that this is related to the errors, but I must mention it. I would strongly recommend against using a field variable to hold a TextReader like you did with saveFileReader. That disassociates scope from the lifetime of the variable. Instead, you should pass the TextReader as an argument to methods, control its lifetime with using, and avoid saving it to a variable which is not controlled by scoping rules (such as a field) unless you are making a convenience object which itself supports scoping rules (by implementing IDisposable, for example).

One thing I noticed about your code is that you never even take advantage of the fact that saveFileReader is a field. You use that variable like it is a local. The fact that it is a field rather than a local suggests to any reader of the code that you would refer to it from multiple methods or intend to store a value in there which must be preserved across multiple calls. For example, it’d make sense for you to access that variable from StringToAgricultureSpecialisation() if you were going to bother making it a field in the first place. However, you only access it as a field using OpenFileForReading() and CloseFileForReading().

I am going to just demonstrate how to use a scoping pattern with your code:

using (var saveFileReader = File.OpenText(saveFilePath)) {
    district.isHQ = bool.Parse(saveFileReader.ReadLine());
    district.controllingFaction = StringToFaction(saveFileReader.ReadLine());
    district.agricultureSpecialisation = StringToAgricultureSpecialisation(saveFileReader.ReadLine());
    district.technologySpecialisation = StringToTechnologySpecialisation(saveFileReader.ReadLine());
    district.militarySpecialisation = StringToMilitarySpecialisation(saveFileReader.ReadLine());
}

See? There was no need for a field.

Generally, if possible, you should avoid using fields or properties if they refer to objects which have scope-based lifetimes because it is easier to make mistakes when using fields or properties. If you can instead use a local variable and using, it is easier to write correct code. Additionally, this technique increases readability because you can know that the variable can only be accessed from within the method instead of possibly being accessed by any other method in the same class.

Sharing Violation on Path when no stream is open

2 Answers2

Line 29

Line 28

Advice