-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make SSESessionManager ctor free threaded #4863
Conversation
} | ||
|
||
[Export(typeof(ISSESessionManager))] | ||
[Export(typeof(SSESessionManager))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont use this anywhere else so I removed the interface so it is the same as the TriggerImportInstall class
/// event from the ActiveSolutionBoundTracker) | ||
/// See https://github.com/SonarSource/sonarlint-visualstudio/issues/3886 | ||
/// </summary> | ||
private void LoadServicesAndDoInitialUpdates(IComponentModel componentModel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With how many there are now I thought it made sense to put it in a seperate method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc comment on the method needs to updated.
@@ -64,7 +64,7 @@ protected override async Task InitializeAsync(CancellationToken cancellationToke | |||
|
|||
logger.WriteLine(Resources.Package_Initializing); | |||
|
|||
sseSessionManager = componentModel.GetService<ISSESessionManager>(); | |||
LoadServicesAndDoInitialUpdates(componentModel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it up as on line 85 there is an await and this way it doesnt disturb the load order as much
A little confused about the dispose code smell as I didnt change anything on that, should I fix it? |
/// <summary> | ||
/// Reacts to project changes and opens/closes Server Sent Events sessions | ||
/// </summary> | ||
internal interface ISSESessionManager : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to make SSESessionManager : IDisposable
now (one of the code smells).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of minor comments.
SonarCloud Quality Gate failed. 0 Bugs 100.0% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
* Make SSESessionManager ctor free threaded * pr fixes
* Make SSESessionManager ctor free threaded * pr fixes
part of #4856