Skip to content

Fix failure to initialize RemoteWorkspace in the out-of-process host#25691

Merged
sharwell merged 1 commit intodotnet:dev15.7.xfrom
sharwell:init-oop
Mar 27, 2018
Merged

Fix failure to initialize RemoteWorkspace in the out-of-process host#25691
sharwell merged 1 commit intodotnet:dev15.7.xfrom
sharwell:init-oop

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Mar 23, 2018

Customer scenario

Visual Studio crashes as soon as a project is opened.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/NET%20Developer%20Experience%20Productivity/_workitems/edit/590114

Workarounds, if any

None.

Risk

Relatively low.

Performance impact

N/A

Is this a regression from a previous update?

Yes, caused by f12bda8

Root cause analysis

There were two contributing factors to this:

  1. After deciding on a pattern to follow in 37a1d2c, I failed to undo related changes in f12bda8 to account for the simpler approach.
  2. Despite efforts to validate every incremental change in Production code changes to aid with test stabilization #25558, I did not realize that integration tests were disabled at the time. Knowing that unit tests simulate the remote host in-process, I was counting on integration tests to catch any errors in the true remoting layer. Once I saw the builds go green, I didn't look into the details to verify that the integration tests were actually providing the validation I expected.

How was the bug found?

We turned integration tests back on.

Test documentation updated?

N/A

@sharwell sharwell requested a review from a team as a code owner March 23, 2018 22:42
@jasonmalinowski
Copy link
Copy Markdown
Member

@dotnet-bot retest windows_debug_vs-integration_prtest please.

@sharwell sharwell mentioned this pull request Mar 26, 2018
get
{
var exportProvider = (IMefHostExportProvider)RoslynServices.HostServices;
var primaryWorkspace = exportProvider.GetExports<PrimaryWorkspace>().Single().Value;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this and code in RoslynServices are dup except HostServices. it would be less confusing who sets PrimaryWorkspace if it is extracted out to like "RemoteWorkspace.GetOrCreate(HostServices)" and reused in both places.

and explain that any one who calls this method first will set PrimaryWorkspace for the HostServices.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The caller should not be attempting to use the workspace prior to an explicit initialization sequence somewhere. This PR is just a workaround until the underlying design flaw can be fixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is no elsewhere in service hub host.

@jasonmalinowski
Copy link
Copy Markdown
Member

@jinujoseph @sharwell Can we take this now and then do a cherry-pick into the Preview 3 branch? I understand we want to take this through escrow but since it's also blocking integration tests I'd like to clear that too.

@heejaechang
Copy link
Copy Markdown
Contributor

if It is blocking some code flow, de-duping the code is not important enough to block code flow.

@jinujoseph
Copy link
Copy Markdown
Contributor

Approved to merge in d15.7.Preview3 via link
@sharwell make sure to retarget the branch to dev15.7-preview3 

@sharwell
Copy link
Copy Markdown
Contributor Author

@jinujoseph This is only needed for Preview 3 if #25469 is approved for Preview 3. We should get a decision on that before merging this.

@sharwell sharwell merged commit ebb3e37 into dotnet:dev15.7.x Mar 27, 2018
@sharwell sharwell deleted the init-oop branch March 27, 2018 14:19
@sharwell sharwell mentioned this pull request Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants