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

Ordering of unstashAll and side effect in typed persistence #26489

Closed
johanandren opened this Issue Mar 8, 2019 · 1 comment

Comments

Projects
2 participants
@johanandren
Copy link
Member

commented Mar 8, 2019

https://github.com/akka/akka/pull/26347/files/39b7db36064facad21dcce0647e4321c46c2d9f9#diff-032187146a93167944a6a9f14b37eaffR517

....thenUnstashAll().thenRun(_ => something)

Executes the thenRun part first rather than after unstashing all the commands in the stash.

@johanandren johanandren changed the title Ordering of unstashAl and side effect in akka-persistence-typed Ordering of unstashAll and side effect in typed persistence Mar 8, 2019

@patriknw patriknw added this to Ready for production backlog in Akka Typed Mar 10, 2019

@patriknw patriknw self-assigned this Mar 21, 2019

@patriknw patriknw moved this from Ready for production backlog to In Progress in Akka Typed Mar 21, 2019

@patriknw

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Hm, I think it would be very complicated to implement that the thenRun that is defined after thenUnstashAll would be kept around and then finally executed when all unstashing is done. Probably complicated for user to reason about as well, suddenly a callback is executed from an old command although several other commands (that were stashed/unstashed) have been processed inbetween.

Maybe we need something in the types to declare such effect as terminal, i.e. it would not have the thenRun method.

Need to discuss/ponder this, and it should not be started before we have merged #26520

@patriknw patriknw removed their assignment Mar 22, 2019

@patriknw patriknw moved this from In Progress to Ready for production backlog in Akka Typed Mar 22, 2019

patriknw added a commit that referenced this issue Mar 25, 2019

unstashAll as terminal Effect, #26489
* Effect.persist(event).thenUnstashAll().thenRun(..) can be misinterpreted
  as the callback of thenRun is invoked when all unstashing has been completed,
  while it is actually running the callback first and the the unstashing process
  follows.
* Unstashing is a process where stashed commands are processed one-by-one
  and waiting for persist effects to complete before processing next.
* Even if we would come up with a way to keep pending callbacks around during
  the unstashing it would probably be complicated for user to reason about it.
  Suddenly a callback is executed from an old command although several other
  commands (that were stashed/unstashed) have been processed inbetween.
* This change makes the unstashAll Effect terminal, meaning that additional
  effects like thenRun can't be added after unstashAll.
* ReplyEffect is also terminal, which makes sense since it's supposed to be
  returned effect. It must still be possible to combine with thenUnstashAll,
  thenReply.thenUnstashAll.

@patriknw patriknw self-assigned this Mar 25, 2019

@patriknw patriknw moved this from Ready for production backlog to In Progress in Akka Typed Mar 26, 2019

@patriknw patriknw moved this from In Progress to Reviewing in Akka Typed Mar 31, 2019

patriknw added a commit that referenced this issue Apr 2, 2019

unstashAll as terminal Effect, #26489
* Effect.persist(event).thenUnstashAll().thenRun(..) can be misinterpreted
  as the callback of thenRun is invoked when all unstashing has been completed,
  while it is actually running the callback first and the the unstashing process
  follows.
* Unstashing is a process where stashed commands are processed one-by-one
  and waiting for persist effects to complete before processing next.
* Even if we would come up with a way to keep pending callbacks around during
  the unstashing it would probably be complicated for user to reason about it.
  Suddenly a callback is executed from an old command although several other
  commands (that were stashed/unstashed) have been processed inbetween.
* This change makes the unstashAll Effect terminal, meaning that additional
  effects like thenRun can't be added after unstashAll.
* ReplyEffect is also terminal, which makes sense since it's supposed to be
  returned effect. It must still be possible to combine with thenUnstashAll,
  thenReply.thenUnstashAll.

patriknw added a commit that referenced this issue Apr 2, 2019

@patriknw patriknw added this to the 2.5.22 milestone Apr 2, 2019

@patriknw patriknw closed this Apr 2, 2019

Akka Typed automation moved this from Reviewing to Done Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.