-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Avoid UI state manipulation on background thread #68507
Conversation
@@ -816,7 +816,7 @@ private async Task CommitCoreAsync(IUIThreadOperationContext operationContext, b | |||
var eventName = previewChanges ? FunctionId.Rename_CommitCoreWithPreview : FunctionId.Rename_CommitCore; | |||
using (Logger.LogBlock(eventName, KeyValueLogMessage.Create(LogType.UserAction), cancellationToken)) | |||
{ | |||
var info = await _conflictResolutionTask.JoinAsync(cancellationToken).ConfigureAwait(false); |
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 ConfigureAwait(false)
was allowing the _dismissed
field to be set from a background thread. Since RenameService.ActiveSession
is cleared after _dismissed
is set, it was allowing a UI thread check here to proceed between the point where _dismissed
was set and the point where ActiveSession
was cleared:
Line 19 in 4f0feeb
if (_renameService.ActiveSession != null) |
// Note: this entire sequence of steps is not cancellable. We must perform it all to get back to a correct | ||
// state for all the editors the user is interacting with. | ||
var cancellationToken = CancellationToken.None; | ||
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.
📝 This will now always NOP in practice, but the relocation of this switch to main thread adheres to JTF best practice. The code intentionally does not include an assertion.
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.
wow. good find :)
Fixes AB#1833091