View changed submodule could hang GE

[gerhardol]

Temporary fix for #8914 (not closing #8914)

The proper fix is in #8923, but that PR introduces some test instabilities so the app code is patched instead.

Proposed changes

Do not hang the application when submodule status is not evaluated when viewing.

Test methodology

Manual.
You may have to insert the error by installing Symantec or adding a delay to submodule status calculation.

---

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

[mstv]

Perhaps:

Suggested change

	var status = ThreadHelper.JoinableTaskFactory.Run(
	() => file.GetSubmoduleStatusAsync() ?? Task.FromResult<GitSubmoduleStatus?>(null));
	GitSubmoduleStatus? status = file.GetSubmoduleStatusAsync()?.GetAwaiter().GetResult();

BTW: GetSubmoduleStatusAsync turned into a property would avoid the misleading call ().

[gerhardol]

I used what @drewnoakes suggested
This is hopefully a short term fix only
Will test this some time...

[gerhardol]

That change hangs GE so I will not change

[gerhardol]

@msftbot merge in 24 hours

[ghost]

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I won't merge this pull request until after the UTC date Thu, 06 May 2021 21:54:18 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[gerhardol]

The tests timed out similar to #8923 so I reverted to my original workaround that seem to be OK.

[gerhardol]

I have rebuilt this on AppVeyor six times now, with #8923 ish solutions the builds failed 2 out of 3 so I believe this solution is OK (it is similar to pre #8881 that introduced the issue).

Note that the check is changed, always entering the if(isSubmodule), not checking for task.
This is how the current codepaths are implemented, the submodule status tasks are always started (with a minor delay), so the task is never null.
If task is not started, the patch will evaluate the task, currently unused.

The proper change should restore the assumption that the task is not evaluated though.

[mstv]

Looks better.
Since this will be reworked anyway, we could continue the discussion after merging.

[mstv]

1. Rant: The syntax of ! is weird in this case. It applies to the return value of the function file.GetSubmoduleStatusAsync, which is not called here - but later by Run.
2. Are we sure that file._submoduleStatus is never null?

---

3. Does JoinAsync() always return the same Task instance? GetSubmoduleStatusAsync() is called pretty often.

[gerhardol]

1. This is compiler info to ignore that the value may be null, it does not matter that function is not called
2. GetSubmoduleStatus is currently always set. Anyway, this is a fix for the current solution that hangs if status is not yet ready
3. Do not know, I assume so

[gerhardol]

Annoying that the branch was merged also when auto merge was removed, a fixup! was merged. (block-fixup should be enough to stop this)
But it was just review and squash that was missing

[RussKie]

I'll check with the msftbot team.