-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Extract async navigation portions of https://github.com/dotnet/roslyn/pull/55635 #55891
Conversation
FunctionId.CommandHandler_FindAllReference, | ||
KeyValueLogMessage.Create(LogType.UserAction, m => m["type"] = "streaming"), | ||
cancellationToken)) | ||
try |
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.
this is jus tmoving to a try/finally so all return paths call ONCompletedAsync.
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.
viewing this PR with whitespace off will help.
var definitions = interfaceImpls.SelectMany( | ||
i => GoToDefinitionHelpers.GetDefinitions( | ||
i, solution, thirdPartyNavigationAllowed: false, cancellationToken)).ToImmutableArray(); | ||
|
||
var title = string.Format(EditorFeaturesResources._0_implemented_members, |
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.
this entire synchronous codepath will hopefully go away in a future PR.
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 can't comment on the implications of this but that changes themselves seem okay ¯\_(ツ)_/¯
} | ||
|
||
if (presenter != null) | ||
{ | ||
// Can only navigate or present items on UI thread. | ||
await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); |
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.
@CyrusNajmabadi there's a bunch of ConfigureAwait(false)es after this -- expected?
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.
taken offline. yes :)
#55635 Is a bit large in the amount of change it causes (and potential risk it introduces). Thsi PR pulls out the portions of that PR that update several codepaths to be async and to avoid sync-over-async-over-sync-etc.