Skip to content

Fix race condition in SolutionExplorer_InProc.OpenFile#26265

Merged
sharwell merged 2 commits intodotnet:dev15.7.xfrom
sharwell:openfile-race
Apr 20, 2018
Merged

Fix race condition in SolutionExplorer_InProc.OpenFile#26265
sharwell merged 2 commits intodotnet:dev15.7.xfrom
sharwell:openfile-race

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Apr 19, 2018

This change updates the code to use ProjectItem.Open instead of relying on the asynchronous File Open command. Prior to the change, the method would occasionally return early, allowing the caller to modify the previous active document before the newly opened document was fully activated.

Fixes #19191
Fixes #26037
Closes #26205

@sharwell sharwell requested a review from a team as a code owner April 19, 2018 14:26
jfleisher
jfleisher previously approved these changes Apr 19, 2018
Copy link
Copy Markdown
Contributor

@jfleisher jfleisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated all integration tests locally. All is we.

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Apr 19, 2018

@jfleisher It broke a few tests for a reason I knew was a possibility¹, but wasn't sure which test would be impacted. I'll get that fixed.

¹ The old code could open any file from disk, but the new code requires the file to be part of the project can only open a top-level file within a project.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this get a comment here? As you know, my faith in people carefully inspecting history is...low. 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

("this is how it should have been written in the first place" is quite possibly a valid counter here...never like comments that explain why the previous, obviously bad code was bad)

@sharwell sharwell dismissed stale reviews from jfleisher and jasonmalinowski April 19, 2018 18:30

Completely rewrote the implementation

@sharwell sharwell changed the title Fix race condition in SolutionExplorer_InProc.OpenFile [WIP] Fix race condition in SolutionExplorer_InProc.OpenFile Apr 19, 2018
This change updates the file open implementation use VsShellUtilities.OpenDocument.
DTE plus waiting for the active window title alone proved insufficient, where some
tests were incorrectly operating on the previous active document.

Fixes dotnet#19191
Fixes dotnet#26037
@sharwell sharwell changed the title [WIP] Fix race condition in SolutionExplorer_InProc.OpenFile Fix race condition in SolutionExplorer_InProc.OpenFile Apr 20, 2018
@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Apr 20, 2018

@jasonmalinowski I didn't want to resort to ff77b97 but I was still seeing failures without it. 😞 I went with this approach.

Some of the inconsistencies - specifically focus issues - revealed by iterative testing (running hundreds of times in sequence) appear to match behavior anomalies I've observed during daily coding.

Copy link
Copy Markdown
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done.

@sharwell sharwell merged commit f076f79 into dotnet:dev15.7.x Apr 20, 2018
@sharwell sharwell deleted the openfile-race branch April 20, 2018 18:57
@sharwell
Copy link
Copy Markdown
Contributor Author

Merged test only change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants