fix: file stream sharing

[HarrisonTCodes]

Closes #1356

This PR adds stateful handling of file streams opened with the FileShare.None option. If a file stream is attempted to be opened whilst another file stream of the same file path and FileShare.None is in use (that is to say, has been opened and not yet disposed), an IOException will be thrown and the creation of the second file stream will be disallowed.

[HarrisonTCodes]

Struggling to initially parse these API tests, not least because I can't seem to reproduce exactly on my machine. However, I expect the cause of failure might be because of the addition of the share argument in the MockFileStream constructor in MockFileStream.cs.

If this is the case, presumably this could be fixed by having share be the final argument (and keeping its default value), so the old argument order, including omission of this new one, would still be valid? This may not be ideal or align with the real file stream implementation though?

Worth nothing none of the factory methods have had their arguments or argument order changed.

[vbreuss]

Struggling to initially parse these API tests, not least because I can't seem to reproduce exactly on my machine. However, I expect the cause of failure might be because of the addition of the share argument in the MockFileStream constructor in MockFileStream.cs.

If this is the case, presumably this could be fixed by having share be the final argument (and keeping its default value), so the old argument order, including omission of this new one, would still be valid? This may not be ideal or align with the real file stream implementation though?

Worth nothing none of the factory methods have had their arguments or argument order changed.

@HarrisonTCodes : You change the public API surface in this PR. The API tests fail, because you didn't make this explicit in the PR. In order for the tests to succeed, you have to run the TestableIO.System.IO.Abstractions.Api.Tests.ApiAcceptance.AcceptApiChanges test once. This will adapt the API files and you have to commit them as part of this PR:

This way accidental API changes can be detected in the review.

Note, that this test is explicit!

[vbreuss]

Can you please add test cases to verify the correct behavior?

[HarrisonTCodes]

Ah, thanks @vbreuss, sorry I missed that. I will try to run these explicit tests locally!

Yes, if we are happy with the structure and changes made in this PR (which seems to be the case based on this request), I shall consider them accepted and add test cases :)

[hangy]

I like the idea!

It might be useful to check if symbolic links need additional handling

[hangy]

You may want to add some normalization before using the path as the dictionary key. Relative paths passed to the ctor could resolve to the same file system object. The mockFileDataAccessor does like this:

System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileStream.cs

Line 57 in 72ea54d

if (mockFileDataAccessor.FileExists(path))

System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileSystem.cs

Line 476 in 72ea54d

path = FixPath(path).TrimSlashes();

System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileSystem.cs

Lines 131 to 142 in 72ea54d

	private string FixPath(string path, bool checkCaps = false)
	{
	if (path == null)
	{
	throw new ArgumentNullException(nameof(path), StringResources.Manager.GetString("VALUE_CANNOT_BE_NULL"));
	}
	var pathSeparatorFixed = path.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar);
	var fullPath = Path.GetFullPath(pathSeparatorFixed);
	return checkCaps ? GetPathWithCorrectDirectoryCapitalization(fullPath) : fullPath;
	}

[HarrisonTCodes]

Thanks for your comment and comprehensive snippets! This is a great point.

I added a private NormalizePath method to the MockFileSystem class largely based on the above, which should handle relative paths passed to the ctor pointing to the same object.

I recognise there is a slight duplication of logic here, and maybe a more rigorous refactor with a general-use, shared FixPath method somewhere (or potentially just making FixPath public?), instead of having these two private methods, would be preferred - this could be worth some discussion.

Please let me know your thoughts, and if you think the normalization implemented is sufficient!

[vbreuss]

I think it would be better to de-duplicate this logic into an internal method that can be used whenever we need to normalize a path to ensure it is done the same everywhere.

[HarrisonTCodes]

I think it would be better to de-duplicate this logic into an internal method that can be used whenever we need to normalize a path to ensure it is done the same everywhere.

Agreed. I have been digging around the codebase, and it seems to me a good approach to this would be to move the FixPath method (and associated other methods) logic from the MockFileSystem class into the PathVerifier class, updating both MockFileSystem and MockFileStream accordingly?

As usual, its possible I'm missing something here or overlooking a more apt approach that those with more experience in this codebase may be able to see, so please do let me know if you think there's a better approach!

[vbreuss]

That sounds reasonable...

[HarrisonTCodes]

This should be covered in eabc64e (explicit API test runs in following commit), let me know if there are any other desired changes here!

[HarrisonTCodes]

I like the idea!

It might be useful to check if symbolic links need additional handling

@hangy thanks for raising this :)

Maybe this is worth more discussion, and a clarification of the scope of this PR. Currently it just targets the behavior shown in the original (linked) issue, ie creation of a MockFileStream with an explicit FileShare.None argument. This seems like handy behavior to have when explicitly passing this argument.

However, there might be appetite to improve the handling of FileShare behavior across more of the APIs, such as in MockFile.cs and implicit file-sharing that mimics the real filesystem with methods like FileInfo.OpenWrite() too.

More realistic handling of other FileShare members might also be worth thinking about in the future too for example. This PR currently just handles FileShare.None. But maybe there is scope to handle these other modes better, such as disallowing write operations on a file when a file stream is open with just FileShare.Read (which could be doable with the use of a ConcurrentDictionary now instead of the old HashSet, with the value representing the share type!)

[vbreuss]

This now works only for FileShare.None, but not for read and write access:

Access	Share	Result
Read	Read	ok
Read	ReadWrite	ok
Read	Write	throw
Read	None	throw
Write	Read	throw
Write	ReadWrite	ok
Write	Write	ok
Write	None	throw
ReadWrite	Read	throw
ReadWrite	ReadWrite	ok
ReadWrite	Write	throw
ReadWrite	None	throw

I think it would be a great idea to also handle these cases correctly. What do you think?

[HarrisonTCodes]

I agree. I briefly mentioned an approach to this in a previous comment, where instead of just using the ConcurrentDictionary as effectively a concurrent hashmap, we actually use the value of entries to store the share they were opened with, and check against it where we currently just check for existence.

This is the approach I am planning to go for and start working on, but let me know if you think there's a better idea I'm overlooking or something I'm missing!

[vbreuss]

I fear the solution is not so easy, as you can have multiple streams with e.g. read access and read share open at the same time and only when all streams got disposed should you be able to open the file with write access. So you would need to create a disposable access reference for each stream and remove this instance upon disposal of the stream from some kind of storage.

For reference: in Testably.Abstractions I used a ConcurrentDictionary<IStorageLocation, ConcurrentDictionary<Guid, FileHandle>> with a custom FileHandle class (see here).

[HarrisonTCodes]

Ah yes, multiple streams of the same file open at once...
Thanks for the reference, I'll checkout how its handled in Testably.Abstractions, maybe some of it could be directly ported over?

[vbreuss]

I think you could combine these two tests into one and verify that after you dispose of the stream it no longer throws...

[vbreuss]

I think it would be better to de-duplicate this logic into an internal method that can be used whenever we need to normalize a path to ensure it is done the same everywhere.