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.

Sign up for GitHub

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

Jump to bottom

Improve robustness of GitStatusMonitor #10743

Merged
merged 5 commits into from
Mar 30, 2023
Merged

Improve robustness of GitStatusMonitor #10743

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc9ac22
GitStatusMonitor: Correctly handle cancellation
mstv Feb 24, 2023
3d6a12a
GitStatusMonitor: Stop only if fails repeatedly
mstv Feb 24, 2023
6020b61
fixup! GitStatusMonitor: Correctly handle cancellation
gerhardol Mar 11, 2023
9d0a1b4
fixup! GitStatusMonitor: Correctly handle cancellation
mstv Mar 12, 2023
a958a47
fixup! GitStatusMonitor: Correctly handle cancellation
mstv Mar 21, 2023
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
99 changes: 73 additions & 26 deletions GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,24 @@ public sealed class GitStatusMonitor : IDisposable
/// </summary>
private const int PeriodicUpdateIntervalWSL = 60 * 1000;

/// <summary>
/// The number how often an update must fail in a row until the monitoring is stopped.
/// </summary>
private const int _maxConsecutiveErrors = 3;

/// <summary>
/// git-status command is running and no cancellation has been requested
/// </summary>
private bool _commandIsRunningAndNotCancelled;

/// <summary>
/// The number of consecutive update failures.
/// </summary>
private int _consecutiveErrorCount;

private readonly FileSystemWatcher _workTreeWatcher = new();
private readonly FileSystemWatcher _gitDirWatcher = new();
private readonly System.Windows.Forms.Timer _timerRefresh;
private bool _commandIsRunning;
private bool _isFirstPostRepoChanged;
private string? _gitPath;
private string? _submodulesPath;
Expand All @@ -50,10 +64,14 @@ public sealed class GitStatusMonitor : IDisposable
// Timestamps to schedule status updates, limit the update interval dynamically
// Note that TickCount wraps after 25 days uptime, always compare diff

// Next scheduled update time
/// <summary>
/// Next scheduled update time
/// </summary>
private int _nextUpdateTime;

// Earliest time for an scheduled update (interactive requests bypasses this)
/// <summary>
/// Earliest time for an scheduled update (interactive requests bypasses this)
/// </summary>
private int _nextEarliestTime;

private GitStatusMonitorState _currentStatus;
Expand Down Expand Up @@ -200,7 +218,9 @@ private GitStatusMonitorState CurrentStatus
get { return _currentStatus; }
set
{
var prevStatus = _currentStatus;
ThreadHelper.AssertOnUIThread();

GitStatusMonitorState prevStatus = _currentStatus;
_currentStatus = value;
switch (_currentStatus)
{
Expand All @@ -209,6 +229,21 @@ private GitStatusMonitorState CurrentStatus
_timerRefresh.Stop();
_workTreeWatcher.EnableRaisingEvents = false;
_gitDirWatcher.EnableRaisingEvents = false;
_consecutiveErrorCount = 0;

if (_currentStatus != prevStatus)
{
lock (_statusSequence)
{
if (_commandIsRunningAndNotCancelled)
{
_statusSequence.CancelCurrent();
_commandIsRunningAndNotCancelled = false;
}
}

InvalidateGitWorkingDirectoryStatus();
}
}

break;
Expand All @@ -224,7 +259,7 @@ private GitStatusMonitorState CurrentStatus

case GitStatusMonitorState.Inactive:
{
if (prevStatus != GitStatusMonitorState.Inactive)
if (_currentStatus != prevStatus)
{
InvalidateGitWorkingDirectoryStatus();
}
Expand All @@ -239,7 +274,7 @@ private GitStatusMonitorState CurrentStatus
// Timer is already running, schedule a new command only if a command is not running,
// to avoid that many commands are started (and cancelled) if quickly switching Inactive/Running
// If data has changed when Inactive it should be updated by normal means
if (!_commandIsRunning)
if (!_commandIsRunningAndNotCancelled)
{
ScheduleNextInteractiveTime();
}
Expand Down Expand Up @@ -393,7 +428,7 @@ private void Update()

lock (_statusSequence)
{
if (_commandIsRunning)
if (_commandIsRunningAndNotCancelled)
{
return;
}
Expand All @@ -408,24 +443,24 @@ private void Update()

// capture a consistent state in the main thread
module = Module;
cancelToken = _statusSequence.Next();
noLocks = !_isFirstPostRepoChanged;
cancelToken = _statusSequence.Next();
_commandIsRunningAndNotCancelled = true;

// Schedule periodic update, even if we don't know that anything changed
_nextUpdateTime = commandStartTime
+ (PathUtil.IsWslPath(_workTreeWatcher.Path) ? PeriodicUpdateIntervalWSL : PeriodicUpdateInterval);
_nextEarliestTime = commandStartTime + MinUpdateInterval;
_isFirstPostRepoChanged = false;
_commandIsRunning = true;
}

ThreadHelper.JoinableTaskFactory.RunAsync(
async () =>
{
try
{
GitExtUtils.ArgumentString cmd = GitCommandHelpers.GetAllChangedFilesCmd(true, UntrackedFilesMode.Default, noLocks: noLocks);
ExecutionResult result = await module.GitExecutable.ExecuteAsync(cmd, cancellationToken: cancelToken).ConfigureAwait(false);
GitExtUtils.ArgumentString cmd = GitCommandHelpers.GetAllChangedFilesCmd(excludeIgnoredFiles: true, UntrackedFilesMode.Default, noLocks: noLocks);
ExecutionResult result = await module.GitExecutable.ExecuteAsync(cmd, cancellationToken: cancelToken).ConfigureAwait(continueOnCapturedContext: false);

if (result.ExitedSuccessfully && !ModuleHasChanged())
{
Expand All @@ -436,11 +471,24 @@ private void Update()
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
GitWorkingDirectoryStatusChanged?.Invoke(this, new GitWorkingDirectoryStatusEventArgs(changedFiles));
}

_consecutiveErrorCount = 0;
}
catch (OperationCanceledException)
{
// No action
}
catch
{
try
{
if (++_consecutiveErrorCount < _maxConsecutiveErrors)
{
// Try again
ScheduleNextInteractiveTime();
return;
}

await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();

// Avoid possible popups on every file changes
Expand All @@ -455,24 +503,23 @@ private void Update()
{
lock (_statusSequence)
{
if (_commandIsRunning && !ModuleHasChanged() && !cancelToken.IsCancellationRequested)
if (!cancelToken.IsCancellationRequested)
{
// Adjust the min time to next update
int endTime = Environment.TickCount;
int commandTime = endTime - commandStartTime;
int minDelay = Math.Max(MinUpdateInterval, 2 * commandTime);
_nextEarliestTime = endTime + minDelay;
if (_nextUpdateTime - commandStartTime < _nextEarliestTime - commandStartTime)
_commandIsRunningAndNotCancelled = false;
if (!ModuleHasChanged())
{
// Postpone the requested update
_nextUpdateTime = _nextEarliestTime;
// Adjust the min time to next update
int endTime = Environment.TickCount;
int commandTime = endTime - commandStartTime;
int minDelay = Math.Max(MinUpdateInterval, 2 * commandTime);
_nextEarliestTime = endTime + minDelay;
if (_nextUpdateTime - commandStartTime < _nextEarliestTime - commandStartTime)
{
// Postpone the requested update
_nextUpdateTime = _nextEarliestTime;
}
}
}

if (!cancelToken.IsCancellationRequested)
Copy link
Member

Choose a reason for hiding this comment

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

Cancellation is done in ScheduleNextInteractiveTime(), then _commandIsRunning is set to false so a new command can be started.
This could allow starting a second command, not wanted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have a look at line 249, please.

This could allow starting a second command, not wanted.

There cannot be a command running here (it has finished or been cancelled).
What do you mean with "second command"? How could it be triggered?

Copy link
Member

Choose a reason for hiding this comment

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

See line 180, called from FormBrowse.

The Git command runs async, a second invocation can be done in the timer loop.
It occasionally occurred in 3.3 or so when git-status was slow, then git-status running in parallel used all resources.

Copy link
Member Author

@mstv mstv Feb 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Then rather a1d7b20 and let the cancellation do the job, i.e. let it reset _commandIsRunning in the finally block.

Copy link
Member

Choose a reason for hiding this comment

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

It may take longer time to start the next refresh (not starting the new until the first is cancelled), but no big difference and the code is easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have finally got how it was meant. So mainly renaming and some complements.

{
_commandIsRunning = false;
}
}
}
})
Expand Down Expand Up @@ -518,8 +565,8 @@ private void ScheduleNextInteractiveTime(int delay = InteractiveUpdateDelay)
// Start commands, also if running already
lock (_statusSequence)
{
_commandIsRunning = false;
_statusSequence.CancelCurrent();
_commandIsRunningAndNotCancelled = false;

int ticks = Environment.TickCount;
_nextEarliestTime = ticks + MinUpdateInterval;
Expand Down