Remove the two-argument form of AsyncLoader.DoAsync<T> #4726
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4726 +/- ##
==========================================
- Coverage 28.5% 28.49% -0.01%
==========================================
Files 510 510
Lines 41080 41107 +27
Branches 5924 5929 +5
==========================================
+ Hits 11708 11713 +5
- Misses 28857 28879 +22
Partials 515 515
Continue to review full report at Codecov.
|
|
Looks like good stuff but there's some new APIs in there for me so I'm not sure I can give a proper review without some more research. |
GitUI/CommandsDialogs/FormBrowse.cs
Outdated
| (result) => toolStripSplitStash.Text = string.Format(_stashCount.Text, result, | ||
| result != 1 ? _stashPlural.Text : _stashSingular.Text)); | ||
| }); | ||
| await TaskScheduler.Default.SwitchTo(alwaysYield: true); |
There was a problem hiding this comment.
We are on UI thread here, can we drop alwaysYield: true?
There was a problem hiding this comment.
➡️ Yes, alwaysYield only has an effect if you are already on the target thread. I'll update both all three cases for this PR and add assertions to the calling method so the current thread is clear. (I made no other changes in the amended commit relative to the initial review.)
| @@ -961,6 +968,8 @@ private void UpdateStashCount() | |||
|
|
|||
| private void CheckForMergeConflicts() | |||
| { | |||
| ThreadHelper.ThrowIfNotOnUIThread(); | |||
There was a problem hiding this comment.
I would like to know little more about the need of putting this assertion. By the first rule we should put it when we require to be on the UI thread. Every method that accesses UI controls has to meet such a requirement. Should all that methods include this assertion? I know that in this case when dropping alwaysYield: true you want to be sure that we are on the UI thread and there is no need to explicitly require to always yield. However, this method accesses UI controls so we can be pretty sure that it can be called only from the UI thread. On the other hand there is UpdateBranchNameDisplayAsync method which does not assert we are on the UI thread nor it requests to alwaysYield. I think we should create a wiki page that explains when it is required to assert that a method is running on UI thread.
There was a problem hiding this comment.
I'm torn on how to handle this in the long run. In #4728, an empty file was added called vs-threading.TypesRequiringMainThread.txt. One could imagine adding the following line to this file:
System.Windows.Forms.*
However, this produced an incredible burden for development (I think 7,000 new warnings, including many warnings for things that are not actually problematic). For now I'm trying to include the assertion when it's relevant in new code, but we haven't started to leverage its full capability yet - and probably shouldn't until we figure out the usability issues.
There was a problem hiding this comment.
which does not assert we are on the UI thread nor it requests to
alwaysYield
The use of alwaysYield is not characteristic of other cases where vs-threading is used. For us, it serves two purposes:
- It allowed me to preserve the behavior of code during a transition to vs-threading. By preserving existing behavior, we did not have to examine each case in detail to determine if
alwaysYieldshould be specified or not. - It hides the latency we have in GitCommands, which does not implement asynchronous commands even though they are run in a separate process. Once GitCommands moves to async code, the need for
alwaysYieldwill likely disappear.
The two-argument form of
DoAsync<T>didn't seem to provide substantial advantages over JTF. This pull request converts the few uses to a more direct JTF approach and requiresDoAsync<T>users pass the third argument.📝 If accepted, please do not rebase or squash this pull request during the merge.