I prefer Garry's suggestion:

override def preRestart(error:Throwable) : Option[AnyRef] = {
  ...
  Some(...) // return None if nothing is needed
}

in the new actor we could do the following

override def postRestart(error: Throwable, stash: Option[AnyRef]) {
  ...
}

I'd prefer to avoid adding a mutable field to ActorRef ('stash' or 'factory' fields), we want to make it immutable for 2.0, even though a 'stash' field is more generally applicable. preStart/postStart we can just chain without any mutable state.

Yes, I also like the features of this approach, but I thought it would require a breaking change: we cannot change the return type of preRestart without forcing everyone to change their code. Come to think of it: wasn't there a request for receiving the bad message in preRestart?
If we just add new methods, then we could do it like this:

def preRestart(cause: Throwable, currentMessage: Any): Option[AnyRef] = {
  preRestart(cause)
  None
}
def postRestart(cause: Throwable, state: Option[AnyRef]) { postRestart(cause) }

Sounds good?

Re-reading that I notice that my proposal #2 (which is heavily inspired by Derek's) also does not add any mutable fields...

I'm still concerned... using this approach, we've lost control over construction of the Actor in the first place. We're in user-space now, and it's probably unwise to predict their wants. I'm thinking of something like this:

class MyActor(state: SomeType, initResponse: () => Unit) {
  preStart() {
    initResponse()
  }
}

Now, the initial factory might look like this

actorOf(new MyActor(SomeType(...), { delete my state and start fresh }).

Without the ability to replace the factory method, this is going to call this response function and wipe out someone's state. We've kinda lied to them by saying this guy is in its initialized state. I'd much rather have this:

override def alternateFactory() = Some(new MyActor(SomeType(myOldState), {}))
// or maybe I could even call a different constructor altogether.

If I look at the approaches I see that the "pass value around" solution solves the resurrection of mutable state, but I lose control over initial construction. If I use a replacement factory method instead, the resurrection problem is solved, and I keep control over initial construction. So we don't seem to be losing anything at all.

Ah, now I see where you are coming from (that passing in of some "user" code was the missing piece). Do I understand correctly that my second proposal (which is yours with a bit more strictly defined semantics) would be sufficient for your purposes?

Yes, 2 is very close. I don't see why you have to take preRestart and postRestart out though (especially considering the cool 'currentMessage' that you've added above).

If you keep them in then I can do this:

preRestart() {
  .. tell someone how sad I am ..
}
freshInstance = if (stateIsGood) Some(new MyActor(oldState)) else None
postRestart() {
  .. Tell someone how happy I am ..
}

The idea being that preRestart() and postRestart() can be independent of the result of freshInstance().

I would like to add though, that while I used the anonymous function to drive the point home, my use case is actually much different (but less flashy). I have an actor that become()s a number of times in its lifecycle. It's not just mutable data I'm talking about, it's runtime state. I need to be able to get back to that 'current' state deterministically, and it might be impossible (or at least extremely hard) if I don't have control over construction. So while the anonymous function is a use case, I wouldn't want its rarity to be cause to throw out the solution. The 'become()' case is much more common. For example:

...
def init(data: SomeData, stateInfo: String) {
  this(data)
  if (stateInfo == 'GoToRunningState')
    become(running)
}
override def freshInstance = {
  if (stateGood && postInitialized) Some(new MyActor(oldState, 'GoToRunningState'))
  else None
}
...

I meant the second option in the ticket description (def freshInstance). So, to add a tiny variation, what about the following:

def preRestart(cause: Throwable, currentMessage: Any): Option[Actor] = {
  preRestart(cause)
  None
}

If this returns None we do what we always did (get actor from initial factory and call preStart and postRestart on it), but if it returns Some(actor) we just use that without any further invocations. This would keep old code happy as well as solve your problem. You may even call self.become() to reach your execution state, if so desired.
We will, however, have to document that going down this route is a bit more dangerous because proliferation of bad state is not excluded (i.e. a sometimes necessary evil).

Yeah we're on the same page now. Sorry about my blindness :)
If passing the factory out of preRestart works for you then that's cool. Looking at the ActorRef code (I'm only at 1.2 so I'm probably out of date) this looked like more trouble than a separate method, which is why I chose the 'freshInstance' approach (i.e. something more like val a = freshInstance.getOrElse(actorFactory)). But preRestart(cause, currentMessage): Option[Actor] might be better since the currentMessage is passed in as well. We're getting down to the 'tough choice' but 'who cares?' area now, perhaps :)

Sorry about that... Assembla's kinda new to me and it magically updated the state back to -none- / New.

yeah, assembla does have some issues with browser caching ...

So, unless Jonas decides otherwise I will look into the gory details, which hopefully allow what the method signature promises (as I said, I have to look into the self fields issue).

Awesome. Thanks a lot for tackling this issue - it's been a thorn in my side for a few months and it's great to see it being addressed.

Ok. I understand the reasoning behind wanting to add a factory. I think that could be good.

It is starting to get down a dangerous path though, messing with actor instantiation, restoring state etc. in the middle of actor restart.
It is against the let-it-crash philosophy. Reset and continue.

However, if it is not the default usage but does not interfere with standard usage, which should be dead simple, but is there for power-mode, then I think it could be a good addition.

I don't like that preRestart should return the Actor though. Not at all.
I also want to keep that preRestart/postRestart is always invoked, as it is today.

Getting the message that caused the crash is a good addition though, add another ticket for that.

Try to implement the factory proposal as Derek suggested it and leave the callbacks as they are for now.
Do it in a branch and ask for review.

Thanks.

Okay, will try without impacting current semantics and careful documentation of the opt-in change.

Good. Thanks.

(In revision:10256991cbd1e52fbebab5d426dee7331e786652) add Actor.freshInstance hook, test #955

Branch: ticket955

(In revision:a0124d6d5c66befa1139f16ea03aa069547b7e3a) add Actor.freshInstance hook, test #955

Branch: release-1.2

Comment (by rk@rkuhn.info):

(In revision:a0124d6d5c66befa1139f16ea03aa069547b7e3a<http://github.com/jboner/akka/commit/a0124d6d5c66befa1139f16ea03aa069547b7e3a>) add Actor.freshInstance hook, test #955<http://www.assembla.com/spaces/akka/tickets/955>

Branch: release-1.2

More details<http://www.assembla.com/spaces/akka/tickets/955?comment=19613614#comment:19613614>

Assembla | Knowledge, Tools, and Talent for agile teams


---------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

Hi Roland,

I guess this ticket needs to be assigned to someone else to verify it? You can assign it to me if you want and I'll have a look.

Updating tickets (#955, #957)

Branch ticket955 was reviewed by Viktor before porting to release-1.2, now I merged it into master. Please take a look whether the unfortunate lack of proximity in time caused some unwanted side effects.

We should probably do some marketing for the @experimental annotation …

Updating tickets (#818, #821, #823, #836, #842, #854, #856, #865, #866, #867, #868, #869, #871, #872, #873, #877, #878, #879, #885, #888, #889, #890, #894, #910, #911, #917, #923, #924, #925, #926, #927, #928, #930, #931, #932, #933, #934, #936, #937, #952, #955, #957, #958, #959, #960, #961, #964, #965, #966)