Skip to content

Conversation

@erenes
Copy link
Contributor

@erenes erenes commented Feb 19, 2019

Solves #226, but needs a review to see if I missed a scenario.

@erenes
Copy link
Contributor Author

erenes commented Feb 20, 2019

Second commit should help with #442, if AllowedFileShare is set to None for a file, File.Delete now throws an IOException as in regular System.IO

@erenes erenes changed the title Initial support for file locking Support for locked files Feb 20, 2019
@fgreinacher
Copy link
Contributor

Thanks for contributing this back!

I am a bit unsure about the approach of controlling the behavior solely through MockFileData - an alternative approach would be to (additionally) handle this by the regular APIs where you lock a file by passing FileShare.None. I see that it might involve some more implementation effort, but on the other side it would really be somehow unintuitive to support locking a file only via test APIs.

So my specific proposal would be to keep the property as convenience layer and additionally support locking via FileShare parameters passed to the various methods that somehow open files.

Other thoughts @jpreese @robkeim @tathamoddie?

@erenes
Copy link
Contributor Author

erenes commented Mar 6, 2019

Hi Florian, thanks for the feedback!

The only place where I expect this to require changes is in File.OpenInternal. When we open the FileStream, we already check the AllowedFileShare property to see if what we're doing is allowed.

It's trivial to add a set of the current FileShare option if successful:

        var mockFileData = mockFileDataAccessor.GetFile(path);
        mockFileData.CheckFileAccess(path, access);
        mockFileData.AllowedFileShare = share; // this would do it

        var length = mockFileData.Contents.Length;
        MockFileStream.StreamType streamType = MockFileStream.StreamType.WRITE;
        if (access == FileAccess.Read)

The tricky part is if you have several file streams open, and close one of them, what should the FileShare setting on the file then be afterwards? We will have to keep track of it somehow, and find out what the expected behavior of all combinations are.

Example:
What is the FileSharing mode after running the code below? Read, ReadWrite, reset to no lock at all?

  if (File.Exists(path)) File.Delete(path);
  File.WriteAllText(path, "hello world");

  FileStream fs = File.Open(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
  FileStream fs2 = File.Open(path, FileMode.Open, FileAccess.ReadWrite, FileShare.Read);
  FileStream fs3 = File.Open(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
  fs.Close();
  fs.Dispose();

While not impossible to implement, I think it would add a lot of complexity to the code. I decided to keep it simple (for now) because for the purposes of a unit test, you probably want to make sure that your tested function handles externally locked files properly.

I'm happy to be convinced otherwise, but I don't really see the need to verify that my tested code path has a contested file lock with itself in a unit test?

@fgreinacher
Copy link
Contributor

Thanks for the argumentation. You're right, keeping track of the intermediate states would add quite some complexity.

So let's start with this simple solution and wait for feedback.

Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit, otherwise looks great

@fgreinacher fgreinacher merged commit e1e14ef into TestableIO:master Mar 10, 2019
@fgreinacher
Copy link
Contributor

Merged, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants