Avoid UI dependency in CloudCache by passing along the solution folder #56829
Conversation
| disposable.Dispose(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
andrew informed us that our ICloudCache proxy will only expose IDisposable. So our consumption code only needs to handle that and not IAsyncDisposable. This greatly simplifies a ton of code.
src/VisualStudio/Core/Def/Storage/AbstractCloudCachePersistentStorageService.cs
Show resolved
Hide resolved
| VisualStudioServices.VS2019_10.CacheService, | ||
| // replace with CacheService.RelativePathBaseActivationArgKey once available. | ||
| new ServiceActivationOptions { ActivationArguments = ImmutableDictionary<string, string>.Empty.Add("RelativePathBase", solutionFolder) }, | ||
| cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
this is the crux of hte change, where we pass the solution folder along as an activation arg.
| var cacheService = await serviceBroker.GetProxyAsync<ICacheService>(VisualStudioServices.VS2019_10.CacheService, cancellationToken: cancellationToken).ConfigureAwait(false); | ||
| var cacheService = await serviceBroker.GetProxyAsync<ICacheService>( | ||
| VisualStudioServices.VS2019_10.CacheService, | ||
| // replace with CacheService.RelativePathBaseActivationArgKey once available. |
There was a problem hiding this comment.
I don't think you reference the cache service package, do you? Everything you use I think you get from VS.RPC.Contracts. I don't know that we'll ever declare a constant for this there. At least, there's no great place for it.
There was a problem hiding this comment.
At least, there's no great place for it.
Can't it be in a type exported from teh same assembly that exports ICacheService? You could also just make it a constant inside the ICacheService interface (though i don't love that).
There was a problem hiding this comment.
The assembly that exports ICacheService is the VS.RPC.Contracts assembly that I mentioned above.
Interfaces can't declare constants, last I checked. We would need to declare the constant on some other class in that assembly.
There was a problem hiding this comment.
Interfaces can declare constants. :-)
There was a problem hiding this comment.
Cool. But in trying this out, C# 8 is required, and C# forbids constants on interfaces except when targeting .NET Core 3.1 or later. VS.RPC.Contracts targets netstandard2.0 as net472 is still of course an important target.
There was a problem hiding this comment.
Are you guys not on C#8? :) I didn't know about teh netstandard issue. That certainly seems weird as netstandard should support this. Bummer.
There was a problem hiding this comment.
Yes, we absolutely leverage C# 8 -- C# 9, in fact. I don't mind using syntax that requires C# 9. But ya, your compiler forbids constants on interfaces except when targeting a runtime that supports the default method implementations feature.
| } | ||
|
|
||
| protected sealed override async ValueTask<ICacheService> CreateCacheServiceAsync(CancellationToken cancellationToken) | ||
| protected sealed override async ValueTask<ICacheService> CreateCacheServiceAsync(string solutionFolder, CancellationToken cancellationToken) | ||
| { | ||
| var serviceContainer = await _serviceProvider.GetServiceAsync<SVsBrokeredServiceContainer, IBrokeredServiceContainer>().ConfigureAwait(false); |
There was a problem hiding this comment.
Where does the switch from WaitAndGetResult to JTF.Run go so that this line won't deadlock?
There was a problem hiding this comment.
There's the original change in SuggestedAction.cs.
|
@jinujoseph for review. this addresses the deadlock we're getting with CloudCache. I'd like to bring this into 17.0 so we can reenable that A/B test. |
CyrusNajmabadi
force-pushed
the
cloudCacheFolder
branch
from
4702141 to
dbe16ed
Compare
|
The cloudcache side of this change will be inserted with this PR. |
ghost
added this to the 
This allows CloudCache to avoid having to go retrieve the same information from teh UI thread from IVsSolution.