Skip to content

Added a multithreaded hydration functional test #212

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.

Sign up for GitHub

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

Merged
Saeed Noursalehi (sanoursa) merged 3 commits into from
Aug 24, 2018
Merged

Added a multithreaded hydration functional test #212

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89d702c
Added a multithreaded functional test that currently fails on Mac by …
sanoursa Aug 23, 2018
9790552
Fix spacing
sanoursa Aug 23, 2018
18611be
Mark the new test as Windows-only
sanoursa Aug 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/MultithreadedReadWriteTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using GVFS.FunctionalTests.FileSystemRunners;
using GVFS.FunctionalTests.FileSystemRunners;
using GVFS.FunctionalTests.Should;
using GVFS.Tests.Should;
using NUnit.Framework;
Expand All @@ -14,8 +14,48 @@ namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture
[Category(Categories.Mac.M1)]
public class MultithreadedReadWriteTests : TestsWithEnlistmentPerFixture
{
[TestCase]
public void CanReadUnhydratedFileInParallelWithoutTearing()
[TestCase, Order(1)]
[Category(Categories.Windows)]

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Marked as Windows-only for now. I'm rerunning the tests on both platforms now, and will merge in once those pass. I'll then remove this category in my other PR for reproing and prototyping fixes for the races.

public void CanReadVirtualFileInParallel()
{
// Note: This test MUST go first, or else it needs to ensure that it is reading a unique path compared to the
// other tests in this class. That applies to every directory in the path, as well as the leaf file name.
// Otherwise, this test loses most of its value because there will be no races occurring on creating the
// placeholder directories, enumerating them, and then creating a placeholder file and hydrating it.

string fileName = Path.Combine("GVFS", "GVFS.FunctionalTests", "Tests", "LongRunningEnlistment", "GitMoveRenameTests.cs");
string virtualPath = this.Enlistment.GetVirtualPathTo(fileName);

Exception readException = null;

Thread[] threads = new Thread[32];
for (int i = 0; i < threads.Length; ++i)
{
threads[i] = new Thread(() =>
{
try
{
FileSystemRunner.DefaultRunner.ReadAllText(virtualPath).ShouldBeNonEmpty();

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there would be value in using all of our runners here, since they may have slightly different timing or system calls that trigger hydration in different ways.

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And that implies a need to make this class create an enlistment per test case, since the test is guaranteed to pass the second time around, regardless of races

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interestingly enough, I can't repro the failures with the BashRunner, so I'm not going to add the overhead of a new enlistment per test case for now. It must be that having to spawn a new bash process per request slows things down enough to avoid hitting these races, because the SystemIORunner fails completely reliably and the BashRunner passes completely reliably, on my MacBook Pro.

}
catch (Exception e)
{
readException = e;

@jrbriggs John Briggs (jrbriggs) Aug 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the number of failed reads provide any interesting signal?

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wondered about that too, but I'm not sure that it would. The number needs to be 0, always. And if it's ever non-zero, it signals that there's at least one race, somewhere, but gives no more signal than that. So I think this is a binary test, unless you can think of more we can get out of it?

@wilbaker William Baker (wilbaker) Aug 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I think this is a binary test, unless you can think of more we can get out of it?

Saeed Noursalehi (@sanoursa), have you checked if the thread(s) that are failing are all failing with the same exception?

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Every time I've seen it fail, it has been one of the two failures that you and Nick reported. One of the directories or the file appears missing.

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's no expectation that they will all fail with the same exception. What's happening is that they are all racing on creating the placeholder directories, expanding those directories, then creating the placeholder file, and finally hydrating it. Since we currently have races in our enumeration of directories, one of the directories or the leaf file will sometimes appear to be missing.

That said, I can definitely update it to print out every exception we get, if you think it'll add more diagnostic value.

@wilbaker William Baker (wilbaker) Aug 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That said, I can definitely update it to print out every exception we get, if you think it'll add more diagnostic value.

I think we can start with just printing one. I agree the most important\interesting part of this test is simply whether or not it succeeds.

}
});

threads[i].Start();
}

for (int i = 0; i < threads.Length; ++i)
{
threads[i].Join();
}

readException.ShouldBeNull("At least one of the reads failed");

@wilbaker William Baker (wilbaker) Aug 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you print the Exception callstack here as well?

@sanoursa Saeed Noursalehi (sanoursa) Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It already is - see your test output up above :-)

@wilbaker William Baker (wilbaker) Aug 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should have checked the output first :) You're right I can see the exception, thanks!

}

[TestCase, Order(2)]
public void CanReadHydratedPlaceholderInParallel()
{
FileSystemRunner fileSystem = FileSystemRunner.DefaultRunner;
string fileName = Path.Combine("GVFS", "GVFS.FunctionalTests", "Tests", "LongRunningEnlistment", "WorkingDirectoryTests.cs");
Expand Down Expand Up @@ -72,6 +112,7 @@ public void CanReadUnhydratedFileInParallelWithoutTearing()
}

[TestCaseSource(typeof(FileSystemRunner), FileSystemRunner.TestRunners)]
[Order(3)]
public void CanReadWriteAFileInParallel(FileSystemRunner fileSystem)
{
string fileName = @"CanReadWriteAFileInParallel";
Expand Down