Skip to content

Commit

Permalink
FunctionalTests: test fetch lock release on POSIX
Browse files Browse the repository at this point in the history
The FetchStepCleansUpStaleFetchLock() functional test currently
only succeeds on Windows (and is therefore marked with the
MacTODO.TestNeedsToLockFile category) because it checks whether
the FetchStep command has released its lock file by testing if
the lock file has been deleted.

This works with our WindowsFileBasedLock implementation, which
opens the lock file with FileOptions.DeleteOnClose, so the file is
removed when the lock is released.  However, the MacFileBasedLock
version leaves the file in place and uses flock(2) and LOCK_EX
to acquire and release the lock atomically; there is no simple
way to delete the file while still holding the open file
descriptor needed for flock().

To make this test succeed on Mac/Linux while still being meaningful,
we need to be robust against the lock file existing both at the
start of the test and afterwards, while still ensuring that the
lock has been released by the FetchStep command.

We therefore simply attempt to open the lock file after the fetch
command completes, using the FileShare.None attribute, which will
fail on all platforms if the lock file has not been released.

On Windows this is because WindowsFileBasedLock uses FileShare.Read
to open the lock file, so if the lock is still held our attempt
with FileShare.None will fail.

On macOS (and Linux) this is because our use of flock() in
MacFileBasedLock will conflict with the flock() with LOCK_EX which
the .NET Core Unix implementation of FileShare.None also uses.

See the discussions in microsoft/VFSForGit#213, dotnet/runtime#24432,
as well as the .NET Core library implementation in:

https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28
aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L8
8

We also rename the test to FetchStepReleasesFetchLockFile() to
more accurately reflect what it is now testing.
  • Loading branch information
chrisd8088 committed Sep 5, 2020
1 parent 5aaba18 commit ac5f67e
Showing 1 changed file with 9 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,13 @@ public FetchStepTests()
}

[TestCase]
[Category(Categories.MacTODO.TestNeedsToLockFile)]
public void FetchStepCleansUpStaleFetchLock()
public void FetchStepReleasesFetchLockFile()
{
this.Enlistment.RunVerb("fetch");
string fetchCommitsLockFile = Path.Combine(
ScalarHelpers.GetObjectsRootFromGitConfig(this.Enlistment.RepoRoot),
"pack",
FetchCommitsAndTreesLock);
fetchCommitsLockFile.ShouldNotExistOnDisk(this.fileSystem);
this.fileSystem.WriteAllText(fetchCommitsLockFile, this.Enlistment.EnlistmentRoot);
fetchCommitsLockFile.ShouldBeAFile(this.fileSystem);

Expand All @@ -43,7 +41,14 @@ public void FetchStepCleansUpStaleFetchLock()
.ShouldEqual(1, "Incorrect number of .keep files in pack directory");

this.Enlistment.RunVerb("fetch");
fetchCommitsLockFile.ShouldNotExistOnDisk(this.fileSystem);

// Using FileShare.None ensures we test on both Windows, where WindowsFileBasedLock uses
// FileShare.Read to open the lock file, and on Mac/Linux, where the C# core libraries
// implement FileShare.None using flock(2) with LOCK_EX and thus will collide with our
// Mac/Linux FileBasedLock implementations which do the same, should the FetchStep
// have failed to release its lock.
FileStream stream = new FileStream(fetchCommitsLockFile, FileMode.OpenOrCreate, FileAccess.Read, FileShare.None);
stream.Dispose();
}
}
}

0 comments on commit ac5f67e

Please sign in to comment.