Improve robustness of GitStatusMonitor #10743

mstv · 2023-02-24T22:03:38Z

Proposed changes

GitStatusMonitor: Correctly handle cancellation
- There is no need to stop the monitor on OperationCanceledException.
- There is no more command running if cancelled.
GitStatusMonitor: Stop only if retry fails, too
- E.g. I have seen the git index being locked perhaps due to VS Code getting the status.
GitStatusMonitor: Call InvalidateGitWorkingDirectoryStatus(), too, on enter of GitStatusMonitorState.Stopped

Test methodology

manual

Test environment(s)

Git Extensions 33.33.33
Build cf19d8d
Git 2.39.2.windows.1
Microsoft Windows NT 10.0.19045.0
.NET 6.0.12
DPI 96dpi (no scaling)
Microsoft.WindowsDesktop.App Versions

    Microsoft.WindowsDesktop.App 6.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

Please do not squash merge this PR

✒️ I contribute this code under The Developer Certificate of Origin.

gerhardol · 2023-02-24T22:31:07Z

GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs

@@ -469,10 +473,7 @@ private void Update()
                                    }
                                }

-                                if (!cancelToken.IsCancellationRequested)


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.

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?

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.

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

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.

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

GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs

gerhardol

+1
Leave it to someone else to review the comment I had and to approve. Otherwise fine, have not run.

gerhardol · 2023-03-08T22:27:45Z

+1
Leave it to someone else to review the comment I had and to approve. Otherwise fine, have not run.

I still have the opinion that the addition on line 440 is the correct.
Removal of _commandIsRunning on line 458/462 may just delay the next update.
IsCancelled check removal on line 472 may cause another parallel Git command
Will approve without these changes, but will not block if someone else approves.

gerhardol · 2023-03-10T22:22:10Z

GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs

@@ -455,7 +490,7 @@ private void Update()
                        {
                            lock (_statusSequence)
                            {
-                                if (_commandIsRunning && !ModuleHasChanged() && !cancelToken.IsCancellationRequested)
+                                if (!ModuleHasChanged() && !cancelToken.IsCancellationRequested)


What about the following?
Keep line 521; _commandIsRunning = false;

Suggested change

if (!ModuleHasChanged() && !cancelToken.IsCancellationRequested)

if (cancelToken.IsCancellationRequested)

{

return;

}

if (!ModuleHasChanged())

I haven't had the time yet to think about your comments in detail. But I have watched it for long time and think to know two things for sure:

The GitStatusMonitor stops doing anything because of _commandIsRunning is true (only resolvable by switching repos).

If we reach this finally block, GitExecutable.ExecuteAsync() has finished or been cancelled, i.e. nothing is pending in GE.

What have I missed?

The git.exe might continue to run detached in the background. We do not need to take care, don't we?
(I started to send Ctrl+C and to terminate the process on timeout - unfinished.)

Keep line 521; _commandIsRunning = false;

I understand your concern about the delay. I need to take time thinking about how to do it without mixing states.

Suggestion in fa36ff1
(_commandIsRunning is more closely related to the cancellation token).

gerhardol

Looks fine, basically what I have been using the last days.
Have not tried to provoke errors though.

gerhardol · 2023-03-19T17:13:33Z

The monitor stopped running once for me (in wsl), no real stress test either.
Just that someone else understands the code is a reason to merge.

RussKie

The naming inhibits my ability to fully grasp the change.

RussKie · 2023-03-19T22:55:06Z

GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs

+        /// <summary>
+        /// git-status command is running that is not cancelled
+        /// </summary>
+        private bool _uncancelledCommandIsRunning;


"uncancelled" this looks very odd, ditto for "is running that is not cancelled" - this sounds like an oxymoron...

"is running that is not cancelled" - this sounds like an oxymoron...

In a few more words: git-statis is started and the status is nether completed (with success or error) or cancelled explicitly. If false, a new git-status can be started.
I do not have a better description. the variably name can be composed from the words in the description too.

Why the original name didn't work?

@mstv suggested it was a clarification, I can live with that quirky name

The only clear meaning of a _commandIsRunning would be:

try { _commandIsRunning = true; RunTheCommand() } finally { _commandIsRunning = false; }

without any conditions.
If it deviates from this scheme, it should be reflected in the identifier - not hidden in the docs.
It should be clear, why this flag must not be reset in case of cancellation. Cancelled commands are detached and ignored. We don't want to wait for their cancellation and start the next command in parallel.
_commandIsRunningAndUncancelled might be clearer.

The renaming is not a behavior change. It is a clarification. The missing edge-case handling is completed.

"Uncancelled" is triggering me :) It's so Unenglish (pun intended).
How about _commandRunningWithoutCancellation or _commandIsRunningAndNotCancelled?

gerhardol · 2023-03-28T22:45:49Z

Time to merge?

mstv self-assigned this Feb 24, 2023

gerhardol reviewed Feb 24, 2023

View reviewed changes

RussKie reviewed Feb 25, 2023

View reviewed changes

GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs Outdated Show resolved Hide resolved

gerhardol reviewed Feb 25, 2023

View reviewed changes

mstv added 2 commits March 6, 2023 22:49

GitStatusMonitor: Correctly handle cancellation

cc9ac22

GitStatusMonitor: Stop only if fails repeatedly

3d6a12a

mstv force-pushed the fix/i9955_gitstatusmonitor branch from a1d7b20 to 3d6a12a Compare March 6, 2023 23:06

gerhardol reviewed Mar 10, 2023

View reviewed changes

mstv marked this pull request as draft March 11, 2023 08:25

gerhardol and others added 2 commits March 12, 2023 21:54

fixup! GitStatusMonitor: Correctly handle cancellation

6020b61

fixup! GitStatusMonitor: Correctly handle cancellation

9d0a1b4

mstv marked this pull request as ready for review March 15, 2023 21:38

mstv added the 👓 status: needs review label Mar 15, 2023

gerhardol approved these changes Mar 15, 2023

View reviewed changes

RussKie reviewed Mar 19, 2023

View reviewed changes

fixup! GitStatusMonitor: Correctly handle cancellation

a958a47

mstv force-pushed the fix/i9955_gitstatusmonitor branch from 224ef1a to a958a47 Compare March 21, 2023 18:04

mstv requested a review from RussKie March 27, 2023 20:02

RussKie approved these changes Mar 30, 2023

View reviewed changes

RussKie merged commit ee3f4c3 into gitextensions:master Mar 30, 2023

RussKie removed the 👓 status: needs review label Mar 30, 2023

ghost added this to the vNext milestone Mar 30, 2023

mstv deleted the fix/i9955_gitstatusmonitor branch March 30, 2023 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve robustness of GitStatusMonitor #10743

Improve robustness of GitStatusMonitor #10743

mstv commented Feb 24, 2023

gerhardol Feb 24, 2023

mstv Feb 24, 2023

gerhardol Feb 25, 2023

mstv Feb 25, 2023 •

edited

gerhardol Feb 25, 2023

mstv Mar 15, 2023

gerhardol left a comment

gerhardol commented Mar 8, 2023

gerhardol Mar 10, 2023

mstv Mar 11, 2023

gerhardol Mar 12, 2023

gerhardol left a comment

gerhardol commented Mar 19, 2023

RussKie left a comment

RussKie Mar 19, 2023

gerhardol Mar 20, 2023

RussKie Mar 20, 2023

gerhardol Mar 20, 2023

mstv Mar 20, 2023 •

edited

mstv Mar 20, 2023 •

edited

RussKie Mar 21, 2023

gerhardol commented Mar 28, 2023

-                                if (!ModuleHasChanged() && !cancelToken.IsCancellationRequested)
+                                if (cancelToken.IsCancellationRequested)
+                                {
+                                return;
+                                }
+                                if (!ModuleHasChanged())

Improve robustness of GitStatusMonitor #10743

Improve robustness of GitStatusMonitor #10743

Conversation

mstv commented Feb 24, 2023

Proposed changes

Test methodology

Test environment(s)

Merge strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstv Feb 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol left a comment

Choose a reason for hiding this comment

gerhardol commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol left a comment

Choose a reason for hiding this comment

gerhardol commented Mar 19, 2023

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstv Mar 20, 2023 • edited

Choose a reason for hiding this comment

mstv Mar 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Mar 28, 2023

mstv Feb 25, 2023 •

edited

mstv Mar 20, 2023 •

edited

mstv Mar 20, 2023 •

edited