System.IO.Abstractions icon indicating copy to clipboard operation
System.IO.Abstractions copied to clipboard

MockFileStream.InternalFlush is not thread-safe

Open Arithmomaniac opened this issue 3 years ago • 2 comments

https://github.com/TestableIO/System.IO.Abstractions/blob/05486f75815bd2f22beefa48407a42cb18034a72/src/System.IO.Abstractions.TestingHelpers/MockFileStream.cs#L93-L107

If Thread1 invokes this method, but Thread2 deletes the same file while Thread1 is at line 96, then Thread1 will throw a NullReferenceException at line 107.

I'm not sure what the desired behavior would be, but at a minimum lines 95-97 should be folded into an atomic mockFileDataAccessor.TryGetFile(path, out var mockFileData). I imagine it would look essentially like this, but without the ternary condition:

https://github.com/TestableIO/System.IO.Abstractions/blob/05486f75815bd2f22beefa48407a42cb18034a72/src/System.IO.Abstractions.TestingHelpers/MockFileSystem.cs#L383-L389

Arithmomaniac avatar Dec 28 '21 11:12 Arithmomaniac

Note that mock behavior should be different if it's mocking Unix or Windows filesystems. On Windows, you can't delete a file that's in use; you should get an IOException in that case. On Unix, you can delete the file, but as long as the reader's already got a reference to it, the data is still alive until the reader is finished with it.

ymarcov avatar Dec 29 '21 11:12 ymarcov

Thanks for bringing this up @Arithmomaniac!

I think it's a good idea to keep the testing helpers thread-safe where possible.

Happy to iterate on a (draft) PR for this!

fgreinacher avatar Jan 06 '22 12:01 fgreinacher