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

A lock file implementation applied to project GitVersionCore for resolving #1031 #2333

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions src/GitVersionCore.Tests/Helpers/TestFileSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ public IEnumerable<string> DirectoryEnumerateFiles(string directory, string sear
throw new NotImplementedException();
}

public FileStream Open(string path, FileMode mode, FileAccess access, FileShare share)
{
return new FileStream(path, mode, access, share);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TestStream is deriving from Stream, but need Lock implementation of FileStream for LockFileApi. So when needed, another Stream wrapper have to be written for FileStream.

}

public Stream OpenWrite(string path)
{
return new TestStream(path, this);
Expand Down
1 change: 1 addition & 0 deletions src/GitVersionCore/Core/Abstractions/IFileSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public interface IFileSystem
void WriteAllText(string file, string fileContents);
void WriteAllText(string file, string fileContents, Encoding encoding);
IEnumerable<string> DirectoryEnumerateFiles(string directory, string searchPattern, SearchOption searchOption);
FileStream Open(string path, FileMode mode, FileAccess access, FileShare share);
Stream OpenWrite(string path);
Stream OpenRead(string path);
void CreateDirectory(string path);
Expand Down
5 changes: 5 additions & 0 deletions src/GitVersionCore/Core/FileSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ public IEnumerable<string> DirectoryEnumerateFiles(string directory, string sear
return Directory.EnumerateFiles(directory, searchPattern, searchOption);
}

public FileStream Open(string path, FileMode mode, FileAccess access, FileShare share)
{
return File.Open(path, mode, access, share);
}

public Stream OpenWrite(string path)
{
return File.OpenWrite(path);
Expand Down
7 changes: 7 additions & 0 deletions src/GitVersionCore/FileLocking/Abstractions/IFileLock.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
using System;

namespace GitVersion.FileLocking
{
public interface IFileLock : IDisposable
{ }
}
7 changes: 7 additions & 0 deletions src/GitVersionCore/FileLocking/Abstractions/IFileLocker.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
namespace GitVersion.FileLocking
{
public interface IFileLocker
{
FileLockUse WaitUntilAcquired();
}
}
76 changes: 76 additions & 0 deletions src/GitVersionCore/FileLocking/FileLockContext.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
using System;
using System.Diagnostics;
using System.IO;
using System.Threading;

namespace GitVersion.FileLocking
{

#nullable enable

internal class FileLockContext
{
public FileStream? FileStream { get; }
public Exception? Error { get; }
public ManualResetEvent? ErrorUnlockDone { get; }

private readonly FileLocker fileLocker;
private object? decreaseLockUseLocker;

private FileLockContext(FileLocker fileLocker, object decreaseLockUseLocker)
{
this.fileLocker = fileLocker ?? throw new ArgumentNullException(nameof(fileLocker));
this.decreaseLockUseLocker = decreaseLockUseLocker;
}

public FileLockContext(FileLocker fileLocker, object decreaseLockUseLocker, FileStream fileStream)
: this(fileLocker, decreaseLockUseLocker)
{
fileStream = fileStream ?? throw new ArgumentNullException(nameof(fileStream));
FileStream = fileStream;
}

public FileLockContext(FileLocker fileLocker, object decreaseLockUseLocker, Exception error, ManualResetEvent errorUnlockDone)
: this(fileLocker, decreaseLockUseLocker)
{
Error = error ?? throw new ArgumentNullException(nameof(error));
ErrorUnlockDone = errorUnlockDone ?? throw new ArgumentNullException(nameof(errorUnlockDone));
}

public void DecreaseLockUse(bool decreaseToZero, string lockId)
{
if (FileStream == null) {
throw new InvalidOperationException("You cannot decrease lock use when no file stream has been assgined.");
}

var decreaseLockUseLocker = this.decreaseLockUseLocker;
Copy link
Member

Choose a reason for hiding this comment

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

This assignment seems unnecessary and makes me think twice about what the below lock statement does. Does it lock the class field or the local variable?

Copy link
Contributor Author

@teneko teneko Jun 28, 2020

Choose a reason for hiding this comment

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

It does lock the Unlock function. You will see that there is another lock in FileLocker in Unlock. These calls can interfer each other and can lead to race conditions like the comment does tell you.


if (decreaseLockUseLocker == null)
return;

// Why surround by lock?
// There is a race condition, when number of file lock uses
// is decrased to 0. It may not have invalidated the file
// stream yet. Now it can happen that the number of file lock
// uses is increased to 1 due to file lock, but right after another
// file unlock is about to decrease the number again to 0.
// There is the possiblity that the actual file lock gets released
// two times accidentally.
lock (decreaseLockUseLocker)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lock (decreaseLockUseLocker)
lock (this.decreaseLockUseLocker)

{
if (!(FileStream.CanRead || FileStream.CanWrite))
{
Trace.WriteLine($"{FileLocker.CurrentThreadWithLockIdPrefix(lockId)} Lock use has been invalidated before. Skip decreasing lock use.", FileLocker.TraceCategory);
return;
}

var locksInUse = fileLocker.DecreaseLockUse(decreaseToZero, lockId);

if (0 == locksInUse)
{
this.decreaseLockUseLocker = null;
}
}
}
}
}
16 changes: 16 additions & 0 deletions src/GitVersionCore/FileLocking/FileLockContextExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
namespace GitVersion.FileLocking
{

#nullable enable

internal static class FileLockContextExtensions
{
public static bool IsErroneous(this FileLockContext? fileLockContext)
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this just an instance method on the FileLockContext class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the parameter "fileLockContext" can be null. With this "trick" you can shift the null check to this method. That's simply a shortcut.

{
if (fileLockContext?.Error is null)
return false;

return true;
}
}
}
36 changes: 36 additions & 0 deletions src/GitVersionCore/FileLocking/FileLockUse.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
using System;
using System.ComponentModel;
using System.IO;

namespace GitVersion.FileLocking
{

#nullable enable

public struct FileLockUse : IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand what FileLockUse does by its name. Can you please explain so we can perhaps figure out a more self-descriptive and intuitive name for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It describes the use of a file lock. As one lock is shared by many I thought the term "use" of one file lock would be somewhat self-explanatory.

{
public FileStream FileStream => fileLockContext.FileStream!;

private readonly FileLockContext fileLockContext;
[EditorBrowsable(EditorBrowsableState.Never)]
public readonly string LockId;

internal FileLockUse(FileLockContext fileLockContext, string LockId)
{
this.fileLockContext = fileLockContext ?? throw new ArgumentNullException(nameof(fileLockContext));

if (fileLockContext.FileStream is null)
{
throw new ArgumentException("File stream context has invalid file stream.");
}

this.LockId = LockId;
}

public void Dispose()
{
// When stream not closed, we can decrease lock use.
fileLockContext.DecreaseLockUse(false, LockId);
}
}
}
Loading