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

CommandAsync for persistent actors #2689

Merged
merged 4 commits into from
Aug 9, 2017

Conversation

zbynek001
Copy link
Contributor

@zbynek001 zbynek001 commented May 21, 2017

Here is another try on CommandAsync topic.
I's introducing similar support for async/await as in ReceiveAsync.

Remarks:

  • As with ReceiveAsync, all other commands to actor are blocked until the command handler task finishes processing the current command, including the Persist and PersistAll callbacks.
  • It does NOT introduce async support for Persist callbacks, they work the same way as before


namespace Akka.Persistence.Tests
{
//internal class BehaviorOneActor : PersistentActorSpec.ExamplePersistentActor
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this part commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to move it to another namespace. It inherits from PersistentActorSpec.ExamplePersistentActor but i need to use a different actor with the RunTask inside

@@ -243,6 +243,7 @@ namespace Akka.Persistence
public void PersistAsync<TEvent>(TEvent @event, System.Action<TEvent> handler) { }
protected abstract bool ReceiveCommand(object message);
protected abstract bool ReceiveRecover(object message);
protected void RunTask(System.Func<System.Threading.Tasks.Task> action) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is not unique for the persistence. It should be either hidden or included in ActorBase class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is unique to Eventsourced, because of the Persist callbacks.
Non-persistence actors already have other equivalents, e.g. UntypedActor have RunTask, ReceiveActor have ReceiveAsync

if (!t.IsCompleted)
{
var tcs = new TaskCompletionSource<object>();
t.ContinueWith(r =>
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably won't need TaskCompletionSource here. ContinueWith will return continuation task, while TaskContinuationOption.NotOnFaulted | TaskContinuationOption.NotOnCancelled should allow you to get rid of forwarding cancelations and faults.

There's also one thing to remember - things like actor's Context or Self are fragile and won't work in task continuations. This also concerns all methods invoked here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I didn't need it, but now i need to react also on failure.
Regarding Context, it works as usual. Context, Sender,.. are accessible inside the command handler. Same for Persist callback except of the Sender, but that's by design

Persist(new Evt(cmd.Data + "-0"), UpdateStateHandler);
Context.Become(NewBehavior);
});
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's fully valid way of calling RunTask. As it won't be invoked immediately, the handler will return true, while the func inside may throw an exception. This may lead to a false positives, but I'd need to check how this would look like in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be the same behavior as with ReceiveAsync

@Aaronontheweb
Copy link
Member

@zbynek001

ApprovalTests.Core.Exceptions.ApprovalMismatchException : Failed Approval: Received file D:\work\d26c9d7f36545acd\src\core\Akka.API.Tests\CoreAPISpec.ApprovePersistence.received.txt does not match approved file D:\work\d26c9d7f36545acd\src\core\Akka.API.Tests\CoreAPISpec.ApprovePersistence.approved.txt.
   at ApprovalTests.Approvers.FileApprover.Fail()
   at ApprovalTests.Core.Approver.Verify(IApprovalApprover approver, IApprovalFailureReporter reporter)
   at ApprovalTests.Approvals.Verify(IApprovalWriter writer, IApprovalNamer namer, IApprovalFailureReporter reporter)
   at ApprovalTests.Approvals.Verify(IApprovalWriter writer)
   at ApprovalTests.Approvals.Verify(String text)
   at Akka.API.Tests.CoreAPISpec.ApprovePersistence() in D:\work\d26c9d7f36545acd\src\core\Akka.API.Tests\CoreAPISpec.cs:line 53

Need to run the API approval process locally... http://getakka.net/docs/akka-developers/public-api-changes

@zbynek001
Copy link
Contributor Author

@Aaronontheweb will do that. Btw there is an issue with the approval ApiGenerator, the order of items is based on local culture, InvariantCulture might be better. Will create another pull request for that

@zbynek001 zbynek001 changed the title [WIP] CommandAsync for persistent actors CommandAsync for persistent actors May 22, 2017
@Aaronontheweb
Copy link
Member

@zbynek001 rebasing on those changes since we merge in your API approval fix.

@Danthar
Copy link
Member

Danthar commented Aug 4, 2017

Has some minor conflicts that need to be resolved. @Aaronontheweb we are not making this part of release 1.3 ?

@Aaronontheweb
Copy link
Member

@Danthar yeah, this should definitely go into 1.3

@Aaronontheweb Aaronontheweb added this to the 1.3.0 milestone Aug 9, 2017
@Aaronontheweb Aaronontheweb merged commit abf49d3 into akkadotnet:v1.3 Aug 9, 2017
@zbynek001 zbynek001 deleted the CommandAsync branch June 24, 2018 12:13
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.

6 participants