Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MockFileStream.InternalFlush is not thread-safe #784

Open
Arithmomaniac opened this issue Dec 28, 2021 · 2 comments
Open

MockFileStream.InternalFlush is not thread-safe #784

Arithmomaniac opened this issue Dec 28, 2021 · 2 comments
Labels
area: testinghelpers Issues that address the testing helpers state: ready to pick Issues that are ready for being worked on type: bug Issues that describe misbehaving functionality

Comments

@Arithmomaniac
Copy link

private void InternalFlush()
{
if (mockFileDataAccessor.FileExists(path))
{
var mockFileData = mockFileDataAccessor.GetFile(path);
/* reset back to the beginning .. */
var position = Position;
Seek(0, SeekOrigin.Begin);
/* .. read everything out */
var data = new byte[Length];
Read(data, 0, (int)Length);
/* restore to original position */
Seek(position, SeekOrigin.Begin);
/* .. put it in the mock system */
mockFileData.Contents = data;

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:

private MockFileData GetFileWithoutFixingPath(string path)
{
lock (files)
{
return files.TryGetValue(path, out var result) ? result.Data : null;
}
}

@ymarcov
Copy link

ymarcov commented Dec 29, 2021

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.

@fgreinacher
Copy link
Contributor

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 fgreinacher added area: testinghelpers Issues that address the testing helpers state: ready to pick Issues that are ready for being worked on type: bug Issues that describe misbehaving functionality labels Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testinghelpers Issues that address the testing helpers state: ready to pick Issues that are ready for being worked on type: bug Issues that describe misbehaving functionality
Projects
None yet
Development

No branches or pull requests

3 participants