Add global named mutex to prevent concurrent use #2669
Conversation
|
@jetersen I saw that, but what I'm proposing is much simpler. There's no need for lock files, just need a named mutex to prevent concurrent usage. |
|
I see that there are some build failures, but I think that they are unrelated to my changes. They appear to be related to some sort of cake problem? I have built the code locally with |
|
Thanks @bording - a named mutex would also be my preferred solution to this, over lock files. File locking is difficult to do correctly with certain file systems throwing edge cases etc. Named mutexes are simpler (specifically designed for this use case) and should work on all supported platforms! So if this fixes the issue that would be a nice step forward imho! Notes on .net (core / 5) underlying mutex implementation:
|
On the other hand, I have used these before on both macOS and Linux, and they've worked just fine. If there is a problem with them, then that's a bug to file with Microsoft to fix in .NET. I don't really see the point of reinventing synchronization primitives when the framework and OS provides what we need here. |
| @@ -56,8 +57,16 @@ public int Execute(GitVersionOptions gitVersionOptions) | |||
|
|
|||
| private int RunGitVersionTool(GitVersionOptions gitVersionOptions) | |||
| { | |||
| var mutexName = gitVersionOptions.WorkingDirectory.Replace("\\", ""); | |||
There was a problem hiding this comment.
If there is no need to make them readable of some sort you could also just hash the path into a guid
|
Since the build is failing on the |
@asbjornu I don't really understand Cake, so I can't really follow what the build log failures mean. To me, it just looks like some unrelated failure. Can you point me to something that shows what's actually failing and maybe I can try to figure it out? |
|
I took another look at the failing build logs, and finally found the error message lurking in there. I have pushed up another commit that fixes the problem, and all the GitHub Action checks are green now. I see there is an Azure DevOps failure, but are those still relevant? |
|
No, Azure Pipelines are failing pretty often. I was considering to switch to only GitHub Actions, but for now keeping Azure Pipelines |
|
Thank you @bording for your contribution! |
|
@arturcic Thanks for merging this. Do you have an ETA for this being released? We're in the process of converting our repos to GitHub Actions and have been hitting the problem this PR solves quite a lot! |
|
@bording version 5.6.9 was released |
|
Hi, This looks like it is still causing concurrency problems. Now creating a lock file.
I updated to 5.6.9 and removed the workaround to enabled the parallelism in msbuild. Did I miss something? The stack on the other one is a little different, so here it goes:
|
|
@joaorosado Hmm, that is a bit surprising, and I'm not quite sure how that might be possible. I've rolled out the update on all of my repos, and it has indeed fixed the problem as I was expecting it to. I think I would need some way to repro it myself before I could understand how it's not working for you. |
|
What value does the gitVersionOptions.WorkingDirectory have? The command that fails looks like this, but the actual project that fails ("MyProjectName" here) is not always the same:
Could it be that gitVersionOptions.WorkingDirectory is different for each project in the solution? (I don't have much context on these tasks, just did a quick search ..so can be totally off target) |
|
I'm not sure what you'd be doing in your project to make that happen though. |
|
I don't see anything special on my solution. I'll try to build a version of GitVersion locally to attempt to debug. |
|
@bording just confirmed and the gitVersionOptions.WorkingDirectory is being called for each project with the folder of the project. WorkingDirectory is the project folder (passed as first argument) As for a repro, I just created a new Console app in visual studio with the default values.
Added gitversion to the project and compiled inside visual studio: Result: |
|
Something sounds wrong there. Per the documentation, the path parameter, which is what the
It should not be getting a per-project value for that. @arturcic Any insight on this? Why would the wrong value be passed in there? Is the documentation wrong? I based this PR on that understanding, and nothing I saw in the code made me expect the documentation was wrong. |
|
By the way the repositoryInfo object does have the correct information at this point:
|
|
@joaorosado The thing I want to make sure we avoid is actually creating any LibGit2Sharp types too early, outside of the mutex, to ensure there's no chance of any stale info being used when repository normalization occurs. I'll take a look at the code path you're showing to see if that looks safe to use. Otherwise, if WorkingDirectory actually isn't guaranteed to be the working directory of the repo from a git perspective, then another option would be to just make the mutex name something like I originally chose to use the WorkingDirectory because it seemed like a good value to scope the mutex down, since two instances running against different repos wouldn't conflict. |
@bording if you go that route it might be nice to detect the build Id provided by the build agent if such thing is available, otherwise falling back to the current branch name, last fallback Global\GitVersion |
|
Looks like |
When using GitVersion.MsBuild in multiple projects, you can get concurrent invocations of the GitVersion app. This can result in exceptions when normalization is run. This PR attempts to fix this problem by introducing a global mutex to ensure only a single instance of GitVersion is running at a time, per working directory.
Description
I have added a named mutex in the
GitVersionExecutor.RunGitVersionToolmethod, using theWorkingDirectoryvalue as the name. Giving it a name along with theGlobal\prefix means that the mutex will be shared across all processes on the machine.This ensures that for a given working directory, only one instance of GitVersion will be executing at a time.
NOTE: Mutex names cannot have a backslash in them, so that is why I am removing them before using it as the name.
Related Issues
#1381
#1031
Motivation and Context
This change is required to prevent the various exceptions that can occur when the normalization process is run concurrently, which is likely to happen when more than one project in a solution is using GitVersion.MsBuild.
These errors include:
LibGit2Sharp.LockedFileException: failed to lock fileLibGit2Sharp.NameConflictException: failed to write referenceHow Has This Been Tested?
Checklist: