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

Fix call chain reentrancy #5145

Merged
merged 10 commits into from
Dec 3, 2018
Merged

Conversation

andhesky
Copy link
Contributor

Current behavior is to only allow call chain re-entrancy on A->B->A or A->A. This adds a new option for allowing A->B->C->A. Also flow the reentrancy context via RequestContext so that it can be viewed and serialized by the application layer.

  • Refactor re-entrancy tests to use common code
  • Change bool to enum in config and introduce new option CallChainReentrancyStrategy.EntireChain

Passes call chain tests. Sending an early version for feedback on the approach.

Remaining questions:

  • Do we want to reconcile the code for Single Call and full call chain?
  • Do we want to eliminate the option for single call (probably needs perf testing to determine impact)
  • Still need to figure out logging in tests

@sergeybykov
Copy link
Contributor

I perf tested it, and do not see any measurable negative impact.

@sergeybykov
Copy link
Contributor

@dotnet-bot test functional

@andhesky
Copy link
Contributor Author

andhesky commented Nov 2, 2018

Thanks for the info. I will move forward with this design. Ben mentioned removing the enum and just keeping it as the default behavior. This will change the behavior, but since there is no perf impact and we consider the current behavior a bug I will take his suggestion.

/// </summary>
/// <param name="message">Message to analyze</param>
private void CheckDeadlock(Message message)
/// <returns></returns>
private bool IsMessageACallChainLoop(Message message)
Copy link
Member

Choose a reason for hiding this comment

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

While the on-wire compatibility shouldn't be an issue between silos from 2.1.x and silos that will implement this logic, there might be issues during the deployment since we are changing the logic here.

But I don't think we have any way to address that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could have release notes that say enable deadlock detection if you want it to work during upgrade.

@benjaminpetit
Copy link
Member

Forgot to publish my reviews... dammit GitHub UI

@ReubenBond
Copy link
Member

@dotnet-bot test functional

@andhesky
Copy link
Contributor Author

@benjaminpetit, I think I addressed all your comments. Let me know if there are any other changes you would like to see.

Copy link
Member

@benjaminpetit benjaminpetit left a comment

Choose a reason for hiding this comment

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

LGTM. I'll kick off a backward compat testing

@sergeybykov sergeybykov merged commit 8d8cd70 into dotnet:master Dec 3, 2018
@andhesky andhesky deleted the andhesky/reentrancy branch December 3, 2018 20:28
sergeybykov added a commit to sergeybykov/orleans that referenced this pull request Dec 12, 2018
sergeybykov added a commit to sergeybykov/orleans that referenced this pull request Dec 12, 2018
sergeybykov added a commit to sergeybykov/orleans that referenced this pull request Dec 12, 2018
ReubenBond pushed a commit that referenced this pull request Dec 12, 2018
Add GSI cache maintenance and tests (#5184)
Revert: Fix call chain reentrancy (#5145, #5225) (#5249)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants