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

teneko · 2020-06-22T03:58:40Z

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

Description

Inject singleton service IFileLock to ServiceCollection. Lock gets disposed when the ServiceProvider is getting disposed too.

Related Issue

See #1031.

Motivation and Context

There were many failing builds in CI environments due to parallelism.

How Has This Been Tested?

Not yet tested...

Screenshots (if appropriate):

Checklist:

Receive opinions of maintainers and people of interests.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

teneko · 2020-06-22T04:10:41Z

I ask you, which timeout is necessary. I just took 15 seconds. I am worried about the fact, that the assemblies are often cached, but the servicecollection filled and serviceprovider built is always happening once in a task, so it will always dispose correct at end of any task in GitVersionTask.MSBuild assembly.

asbjornu

Thanks for starting to look into this, @teroneko! 🙏

I ask you, which timeout is necessary. I just took 15 seconds.

We have reports that GitVersion is taking more than a minute to complete, so I'm not sure 15 seconds is enough. #1448 (comment)

I am worried about the fact, that the assemblies are often cached, but the servicecollection filled and serviceprovider built is always happening once in a task, so it will always dispose correct at end of any task in GitVersionTask.MSBuild assembly.

If I understand you correctly, that's the source of the problem and why this is so hard to fix: The lock needs to exist and work across processes. Locking inside the GitVersion process won't help, since we have many instances of GitVersion running in parallel in different processes.

src/GitVersionCore/GitVersionCoreDefaults.cs

src/GitVersionCore/Helpers/LockFile.cs

teneko · 2020-06-22T09:34:52Z

At the evening I can spend some time I hope. Time is rare.

About your time consideration. So it could happen then, that one process is staling then for about 1 minute for wating to acquire the lock until it continues its IServiceCollection build process, otherwise a TimeoutException will be thrown.

teneko · 2020-06-22T09:52:47Z

I ask you, which timeout is necessary. I just took 15 seconds.

We have reports that GitVersion is taking more than a minute to complete, so I'm not sure 15 seconds is enough. #1448 (comment)

See comment above.

I am worried about the fact, that the assemblies are often cached, but the servicecollection filled and serviceprovider built is always happening once in a task, so it will always dispose correct at end of any task in GitVersionTask.MSBuild assembly.

If I understand you correctly, that's the source of the problem and why this is so hard to fix: The lock needs to exist and work across processes. Locking inside the GitVersion process won't help, since we have many instances of GitVersion running in parallel in different processes.

The point is we have to ensure, that each Task (from MSBuild) in GitVersionTask.MSbuild assembly is correctly disposing the singleton IFileLock. (in this current state of implementation - changed later). We should not rely on it, that Garbage Colelctor is doing it as some time, when built ServiceProvider is out of scope beside the fact, that the assembly can be cached and its automatical dispose is not guaranteed. So as you can see is IServiceProvider not implementing IDisposable, but ServiceProvider in assembly Microsoft.Extensions.DependencyInjection is doing so. So we have to return a derived IServiceProvider implementation that implements IDisposable and write a wrapper like I did for IFileLock. With such exposed custom IServiceProvider we can now manually dispose Microsoft.Extensions.DependencyInjection.ServiceProvider your just crafted at

GitVersion/src/GitVersionTask/GitVersionTasks.cs

Line 84 in 5330bbb

var sp = services.BuildServiceProvider();

. This Serviceprovider-dispose will also dispose any singleton that implements IDisposable (Here a reference). 🙂

asbjornu · 2020-06-22T14:11:26Z

The point is we have to ensure, that each Task (from MSBuild) in GitVersionTask.MSbuild assembly is correctly disposing the singleton IFileLock. (in this current state of implementation - changed later). We should not rely on it, that Garbage Colelctor is doing it as some time, when built ServiceProvider is out of scope beside the fact, that the assembly can be cached and its automatical dispose is not guaranteed.

Yes, I agree. We should explicitly dispose as soon as we are done indirectly poking the file system with LibGit2Sharp.

…an produce FileLockUses; FileLock wraps FileLockUse and implements IDisposable for being cleaned when disposing ServiceProvider

lgtm-com · 2020-06-24T00:29:22Z

This pull request introduces 1 alert when merging e6754e2 into 5330bbb - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

teneko

Should I implement it?
I don't feel responsible.

teneko · 2020-06-24T07:23:40Z

src/GitVersionCore.Tests/Helpers/TestFileSystem.cs

@@ -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);


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.

…executed action in assembly GitVersion.MSBuildTask

src/GitVersionCore/Core/Abstractions/IDisposableServiceProvider.cs

src/GitVersionCore/FileLocking/Abstractions/IFileLocker.cs

asbjornu

This is fantastic work, @teroneko! 👏 🎉

But as with any feature as big as this, there are things we need to discuss before this is ready to be merged. We also need to figure out how to test this.

src/GitVersionCore/FileLocking/Abstractions/IFileLocker.cs

src/GitVersionCore/Core/Abstractions/IDisposableServiceProvider.cs

src/GitVersionCore/FileLocking/Abstractions/IFileLock.cs

src/GitVersionCore/FileLocking/Abstractions/IFileLocker.cs

asbjornu · 2020-06-28T00:30:57Z

src/GitVersionCore/FileLocking/FileLockContext.cs

+                throw new InvalidOperationException("You cannot decrease lock use when no file stream has been assgined.");
+            }
+
+            var decreaseLockUseLocker = this.decreaseLockUseLocker;


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?

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.

asbjornu · 2020-06-28T00:33:29Z

src/GitVersionCore/FileLocking/FileLockUse.cs

+
+#nullable enable
+
+    public struct FileLockUse : IDisposable


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?

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.

asbjornu · 2020-06-28T00:36:24Z

src/GitVersionCore/FileLocking/ILockFileApi.cs

+
+namespace GitVersion.FileLocking
+{
+    public interface ILockFileApi


I don't quite understand the distinction between IFileLocker andILockFileApi. Why are they separated? I'm also not a fan of using Api in the name of an interface. All C# interfaces (and public classes) are application programming interfaces. :)

I decided to implement this in a composition flavor. When the "API" is about to have the ability to lock anything the FileLocker can lock a specific file that has the ability to lock one file but being callable by many concurrent calls to FileLocker.Lock(). So the file gets unlocked first when all current FileLockUse instances are got disposed. So later you can graduate the use of the instance of FileLocker to your behave when you decide to go away from one global lock.

src/GitVersionCore/FileLocking/LockFileApi.cs

src/GitVersionCore/Model/FileLock.cs

Remove because of false impression Co-authored-by: Asbjørn Ulsberg <[email protected]>

Remove because of false impresssion Co-authored-by: Asbjørn Ulsberg <[email protected]>

Remove due to implicit copyright Co-authored-by: Asbjørn Ulsberg <[email protected]>

Co-authored-by: Asbjørn Ulsberg <[email protected]>

Remove because of false impression Co-authored-by: Asbjørn Ulsberg <[email protected]>

…mespaces

teneko · 2020-06-28T02:44:41Z

When I get some free time I would set up your prepared repository. Currently I am just too busy.
The implementation itself I've tested with Xunit by myself. Perhaps they can be integrated here in GitVersion too?
So another the question is how to test GitVersionTask. The sub-projects should be in not that small. Perhaps we can take some big open source projects and include them just. :)

DerAlbertCom · 2020-07-08T10:40:44Z

Is there way to easy get a Version of the BuildTask with this fix in? (like a alpha nuget package) so that we can test this in the wild?

teneko · 2020-07-09T13:07:09Z

@DerAlbertCom Unfortunately I have no time because of business work. I can repeat the words of @asbjornu that you are free to assist in this pull request. But I'm looking forward to spend time as soon as I can. I'd like to see this feature too.

If you are really in need of a solution right now, you can use my experimental workaround. Use it only when you need pacakge level versioning (package name & .nuspec-file, that file that is is generated exclusively for package), but not compile level versioning (Assembly file). It does mimic GitVersionTask behaviour and supports concurrency with the above implementation. It does so by wrapping GitVersion.exe and locking file at path "<repository-root>/.gvc/index" when talking to GitVersion.exe. Use this package as you do with GitVersionTask.
The reason why I implemented this is, that I could not wait for an official fix that long, because my project is dependant of private projects.

asbjornu · 2020-07-31T21:30:04Z

@teroneko, I'm sorry to see this closed. Would a viable road ahead be to add this feature behind a configuration option so we can ship it for testing? Then if people report back that this actually fixes the problem, we can consider removing the configuartion in v6 and just make this the default.

riksking · 2020-08-12T10:15:53Z

Hi all.

Fix this, please, and give a pre-release for test.
This is a very unnerving mistake and I want to get rid of it as soon as possible.

stale · 2020-11-11T16:16:42Z

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

Tognolo · 2020-11-25T15:55:43Z

No stale please. Any news?

arturcic · 2020-11-25T16:37:22Z

Closing this one in favor of #2410

Initial commit

9ccf55c

teneko changed the title ~~Initial commit~~ A lock file implementation applied to project GitVersionCore for resolving #1031 Jun 22, 2020

asbjornu requested changes Jun 22, 2020

View reviewed changes

teneko added 2 commits June 24, 2020 01:05

LockFile API is now mockable; FileLocker implements IFileLocker and c…

38ee45f

…an produce FileLockUses; FileLock wraps FileLockUse and implements IDisposable for being cleaned when disposing ServiceProvider

Added default implementation for Open(..) in class TestFileSystem

e6754e2

Code tidied up and nullable enabled for files of file locking

d175f30

teneko commented Jun 24, 2020

View reviewed changes

teneko added 2 commits June 24, 2020 08:31

Increated timeout to 1 minute and 30 seconds

690087d

Implemented disposable service provider that get disposed after each …

081dec9

…executed action in assembly GitVersion.MSBuildTask

teneko requested a review from asbjornu June 25, 2020 23:04

arturcic reviewed Jun 26, 2020

View reviewed changes

src/GitVersionCore/Core/Abstractions/IDisposableServiceProvider.cs Outdated Show resolved Hide resolved

arturcic reviewed Jun 26, 2020

View reviewed changes

src/GitVersionCore/FileLocking/Abstractions/IFileLocker.cs Outdated Show resolved Hide resolved

asbjornu requested changes Jun 28, 2020

View reviewed changes

teneko and others added 7 commits June 28, 2020 03:51

Update src/GitVersionCore/FileLocking/Abstractions/IFileLock.cs

d5c806d

Remove because of false impression Co-authored-by: Asbjørn Ulsberg <[email protected]>

Update src/GitVersionCore/FileLocking/Abstractions/IFileLocker.cs

3759415

Remove because of false impresssion Co-authored-by: Asbjørn Ulsberg <[email protected]>

Update src/GitVersionCore/FileLocking/LockFileApi.cs

2432a8c

Remove due to implicit copyright Co-authored-by: Asbjørn Ulsberg <[email protected]>

Update src/GitVersionCore/Model/FileLock.cs

90e57d1

Co-authored-by: Asbjørn Ulsberg <[email protected]>

Apply suggestions from code review

a5edfba

Remove because of false impression Co-authored-by: Asbjørn Ulsberg <[email protected]>

Removed explicit copyright messages due to false impression; Fixed na…

d9cdcab

…mespaces

Removed IDisposableServiceProvider due to recommendations

426f329

asbjornu mentioned this pull request Jul 8, 2020

GitVersionTask Parallel restore issue when multi targeting #1381

Closed

teneko closed this Jul 25, 2020

asbjornu reopened this Jul 31, 2020

asbjornu mentioned this pull request Sep 23, 2020

Lock file #2410

Closed

stale bot added the stale label Nov 11, 2020

stale bot removed the stale label Nov 25, 2020

arturcic closed this Nov 25, 2020

teneko deleted the feature/lock-file branch December 5, 2020 15:06

teneko mentioned this pull request Apr 21, 2021

Add global named mutex to prevent concurrent use #2669

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

teneko commented Jun 22, 2020 •

edited

Loading

teneko commented Jun 22, 2020

asbjornu left a comment

teneko commented Jun 22, 2020

teneko commented Jun 22, 2020 •

edited

Loading

asbjornu commented Jun 22, 2020

lgtm-com bot commented Jun 24, 2020

teneko left a comment •

edited

Loading

teneko Jun 24, 2020

asbjornu left a comment

asbjornu Jun 28, 2020

teneko Jun 28, 2020 •

edited

Loading

asbjornu Jun 28, 2020

teneko Jun 28, 2020

asbjornu Jun 28, 2020

teneko Jun 28, 2020 •

edited

Loading

teneko commented Jun 28, 2020

DerAlbertCom commented Jul 8, 2020

teneko commented Jul 9, 2020 •

edited

Loading

asbjornu commented Jul 31, 2020

riksking commented Aug 12, 2020

stale bot commented Nov 11, 2020

Tognolo commented Nov 25, 2020

arturcic commented Nov 25, 2020

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

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

Conversation

teneko commented Jun 22, 2020 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

teneko commented Jun 22, 2020

asbjornu left a comment

Choose a reason for hiding this comment

teneko commented Jun 22, 2020

teneko commented Jun 22, 2020 • edited Loading

asbjornu commented Jun 22, 2020

lgtm-com bot commented Jun 24, 2020

teneko left a comment • edited Loading

Choose a reason for hiding this comment

teneko Jun 24, 2020

Choose a reason for hiding this comment

asbjornu left a comment

Choose a reason for hiding this comment

asbjornu Jun 28, 2020

Choose a reason for hiding this comment

teneko Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

asbjornu Jun 28, 2020

Choose a reason for hiding this comment

teneko Jun 28, 2020

Choose a reason for hiding this comment

asbjornu Jun 28, 2020

Choose a reason for hiding this comment

teneko Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

teneko commented Jun 28, 2020

DerAlbertCom commented Jul 8, 2020

teneko commented Jul 9, 2020 • edited Loading

asbjornu commented Jul 31, 2020

riksking commented Aug 12, 2020

stale bot commented Nov 11, 2020

Tognolo commented Nov 25, 2020

arturcic commented Nov 25, 2020

teneko commented Jun 22, 2020 •

edited

Loading

teneko commented Jun 22, 2020 •

edited

Loading

teneko left a comment •

edited

Loading

teneko Jun 28, 2020 •

edited

Loading

teneko Jun 28, 2020 •

edited

Loading

teneko commented Jul 9, 2020 •

edited

Loading