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

#2578: Retry some file reads and writes to avoid 'file in use' on gitversion.json #2581

Merged
merged 15 commits into from
Feb 7, 2021

Conversation

MarkusHorstmann
Copy link
Contributor

@MarkusHorstmann MarkusHorstmann commented Feb 4, 2021

Description

Retry some file reads and writes to avoid 'file in use' on gitversion.json. Up to 6 attempts will be made on any IO exception, with exponential back-off between retries. The contentious operations typically write and read the entire gitversion.json file and tend to be sub-second on typical systems.

Also retry LibGit2Sharp.LockedFileException failures on some GIT operations.

Related Issue

Closes #2578.
Mitigates #1031.

Motivation and Context

With multi-targeted projects and solutions with many projects, there is read/write contention on the gitversion.json files, resulting in intermittent failures. See linked issue for one repro.

How Has This Been Tested?

Used in local builds on our repos. Reproduced failures like this, and confirmed the fix eliminates them. https://github.com/TRUMPF-IoT/cdePlugins/pull/23/checks?check_run_id=1827871101.

Checklist:

  • [x ] 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.
  • [ x] All new and existing tests passed.

@arturcic
Copy link
Member

arturcic commented Feb 4, 2021

@MarkusHorstmann we already have a class for retry mechanism, do you mind checking this one

var retryOperation = new OperationWithExponentialBackoff<IOException>(new ThreadSleep(), log, WriteCacheOperation, maxRetries: 6);
and update ?

@MarkusHorstmann
Copy link
Contributor Author

MarkusHorstmann commented Feb 4, 2021

@MarkusHorstmann we already have a class for retry mechanism, do you mind checking this one

var retryOperation = new OperationWithExponentialBackoff<IOException>(new ThreadSleep(), log, WriteCacheOperation, maxRetries: 6);

and update ?

Absolutely. Missed that helper.

2 questions (not familiar with the codebase so asking for guidance before I start):

  1. There is no logger in the context of the operations to be retried. OK to extend the retry mechanism to allow for no logger? Try to plumb a logger through? Implement a null logger?
  2. One of the operations returns a result. OK to add another retry helper with a Task return value?

@MarkusHorstmann
Copy link
Contributor Author

Took a stab at the above. Hope this is not too impactful.

src/GitVersion.MsBuild/GitVersionTaskExecutor.cs Outdated Show resolved Hide resolved
src/GitVersion.Core/GitVersion.Core.csproj Outdated Show resolved Hide resolved
src/GitVersion.Core/Model/VersionVariables.cs Outdated Show resolved Hide resolved
src/GitVersion.Core/Git/IReferenceCollection.cs Outdated Show resolved Hide resolved
@arturcic
Copy link
Member

arturcic commented Feb 5, 2021

Please rebase on top of main before continuing

@@ -53,10 +54,20 @@ public void Dispose()

public ICommit FindMergeBase(ICommit commit, ICommit otherCommit)
{
var mergeBase = repositoryInstance.ObjectDatabase.FindMergeBase((Commit)commit, (Commit)otherCommit);
return new Commit(mergeBase);
return new OperationWithExponentialBackoff<LockedFileException, ICommit>(new ThreadSleep(), log, () =>
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, do you think we can move the retry from the GitRepository and from the LibGit2Sharp implementation upper the stack to the consumer. That will make it more consistent.

@arturcic
Copy link
Member

arturcic commented Feb 7, 2021

@MarkusHorstmann thank you so much for your contributions 👍

@jetersen
Copy link
Contributor

jetersen commented Feb 7, 2021

Should this PR have closed #2410 or do we intend to introduce the file lock?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants