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

Directly propagate OnActivateAsync failures to callers #3315

Merged
merged 3 commits into from Aug 21, 2017

Conversation

ReubenBond
Copy link
Member

Fixes #3213

  • Failures in OnActivateAsync will be propagated directly back to the caller.
  • Calls will be retried up to IMessagingConfiguration.MaxResendCount times (default is 0) because the rejection is marked as Transient.
  • Other message rejections which include an exception will also be propagated directly back to the caller, but this doesn't seem undesirable. The difference is that previously the exception would be discarded in favor of a generic exception (OrleansMessageRejectionException) which typically contained the original exception's Message in its Message property.

@ReubenBond
Copy link
Member Author

The test failure can be ignored.

RerouteAllQueuedMessages(activation, null, "Failed InvokeActivate", exception);
// Reject all of the messages queued for this activation.
var activationFailedMsg = nameof(Grain.OnActivateAsync) + " failed";
RejectAllQueuedMessages(activation, activationFailedMsg, exception);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a race here that will allow new requests to get enqueue right after we reject all messages that are already in the queue? Are we preventing enqueuing of new requests before we start the rejection loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

The activation is already marked as invalid and this is running inside of a lock on the activation. Is there room for a race?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's already marked as invalid, then I think we are fine.

@sergeybykov sergeybykov merged commit e7d5ff7 into dotnet:master Aug 21, 2017
@ReubenBond ReubenBond deleted the propagate-activate-failures branch August 21, 2017 22:07
@rvarna
Copy link

rvarna commented Oct 17, 2017

Is this change available in 1.5.1 or 1.5.2 (and 1.5.1)?

@sergeybykov
Copy link
Contributor

No, it's only in master. Will be in 2.0.0-beta.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
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.

None yet

4 participants