Switch to installing/uninstalling nuget packages in the BG instead of the UI thread#55671
Switch to installing/uninstalling nuget packages in the BG instead of the UI thread#55671CyrusNajmabadi merged 9 commits intodotnet:mainfrom
Conversation
| private async Task<T?> PerformNuGetProjectServiceWorkAsync<T>( | ||
| Func<INuGetProjectService, CancellationToken, ValueTask<T?>> doWorkAsync, CancellationToken cancellationToken) | ||
| { | ||
| await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); |
There was a problem hiding this comment.
i don't know if this is necessary. but i was trying to preserve prior semantics. @AArnott do you need to be on the UI thread to call .GetServiceAsync to get the IBrokeredServiceContainer?
There was a problem hiding this comment.
No. GetServiceAsync can be called from a background thread. But it's generally not safe to cast the result as you're doing unless you are on the main thread or know that the service object is an MTA object. In this case, it is an MTA so you're fine.
src/VisualStudio/Core/Def/Packaging/PackageInstallerServiceFactory.cs
Outdated
Show resolved
Hide resolved
| private async Task<T?> PerformNuGetProjectServiceWorkAsync<T>( | ||
| Func<INuGetProjectService, CancellationToken, ValueTask<T?>> doWorkAsync, CancellationToken cancellationToken) | ||
| { | ||
| await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); |
There was a problem hiding this comment.
No. GetServiceAsync can be called from a background thread. But it's generally not safe to cast the result as you're doing unless you are on the main thread or know that the service object is an MTA object. In this case, it is an MTA so you're fine.
| { | ||
| await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); | ||
|
|
||
| var serviceContainer = (IBrokeredServiceContainer?)await _asyncServiceProvider.GetServiceAsync(typeof(SVsBrokeredServiceContainer)).ConfigureAwait(false); |
There was a problem hiding this comment.
But because of that caveat, we recommend folks switch to using the extension method:
| var serviceContainer = (IBrokeredServiceContainer?)await _asyncServiceProvider.GetServiceAsync(typeof(SVsBrokeredServiceContainer)).ConfigureAwait(false); | |
| IBrokeredServiceContainer? serviceContainer = await _asyncServiceProvider.GetServiceAsync<SVsBrokeredServiceContainer, IBrokeredServiceContainer>(throwOnFailure: false).ConfigureAwait(false); |
There was a problem hiding this comment.
No. GetServiceAsync can be called from a background thread. But it's generally not safe to cast the result as you're doing unless you are on the main thread or know that the service object is an MTA object. In this case, it is an MTA so you're fine.
Ah ok.
There was a problem hiding this comment.
In this case, it is an MTA so you're fine.
@AArnott I wasn't able to find the documentation for this. Can you link it?
But because of that caveat, we recommend folks switch to using the extension method
The last time we discussed this, the extension method is incompatible with unit testing because we cannot provide the correct JTF instance (ThreadHelper cannot be used in test suites that rely on state reset for test isolation). Has this been corrected?
There was a problem hiding this comment.
Has this been corrected?
No, I don't think so. I use vssdktestfx which sets the statics so that it works, but I respect the goal of pure test isolation and we should offer an overload that I guess takes a JoinableTaskContext object.
| workspace, _document.Id, _source, _packageName, | ||
| _versionOpt, _includePrerelease, cancellationToken); | ||
| _versionOpt, _includePrerelease, | ||
| progressTracker, cancellationToken); |
There was a problem hiding this comment.
once #55682 goes in, this can be async end to end.
src/VisualStudio/Core/Def/Packaging/PackageInstallerServiceFactory_UndoRedo.cs
Outdated
Show resolved
Hide resolved
| // explicitly switch to BG thread to do the installation as nuget installer APIs are free threaded. | ||
| await TaskScheduler.Default; |
There was a problem hiding this comment.
💡 This is going to switch to the background thread and then immediately switch back to the main thread to update the status bar. This line should be moved down to right before the tryUninstallPackage.
There was a problem hiding this comment.
seems reasonable. but in that case, why do we need the switch at all? the UpdateStatusBarAsync call has CA(false) on it. So we'll be off hte UI thread then, right?
There was a problem hiding this comment.
Overall the change is fine as a refactoring. Since we are blocked on NuGet/Home#11021 for the current form, I recommend the following:
- Add explicit calls to
SwitchToMainThreadAsyncbefore the NuGet operations which are not yet documented (there are three locations for this which are given as comments above) - Retitle this pull request to indicate that it is a refactoring, since it will no longer be changing the threads used for installation/uninstallation of packages
- File a follow-up issue to change the
SwitchToMainThreadAsynccalls over toTaskScheduler.Defaultonce the blocking issue is addressed
This approach will expedite this pull request by side-stepping the blocking issue.
|
:'( |
sharwell
left a comment
There was a problem hiding this comment.
Looks good now that NuGet/NuGet.Client#4209 is moving forward
|
@sharwell do you want me to wait on NuGet/NuGet.Client#4209 being merged? or are you ok knowing it's in progress? |
|
NuGet/NuGet.Client#4209 merged in. Merging this in now. |
Fixes AB#959448
Blocked on NuGet/Home#11021 where Nuget documents that what we are doing is explicitly ok.