From ac5f67ee23f2d7d3304062b28e7f29ce634319ff Mon Sep 17 00:00:00 2001 From: Chris Darroch Date: Fri, 4 Sep 2020 22:17:24 -0700 Subject: [PATCH] FunctionalTests: test fetch lock release on POSIX 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. --- .../Tests/EnlistmentPerFixture/FetchStepTests.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Scalar.FunctionalTests/Tests/EnlistmentPerFixture/FetchStepTests.cs b/Scalar.FunctionalTests/Tests/EnlistmentPerFixture/FetchStepTests.cs index e9115e67a70..8d3835e4093 100644 --- a/Scalar.FunctionalTests/Tests/EnlistmentPerFixture/FetchStepTests.cs +++ b/Scalar.FunctionalTests/Tests/EnlistmentPerFixture/FetchStepTests.cs @@ -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); @@ -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(); } } }