Ban incompatible service provider extension methods#60276
Ban incompatible service provider extension methods#60276sharwell merged 1 commit intodotnet:mainfrom
Conversation
| /// </exception> | ||
| public static TInterface GetService<TService, TInterface>( | ||
| this IServiceProvider serviceProvider, | ||
| JoinableTaskFactory joinableTaskFactory) |
There was a problem hiding this comment.
can we have some extensions that also take an IThreadingContext instead of hte JTF?
There was a problem hiding this comment.
I would prefer not. These are the extensions under consideration for future addition to the SDK, and matching signatures will make it easy to get used to this form and eventually remove the custom implementations.
There was a problem hiding this comment.
They can be in a different type :-). But I don't feel strongly. But it's more about the convenience for us.
| { | ||
| await joinableTaskFactory.SwitchToMainThreadAsync(); | ||
|
|
||
| var service = serviceProvider.GetService(typeof(TService)); |
There was a problem hiding this comment.
should this just call: GetServiceOnMainThread at this point?
There was a problem hiding this comment.
No. The error handling behavior is different.
GetServiceOnMainThread is a Roslyn-specific behavior. The IDE implementation of these GetService calls always use JTF.
| [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| public ServiceFactory(SVsServiceProvider serviceProvider) | ||
| => _serviceProvider = (IAsyncServiceProvider)serviceProvider; | ||
| public ServiceFactory(SVsServiceProvider serviceProvider, IThreadingContext threadingContext) |
There was a problem hiding this comment.
i'm so confused about what pattern we're supposed to use for importating. i see some code actually importing IAsyncServiceProvider above, so i don't know why we'd use SVsServiceProvider .
There was a problem hiding this comment.
These types are originally exported here:
https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform?path=/src/Shell/MefHosting/ComponentModelHostImpl/VsComponentModelHostExporter.cs&version=GBmain&line=23&lineEnd=30&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents
The import strategy depends on whether you are using MEF 1 or MEF 2 attributes (MEF 1 is more expressive here).
| await TaskScheduler.Default; | ||
|
|
||
| var componentModel = await AsyncServiceProvider.GlobalProvider.GetServiceAsync<SComponentModel, IComponentModel>().ConfigureAwait(false); | ||
| var componentModel = (IComponentModel?)await AsyncServiceProvider.GlobalProvider.GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(false); |
There was a problem hiding this comment.
whis is this safe, if we're running in the bG? doesn't the cast have the same issue?
There was a problem hiding this comment.
This is an old integration test outlier. It will eventually be removed by the migration to the new integration test project. In the meantime, it leverages the fact that IComponentModel is implemented by a managed service. If this was production code, I would require documentation that the SComponentModel service returns a free-threaded object before making this change.
There was a problem hiding this comment.
This was a good catch. There were a couple other places where I switched to the non-generic form prior to adding RoslynServiceExtensions, so I audited the PR for those and will include the fixes in the next push.
70e2598 to
798da18
Compare
ThreadHelper.JoinableTaskFactoryinternally)JoinableTaskFactoryas a parameter