akka / akka-meta

This repository is dedicated to high-level feature discussions and for persisting design decisions.
Apache License 2.0
201 stars 23 forks source link

Akka Typed: API discussion #46

Open jrudolph opened 7 years ago

jrudolph commented 7 years ago

Today we reviewed the current typed Actor API for Scala. We tried to improve a few things:

We attempted a prototype at how it could look like. The basic ideas are those:

We reimplemented the chatroom example using different kind of styles using the new API. We currently favor the chatRoomMutable and chatRoomFunctional kind of doing things. (We understand that the chatRoomFunctional example will use one extra lambda allocation to support withContext which might be fine. chatRoomFunctionalRecursive improves upon that but may be harded to read).

object PotentialAPIImprovements {
  trait Ctx[T] {
    def self: ActorRef[T]
    def deriveActorRef[U](f: U ⇒ T): ActorRef[U]
    def watch(actorRef: ActorRef[_]): Unit
  }

  trait Behavior[T]

  def Same[T]: Behavior[T] = ???

  def withContext[T](f: Ctx[T] ⇒ Behavior[T]): Behavior[T] = ???

  def handleMessages[T](f: T ⇒ Behavior[T]): Behavior[T] = handleMessagesAndSignals(f)(PartialFunction.empty)
  def handleMessagesAndSignals[T](f: T ⇒ Behavior[T])(signals: PartialFunction[Signal, Behavior[T]]): Behavior[T] = ???

  def recursively[T, U](initialState: U)(f: (U ⇒ Behavior[T]) ⇒ U ⇒ Behavior[T]): Behavior[T] = ???

  // current `Stateful` for comparison
  def functionalTwoArgs[T](f: (Ctx[T], T) ⇒ Behavior[T]): Behavior[T] = ???
}

object TestExample {
  sealed trait Command
  final case class GetSession(screenName: String, replyTo: ActorRef[SessionEvent])
    extends Command
  //#chatroom-protocol
  //#chatroom-behavior
  private final case class PostSessionMessage(screenName: String, message: String)
    extends Command
  //#chatroom-behavior
  //#chatroom-protocol

  sealed trait SessionEvent
  final case class SessionGranted(handle: ActorRef[PostMessage]) extends SessionEvent
  final case class SessionDenied(reason: String) extends SessionEvent
  final case class MessagePosted(screenName: String, message: String) extends SessionEvent

  final case class PostMessage(message: String)

  import PotentialAPIImprovements._

  def chatRoomMutable: Behavior[Command] =
    withContext[Command] { ctx ⇒
      var sessions: List[ActorRef[SessionEvent]] = Nil

      handleMessagesAndSignals[Command]({
        case GetSession(screenName, client) ⇒
          val wrapper = ctx.deriveActorRef {
            p: PostMessage ⇒ PostSessionMessage(screenName, p.message)
          }
          client ! SessionGranted(wrapper)
          sessions ::= client
          ctx.watch(client)
          Same
        case PostSessionMessage(screenName, message) ⇒
          val mp = MessagePosted(screenName, message)
          sessions foreach (_ ! mp)
          Same
      })({
        case Terminated(ref) ⇒
          sessions = sessions.filterNot(_ == ref)
          Same
      })
    }

  def chatRoomFunctional: Behavior[Command] = {
    def state(sessions: List[ActorRef[SessionEvent]]): Behavior[Command] =
      handleMessagesAndSignals[Command]({
        case GetSession(screenName, client) ⇒
          withContext { ctx ⇒
            val wrapper = ctx.deriveActorRef {
              p: PostMessage ⇒ PostSessionMessage(screenName, p.message)
            }
            client ! SessionGranted(wrapper)
            ctx.watch(client)
            state(client :: sessions)
          }
        case PostSessionMessage(screenName, message) ⇒
          val mp = MessagePosted(screenName, message)
          sessions foreach (_ ! mp)
          Same
      })({
        case Terminated(ref) ⇒ state(sessions.filterNot(_ == ref))
      })

    state(Nil)
  }

  def chatRoomFunctionalRecursive: Behavior[Command] =
    withContext { ctx ⇒
      recursively(List.empty[ActorRef[SessionEvent]]) { (state: List[ActorRef[SessionEvent]] ⇒ Behavior[Command]) ⇒ sessions: List[ActorRef[SessionEvent]] ⇒
        handleMessagesAndSignals[Command]({
          case GetSession(screenName, client) ⇒
            val wrapper = ctx.deriveActorRef {
              p: PostMessage ⇒ PostSessionMessage(screenName, p.message)
            }
            client ! SessionGranted(wrapper)
            ctx.watch(client)
            state(client :: sessions)
          case PostSessionMessage(screenName, message) ⇒
            val mp = MessagePosted(screenName, message)
            sessions foreach (_ ! mp)
            Same
        })({
          case Terminated(ref) ⇒ state(sessions.filterNot(_ == ref))
        })
      }
    }

  def chatRoomStatefulStyle: Behavior[Command] = {
    def state(sessions: List[ActorRef[SessionEvent]] = Nil): Behavior[Command] =
      functionalTwoArgs[Command] { (ctx, msg) ⇒
        msg match {
          case GetSession(screenName, client) ⇒
            val wrapper = ctx.deriveActorRef {
              p: PostMessage ⇒ PostSessionMessage(screenName, p.message)
            }
            client ! SessionGranted(wrapper)
            state(client :: sessions)
          case PostSessionMessage(screenName, message) ⇒
            val mp = MessagePosted(screenName, message)
            sessions foreach (_ ! mp)
            Same
        }
      }

    state()
  }
}

WDYT, @rkuhn?

patriknw commented 7 years ago

We also noted a few other things (please fill in with your notes).

ktoso commented 7 years ago

Some of my comments and notes:

+100, it really feels like a Props - stuff that accompanies "the actor".

We also contemplated putting restart things into the Props, so yes it would be synchronous but in failure restarts would be in control of the one that starts the Actors. This is specifically very nice for things that start Actors for users like cluster stuffs and persistence where we (or just users) may want to add exponential backoff supervision (I think that's one of the more important changes we can do).

Slightly in favour of this. Since it's not as easy to get things wrong (closing over), and people use ask anyway this can make ask simpler to people. I also think

Other questions:

Will comment some more pretty soon, heading out for now. Very exciting to get hands and personal on with this finally!

jducoeur commented 7 years ago

A few thoughts (from the viewpoint of not having actually had a chance to play with it, so take or leave these as seems appropriate):

we might get rid of the preStart signal

Is there a better opportunity to say "do this before I accept messages" in the new world? That's the real need, as I see it.

ask should perhaps be directly on ActorRef instead of via that extra patterns package

I'll just note that having it separate is precisely why Requester works as well as it does, as a nearly drop-in replacement for ask. Of course, Requester is kind of a hack, so I'd be happy for it to be proven unnecessary in the new world, but we should try to be confident about that.

(Actually, in my perfect world, Requester would be the standard behavior -- the Actor would provide a Requester-style ExecutionContext, and that's how Futures would work by default. But I'll admit that I haven't looked hard enough at the pros and cons of that.)

Since it's not as easy to get things wrong (closing over)

Is it that much less common to close over mutable state in the new architecture? I'm willing to believe it, and losing sender() obviously helps, but it's not intuitively obvious that it helps with the rest of the problem of mutable state and races. (In practice, race conditions are why I am so very fanatical about avoiding Futures in my code, and were the main impetus for Requester.) And I do want to have some reasonably safe and easy way to compose complex multi-Actor operations while preserving the "single-threaded" invariant -- I use this all the time.

Actually, here's a question that occurs to me: are there any examples of the interaction between ask and changing the Behavior state? Based on my existing code, I suspect that I'm frequently going to want to call several roundtrips and then change my Actor's state based on the final result. I can kind of see how I might build that in the new world, but it feels a bit clunky; I'm curious about whether anybody has given any thought yet into how best to represent that monadically. (I'm kinda hoping that I can avoid having to write Requester2.)

In general, though, this is looking really exciting. Going to take a while to get the new style into my head, but I love the example code, and am already chewing over the problems in my existing system that would be solved by the new model.

(A nice example from my own code, in case anybody's tracking use cases: while the ideal is Run-Anywhere, in practice that's not always practical. Querki is intentionally built so that the local troupe of Actors around a given Space can share the SpaceState -- a huge immutable structure that I do not ever want to serialize over the wire. One problem I've had in practice is distinguishing whether a command is coming from that local troupe (and thus can receive the updated state in the response) or from outside (so it can't). That's currently painfully ad-hoc, but in the new world I should be able to distinguish "local" and "potentially remote" channels in a lovely and principled way.)

rkuhn commented 7 years ago

I would have loved to join you in Vilnius! But I guess one cannot always win …

First the good ones:

withContext

I sympathise with the concept of unifying context extraction with Deferred, but I disagree with the premise. The current DSL is designed to minimize coupling between Behavior and ActorContext, to factor these concerns of description and execution cleanly into different parts.

Syntax wise withContext { ctx => ... } is problematic because people will assume that it works like for managed resources or database connections, which is untrue unless we couple this construct to some ThreadLocal or other monstrosity. The beauty of Akka Typed is that it is free from magic, and I would really like to retain that.

adding ask methods to ActorRef

I’d much rather remove ask completely, at least the Future-based version. Within an actor I don’t see a good reason to use ask, ever:

bubbling up failures

Unhandled child termination will bubble up if watched—if not watched then obviously the parent does not care. This simplifies things to a single concept that also makes sense semantically.

taking PartialFunction arguments

In general I dislike requiring PartialFunction in signatures because it means that client code is forced to use case statement syntax, which is not otherwise a given in all cases. The argument of extensibility is a good one, though: the Signal trait should most definitely not be sealed!

Opaque Behavior

The beauty of the current model is that it expresses exactly the nature of an Actor: a behavior is a function from input to new behavior. Having two methods is owed to the lack of union types in Scala 2. If Behavior became opaque, it would be impossible to write new ones without the overhead imposed by the current ones (i.e. requiring extra closures to be allocated and executed). One example is the process DSL. It is not clear to me what the advantage would be of switching from a function to something that needs an interpreter. Being able to call message and management (names can be improved!) in tests is very useful, the openness and the absence of tools and magic are major benefits of the current DSL over Akka TestKit.

DeploymentConfig vs. Props

The reason why I chose that name is that Props have a different connotation, they used to wrap a behavior (coming from untyped). Would it not be confusing to users? If my concern here is unwarranted, then of course the rename is good, I’m always for shorter names.

Restarter

This part is not clear to me: the API already is in the DSL packages, as is appropriate, and since both languages share a common implementation that is put elsewhere. The implementation could move to the internal package.

But I don’t understand the comment of wrapping at the wrong level: as far as I can see Restarter is a decorator that can be added many times, at all levels. What is wrong with that? In particular, every instance of it catches a specific type of exception, so some layering is needed. And with the current scheme an actor that accepts foreign Behaviors can easily wrap them in a Restarter, no need to make that part of the Props.

One thought here is that it might make sense to have a non-sticky variant of Restarter, so that the actor itself could declare part of its operations as restartable.

recursively

It took me a few minutes to figure out what the signature means, which I think is a rather bad sign in terms of usability for newbies. The benefit would obviously be that the state is reified instead of implicit, allowing better debug logging in tests. A signature that would be simpler to understand is

def stateful[T, U](initial: U)(step: (U => Behavior[T], U, T) => Behavior[T]): Behavior[T]
// or even
def stateful[T, U](initial: U)(step: (U, T) => Either[U, Behavior[T]]): Behavior[T]

Still, I’m on the fence whether this should be in the standard package. People can easily write this themselves (with extensible Behavior!) or copy from some gist.

General

I would have loved to hear why Actor.stateful is less clear than handleMessagesAndSignals: the former expresses the intended semantics while the latter focuses on the implementation details. And I still think that Actor.stateless is useful in many cases (i.e. state is only created during initialization and not changed afterwards).

jducoeur commented 7 years ago

(More from the peanut gallery. Y'all feel free to tell me to quiet down if it's too much.)

I’d much rather remove ask completely, at least the Future-based version. ... or use the process DSL

... fascinating. I'd missed your post on the process DSL (I just looked it up), but if I'm understanding it correctly, I agree with your point. Futures inside of Actors have always been, at best, a barely-necessary evil, and eliminating (or taming) them inside a better abstraction would be a huge win.

Am I correct that, in the process DSL, the combination of opCall and opAsk has the effect "Send this message, and pause processing any new messages in me until I get the response"? If so, that's exactly what I usually want, and have never had a good way to achieve before. (Requester was a stab in this direction, but "pausing" the mailbox still requires manual hacking around with stash().)

IMO the DSL's syntax is a tad clunky for everyday use, and I'd like to better understand how it would work in the context of an ongoing stateful Actor. In particular, the model I want all the time is:

If that's achievable, I suspect I would be happy to see ? redefined as a facade around this combination of opCall(opAsk()) -- I believe it would not only be intuitive, but it helps with the race-condition traps that traditionally make Akka programming a tad fraught. (Heck, it might even be sufficiently reason-able for me to be able to argue for it with the FP crowd on Gitter, some of whom are often a tad unkind in their opinions of Akka.)

Very neat stuff, and I love the composability...

ktoso commented 7 years ago

Thanks a ton for the elaborate replies, that's the kind of discussions I love :-) Replying to a sub-set of things I can right now during the night:

DeploymentConfig vs. Props

I think the worry about confusion is overprotective to users. I think that users mostly think of Props as "the additional actor config stuff". I have not met a person outside the team that would notice that the primary goal would be the factory trick - since we've hidden it in the => Actor signature kind of, so people did not notice.

I think the rename is safe and useful enough to warrant doing it :-)

Actor.stateful

IMHO the handleMessagesAndSignals is definitely not something that is meant to be final API - correct me if I'm wrong guys. I do, however, have a problem around the "what it is actually doing" mismatch with stateful/stateless methods since they're not entirely about that, as the currently-names-as Deferred is the actual thing that enables "keep a var around", which sounds like the "mutable" to me.

I was also thinking about calling anything that gives space to "keep a mutable var" simply "Mutable { var x = ???; { case ... => ... } }` if you know what I mean?

I really would like to avoid the (ctx, msg) => though as it makes things a hassle. I would much much rather opt for ctx => msg => since then the 2nd function can be case Bla already. The counter to that was though that people get confused by currying hm...


My personal "dream" API is this by the way (core of it at least, which basically combines the Stateful and Deferred, and would add some overloads or conversions to allow "if I want context I just add ctx =>.

This is the draft I had, for inspiration (i had it working, but impl was very very sub optimal, since it would allocate function for every message receive):

// ---- FOR INSPIRATION ---- 

  // while I love the "Actor" name here, perhaps NOT what we want as it's confusing?
  final case class Actor[T](receive: ActorContext[T] ⇒ T ⇒ Behavior[T])
    extends Behavior[T] {

    override def management(ctx: ActorContext[T], msg: Signal): Behavior[T] =
      Same

    override def message(ctx: ActorContext[T], msg: T): Behavior[T] =
      receive(ctx)(msg)
  }

//  implicit def hakk[T](f: T ⇒ Behavior[T]): ActorContext[T] ⇒ T ⇒ Behavior[T] =
//    ctx ⇒ t ⇒ f(t)

  implicit def hakk[T](f: PartialFunction[T, Behavior[T]]): ActorContext[T] ⇒ T ⇒ Behavior[T] =
    ctx ⇒ t ⇒ {
      if (f.isDefinedAt(t)) f(t)
      else Unhandled
    }

  sealed trait Model
  case class Greet(who: ActorRef[Greeted]) extends Model
  case class Greeted() extends Model

  def lol(take: String) = Actor[Model] { ctx ⇒ 

    {
      case newName: Greet ⇒
        lol(take) // TODO WHAT DOES THIS MEAN
      case changeName: Greeted ⇒
        Same
    }
  }

  Actor[Model] {
    case newName: Greet ⇒
      Same
    case changeName: Greeted ⇒
      Same
  }

  Actor[Model] {
    model ⇒ Same
  }
  // the above works because in Behavior object we could:
  //  implicit def same_2_same[T](b: Behavior[Nothing]): T ⇒ Behavior[T] =
  //    null // FIXME, can we figure it out?
rkuhn commented 7 years ago

@ktoso We are talking about a handful of characters of boilerplate code. For purpose of ignoring naming for now, assume a factory method makeActor.

makeActor[Command] {
  case (ctx, Ping) => // send pong
}

// vs

makeActor[Command] { ctx =>

  {
    case Ping => // send Pong
  }
}

To me the cost of making this magic work consistently is just too high (not to mention the reason for that funny empty line and all the allocations; with the current DSL users can decide to spend a few extra characters on a match in order to get rid of them). We are talking about the foundational abstraction of Akka, and I would not like to see that suffer from the same cognitive issues as the HTTP DSL (which is utterly impenetrable for newbies, who are thus limited to copy-pasting tutorial snippets without understanding how things work).

Again, we can experiment with alternative ways of doing things, there can easily be a plurality of solutions in this space. My preference is to keep the first solution that we offer clean, minimal, and dead obvious.

And of course, Behavior must remain extensible.

patriknw commented 7 years ago

couple this construct to some ThreadLocal or other monstrosity

Our idea of how to implement was to let the machinery detect the returned WithContext behavior and invoke it with the ctx. There is already similar things for Same and Unhandled.


RK: Yes, I got that part, but the syntax would invite mistakes like the following

case Ping =>
  withContext { ctx =>
    // do stuff, create actors, whatever
  }
  Same

It is enough to have Actor.deferred to solve this, this makes it very clear what will happen.

patriknw commented 7 years ago

case (ctx, Ping) =>

suffers from tuple allocation without most users knowing (but I'm not sure it's a big deal)


RK: yes, as I said, the user can choose aesthetics over performance, and we know from Free monad usage that many people just don’t care about execution characteristics

patriknw commented 7 years ago

getting rid of preStart

That would require usage of WithContext (or Deferred) when anything with context is to be done at construction time, e.g. starting children


RK: Yes, true, thanks for making this explicit.

rkuhn commented 7 years ago
scala> object A { def f(x: Int => Int)=42; def f(x: (Int, Int) => Int)=43 }
defined object A

scala> A.f(x => x)
res3: Int = 42

scala> A.f((x, y) => x)
res4: Int = 43

So we can actually overload Actor.stateful and Actor.stateless with two signatures, one taking an unary function (without context) and one taking a binary function (context and message). Does this not solve it in a much simpler fashion?

EDIT: well, that does not work with partial function literals, so separate names would be needed to support those

ktoso commented 7 years ago

Yes I think this is indeed the direction of what we're (I'm) after. Sadly the => => style could not be overloads... so perhaps this style is a good tradeoff, going with the withContext route instead of the Deferred seemed to us to enable dropping preStart since you can do everything in the "constructor". Also you can only get the context when you use it. The overloads solution may be good hm, but let's play with the idea still.

To me the cost of making this magic work consistently is just too high (not to mention the reason for that funny empty line and all the allocations;

To clarify: The funny empty line requirement does not seem to exist AFAICS? Just checked and it compiles on 2.12.

with the current DSL users can decide to spend a few extra characters on a match in order to get rid of them).

Though one could also look at it that way that most of the time context will likely not be used yet we keep matching on it.

Though the case (ctx, msg) style does imply allocations (in 2.12 still). We're worried that every actor will be doing this all the time, and going to the "just match" feels really really ugly, as in "why have a DSL at all if I have to manually match anyway?"

Perhaps it is fine though...

You may be right that the ctx => msg => indeed may be confusing to newbies, so that one is out the window I think. Though back to thinking about withContext {} for getting it or the overload as you proposed now.

We are talking about the foundational abstraction of Akka, and I would not like to see that suffer from the same cognitive issues as the HTTP DSL (which is utterly impenetrable for newbies, who are thus limited to copy-pasting tutorial snippets without understanding how things work).

Absolutely! Which is why I'd want to bounce some more idea before settling on the API - even if you have bounced it back and forth some time, we have not had much time hands on with it yet and may come with new observations/options. So the => => I think we drop, but back to the context hm

drewhk commented 7 years ago

Sadly the => => style could not be overloads... so perhaps this style is a good tradeoff, going with the withContext route instead of the Deferred seemed to us to enable dropping preStart since you can do everything in the "constructor".

Just my uneducated 2cs: the new behavior system allows for various composition patterns, hence I think the goal should be to have the most flexible, and still reasonable base-line implementation, then one can add different styles of APIs on top. I would personally prefer if one can do things allocation free if necessary.

patriknw commented 7 years ago

Yes, I got that part, but the syntax would invite mistakes like the following

case Ping =>
withContext { ctx =>
// do stuff, create actors, whatever
}
Same

That's a good argument. I have been on the fence but this makes me tip over to that we should not provide withContext and stick with the two parameters.

patriknw commented 7 years ago

Conclusions:

If you agree I will create separate tickets for the tasks.

rkuhn commented 7 years ago

Sounds very good, with one exception: I think PreStart can and should be dropped because using Deferred is the right thing to do. When creating actors during construction, the resulting behavior will need to differ from the initial one in that it now has references to those actors; this makes PreStart a signal that is handled exclusively to then create the real behavior—Deferred currently is a shorthand for this. The issue I have (which is still reflected in some TODOs) is that it is easy to create something that is not composable with this approach because it forgets to inject a PreStart as the first message. Using Deferred for all these purposes would solve this elegantly—I don’t know why I didn’t see that previously ;-)

In that light, Deferred should not be renamed to Mutable because it will often be used in a completely immutable fashion, just deferring things until instantiated. If you find a better term that conveys this meaning, then I’m all ears, but so far Deferred is the one that best matches the semantics.

jrudolph commented 7 years ago

Hmm, I'm not yet sure.

I didn't really explained the rationale behind the combinators as shown above and just dumped them there (sorry for that). Not all of them are necessary (like recursively which isn't needed).

Here's basically the rationale for what we did:

We can approach the space either from the functional or from the side-effecting direction.

The pure, functional way would model all effects explicitly (including those provided by the context and message sending) as the return type of the handler. Effects could be composed to a composite return value. Whatever runs the handler will "interpret" the effects and execute them.

The side-effecting is very much the old API where all side-effects are achieved directly by mutating internal directly or indirectly.

I think we all agree that the purely functional way of doing things would be slow because too many wrapper objects are being created. I think we also agree that we should provide a user API both for the functional and the mutable way of specifying actors.

The current approach of akka-typed in master is already a mix of both:

The functional part (= changing the state) is already now achieved by interpreting the result of the handler. The result can either be

The summary of that is that we already now have an interpreter in place that is spread over multiple places.

Regarding the "combinators" to provide, the problem I see with Deferred is that it doesn't compose. That means that you cannot build a library of actor building blocks where some parts are written in functional style and others in the mutable style.

When we tried to achieve that we ended up with only the two main building blocks shown above, handleMessagesAndSignals and withContext (the names can be improved). handleMessagesAndSignals defines the mostly functional way of defining behavior (sending messages is the only side-effect), withContext can be used for anything that needs to built on top of side-effects. Behaviors built from those two building blocks can be mixed freely. This could be useful if we recommend the functional style and you later want to optimize performance by converting just single states to the mutable style. In contrast, Deferred can only be used to define the complete actor behavior in the mutable style, you cannot build the reusable blocks with it.

E.g. if you want to build a behavior, i.e. one state that is part of a larger actor state machine, that spawns a child actor and then waits for three messages and sends them to the child in a batch:

// new style
def getThreeAndSendToChild: Behavior[String] =
  withContext { ctx =>
     val child = ctx.actorOf(...)

     var batch: List[String] = Nil

     handleMessages {
       case msg =>
          batch ::= msg
          if (batch.size == 3) {
            child ! batch
            batch = Nil
          }
          Same
     }
  }

// example usage:

def fooActor: Behavior[String] = 
  handleMessages {
    case "init" => getThreeAndSendToChild
    case _ => Unhandled
  }

How would you build that behavior with the current API?

Maybe like the following. We cannot use Deferred for that but we need to pass ctx explicitly:

// current style
def getThreeAndSendToChild(ctx: ActorContext[String]): Behavior[String] = {
     val child = ctx.actorOf(...)

     var batch: List[String] = Nil

     Stateful { (ctx, msg) =>
         batch ::= msg
         if (batch.size == 3) {
           child ! batch
           batch = Nil
         }
         Same
     }
  }

// example usage:

def fooActor: Behavior[String] = 
  Stateful {
    case "init" => 
       getThreeAndSendToChild(ctx)
       Same
    case _ => Unhandled
  }}

but whoops, now we have the same problem even with the existing API, that was mentioned before:

Yes, I got that part, but the syntax would invite mistakes like the following

 case Ping =>
  withContext { ctx =>
    // do stuff, create actors, whatever
  }
  Same

So, I see the danger, though I'm not sure if that's really a practical problem. In a real example you would have to return a behavior inside the block as well. Wouldn't people notice that they return behaviors at two different places? Could the name be improved to prevent that or could we add runtime checks that prevent such a usage?

jrudolph commented 7 years ago

Sounds very good, with one exception: I think PreStart can and should be dropped because using Deferred is the right thing to do. When creating actors during construction, the resulting behavior will need to differ from the initial one in that it now has references to those actors; this makes PreStart a signal that is handled exclusively to then create the real behavior—Deferred currently is a shorthand for this. The issue I have (which is still reflected in some TODOs) is that it is easy to create something that is not composable with this approach because it forgets to inject a PreStart as the first message. Using Deferred for all these purposes would solve this elegantly—I don’t know why I didn’t see that previously ;-)

I don't understand that completely. How would you propose that the Deferred implementation and semantics should look like without PreStart? When would the given ActorContext => Behavior block then be executed? On any next message or signal, not just when PreStart is received?

rkuhn commented 7 years ago

Johannes, I am not sure how, but we are definitely talking past each other. Deferred and withContext have exactly the same signature and semantics, do they not? Where is the dichotomy?

johanandren commented 7 years ago

The current Deferred can only be used as the outermost behavior, and fails in runtime if it is not outermost (expected PreStart signal as first signal), while withContext would be usable anywhere. Same signature, different semantics.

ktoso commented 7 years ago

It would be interpreted in a special way same as Same or Unhandled are - the interpreter could invoke it right away then by passing in the context which is how it could be made working even in when not the only "on start".

rkuhn commented 7 years ago

Ah, I see, we are splitting hairs ;-) Because

  1. this is not true (if every recipient of a new Behavior applies Behavior.preStart to it—which is the wart I was referring to earlier today)
  2. it becomes automatically identical after the removal of PreStart (which I am taking as a given at this point)

So, if these are unconvincing to you, then I’d also be fine with the current withContext proposal if the factory is renamed to Deferred :-)


On the interpreter, @jrudolph has a good point in that it is currently easy to mistreat a Behavior. Perhaps we can have the best of both worlds using a sealed abstract class Behavior that does not have message handling methods, plus a non-final abstract class ExtensibleBehavior extends Behavior that is the current Behavior. Then we formalize the small helper methods at the bottom of Behavior.scala into a tiny stateless static interpreter that executes each behavior as appropriate, where ExtensibleBehavior just gets its methods invoked (and the results canonicalized) to allow efficient custom behaviors.

jrudolph commented 7 years ago

@rkuhn I only saw your comment regarding Deferred when I had sent my long answer. Yes, we seem to agree :) Not yet sure what the best names are but we'll get there :)

Regarding the Behavior hierarchy, introducing an ExtensibleBehavior like you propose was something we also talked about IIRC. Seems that could work.

rkuhn commented 7 years ago

Excellent!

patriknw commented 7 years ago

Does that mean that we remove the ctx parameter from the Stateful, or do you still want to keep that?

rkuhn commented 7 years ago

Not offering the binary signature means that every access to the ActorContext necessarily involves an extra closure somewhere. The most composable, encapsulated way would then need to allocate a new closure for every message(*), since the Deferred would be needed within the factory method—passing the context into the method is not desirable for many reasons, including testability.

We should try whether both—unary and binary—can be offered.


(*) Actually, I think it can be avoided, but the code will not look as nice as it otherwise could.

patriknw commented 7 years ago

I have created tickets