Skip to content
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

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

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

CyrusNajmabadi
Copy link
Member

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.

Copy link
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
Member 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