Migrate to new output window API #38653
Conversation
src/VisualStudio/VisualStudioDiagnosticsToolWindow/Loggers/OutputWindowLogger.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/VisualStudioDiagnosticsToolWindow/Loggers/OutputWindowLogger.cs
Outdated
Show resolved
Hide resolved
|
Close/open to re-trigger tests. |
dibarbet
changed the title
|
@heejaechang @sharwell ready for review |
|
The integration tests are failing as the roslyn package is failing to load. There is a new dependency on the IBrokeredServiceContainer that is not available in 16.3P4. So until the integration testing machines are on 16.4P1, this cannot be merged. |
| @@ -91,6 +92,7 @@ | |||
| <MicrosoftNuGetBuildTasksVersion>0.1.0</MicrosoftNuGetBuildTasksVersion> | |||
| <MicrosoftPortableTargetsVersion>0.1.2-dev</MicrosoftPortableTargetsVersion> | |||
| <MicrosoftServiceHubClientVersion>2.0.48</MicrosoftServiceHubClientVersion> | |||
| <MicrosoftServiceHubFrameworkVersion>2.0.84</MicrosoftServiceHubFrameworkVersion> | |||
There was a problem hiding this comment.
Should this be kept in sync with MicrosoftServiceHubClientVersion? (Not sure if they are "related" versions or not...)
There was a problem hiding this comment.
I don't think it's necessary, and there are binding redirects in VS for both anyway.
There was a problem hiding this comment.
You should link these two. Hubclient and Framework are changing some in tandem and you may suffer regressions if they are out of sync for your tests at runtime.
|
|
||
| _asyncListener = asyncListenerProvider.GetListener(FeatureAttribute.OutputWindowLogger); | ||
| _threadingContext = threadingContext; | ||
| _serviceBrokerClient = new ServiceBrokerClient(serviceBroker, _threadingContext.JoinableTaskFactory); |
There was a problem hiding this comment.
Is there any overhead of creating multiple Clients, or is there some benefit of sharing things?
There was a problem hiding this comment.
Good question, I'll defer to Andrew
@AArnott is there any issue with sharing a single service broker client to retrieve various distinct services? Is there any benefit to doing that?
There was a problem hiding this comment.
A ServiceBrokerClient is totally fine with you requesting several services from it. It's optimized for that. What you shouldn't do is share your ServiceBrokerClient with a lot of users/classes if you can avoid it. The AvailabilityChanged event can get noisy if it's raising events to a lot of listeners for services that aren't related to an individual handler. So basically if you're setting up multiple handlers for that event to inform different classes that all consume from the same ServiceBrokerClient, consider creating a separate ServiceBrokerClient (and each with its own IServiceBroker passed to it) for each of these classes.
| var asyncToken = asyncListener.BeginAsyncOperation(nameof(InitializeWorkspaceFailureOutputWindowAsync)); | ||
| Task.Run(async () => | ||
| { | ||
| using var outputChannelStore = await serviceBrokerClient.GetProxyAsync<IOutputChannelStore>(VisualStudioServices.VS2019_4.OutputChannelStore).ConfigureAwait(false); |
There was a problem hiding this comment.
What is this using syntax? I've never seeing a using block that was missing a parentheses around the expression to be disposed.
Regardless, this looks buggy. It appears you're acquiring a rental and disposing it immediately, only to then try to use the proxy that you just released the rental for. You should make sure the using block includes all uses of the rented proxy before disposing the rental.
There was a problem hiding this comment.
It's a new C# 8 feature, so it's possible I'm using it incorrectly - https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using
But as far as I understand it, the lifetime of the variable defined in the implicit using will be the scope where it is declared, and once it goes out will be disposed of properly.
There was a problem hiding this comment.
(Actual non-proposal link) - https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-8#using-declarations
There was a problem hiding this comment.
That's awesome. Thanks for teaching me more about C# 8. In that case what you're doing here looks fine. I'm going to go simplify all my code now. :)
|
I want to call out that the API for the output window service that you're switching to is in flux. You can see what we're switching to here: https://dev.azure.com/devdiv/DevDiv/_git/VS.RPC.Contracts/pullrequest/202619?_a=overview So I'd recommend you not complete this PR till we complete our PR and you've compensated for the changes. |
Sounds good. I'll hold off for now, thanks for letting me know (and the comments!) |
Use the new output window API