Skip to content

Do not try to refcount solution syncing when communicating with OOP#60753

Merged
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:revertSolutionRefCount
Apr 14, 2022
Merged

Do not try to refcount solution syncing when communicating with OOP#60753
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:revertSolutionRefCount

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

This logic is correct on the OOP side. However, on the client side it is broken. Specifically, consider this set of interleaved concurrent operations on the client side and OOP:

Client Side:

  1. Operation 1 starts up creating a scope<->solution-state mapping. (scopeId 0)
  2. Operation 2 starts up (on same solution snapshot) creating a scope<->solution-state mapping. (scopeId 1)
  3. Operation 1 cancels, and disposes it's mapping.

OOP side:

  1. Operation 1 is received and creates a syncing operation to take hydrate the solution corresponding to its checksum.
  2. Operation 2 is received and sees an existing syncing operation to hydrate the solution. It attaches itself to that, increasing the refcount.
  3. On hte client side operation 1 is canceled. This cancels operation1 on the OOP side (but not the syncing operation, which is refcounted). Operation 2 continues to wait on this syncing operation. This syncing operation then crashes on the client side because it refers to scopeId-0, which has been removed from the mapping on the host.

The real fix here will need to be that on the client side we keep these checksum->scope-id mappings around with a refcount as well, so that we only remove from teh client mapping once all concurrent calls actually finish.

In the meantime though, stop trying to share the syncing computation and instead have all calls create their own concurrent sync that is safe from other operations being canceled.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 14, 2022 20:07
@ghost ghost added the Area-IDE label Apr 14, 2022
@CyrusNajmabadi CyrusNajmabadi requested a review from dibarbet April 14, 2022 20:07
Copy link
Copy Markdown
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

am OK with the directives as long as the plan is to bring it back with fixes soon

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

am OK with the directives as long as the plan is to bring it back with fixes soon

Yup. WOrking on that PR now :)

@CyrusNajmabadi CyrusNajmabadi merged commit 7711d35 into dotnet:main Apr 14, 2022
@ghost ghost added this to the Next milestone Apr 14, 2022
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the revertSolutionRefCount branch May 3, 2022 22:04
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.

2 participants