-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Properly await calls to ensure exception handling works for sync and async scenarios #81826
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
Conversation
|
Fascinating. This tiny change massively affected razor. |
| { | ||
| return notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken); | ||
| await notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken).ConfigureAwait(false); | ||
| return; |
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.
note: this preserves thee old behavior. we were returning. so this would stop after the first hit. but i don't know if that's actually what we want here. note: if i don't return we get a high amount of memory use (presumably because we're issuing the refresh for a ton of cases. given that we dont' pass the docUri in, i'm guessing we do want tehse semantics.
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.
yes, the old behavior is correct. refresh notifications are typically server-wide, and don't apply to a specific document or project. Once we've sent one, we don't need to send any more as a single request indicates everything (for that feature) is out of date.
| { | ||
| return notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken); | ||
| await notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken).ConfigureAwait(false); | ||
| return; |
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.
yes, the old behavior is correct. refresh notifications are typically server-wide, and don't apply to a specific document or project. Once we've sent one, we don't need to send any more as a single request indicates everything (for that feature) is out of date.
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=13038110&view=results