From 63b8c33483ab07ce77b4e6f5c376c0e3678e489b Mon Sep 17 00:00:00 2001 From: Viktor Klang Date: Mon, 11 Mar 2013 11:39:07 +0100 Subject: [PATCH] #3114 - Replace props on ActorCell termination to avoid leakage if one holds onto LocalActorRefs. --- .../actor/LocalActorRefProviderSpec.scala | 18 ++++++++ .../src/main/scala/akka/actor/ActorCell.scala | 45 ++++++++++--------- .../akka/actor/dungeon/FaultHandling.scala | 7 +-- .../src/main/scala/akka/routing/Routing.scala | 4 +- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/akka-actor-tests/src/test/scala/akka/actor/LocalActorRefProviderSpec.scala b/akka-actor-tests/src/test/scala/akka/actor/LocalActorRefProviderSpec.scala index 314739e78e..8fc765f9d8 100644 --- a/akka-actor-tests/src/test/scala/akka/actor/LocalActorRefProviderSpec.scala +++ b/akka-actor-tests/src/test/scala/akka/actor/LocalActorRefProviderSpec.scala @@ -57,6 +57,24 @@ class LocalActorRefProviderSpec extends AkkaSpec(LocalActorRefProviderSpec.confi } + "A LocalActorRef's ActorCell" must { + "not retain its original Props when terminated" in { + val GetChild = "GetChild" + val a = watch(system.actorOf(Props(new Actor { + val child = context.actorOf(Props.empty) + def receive = { case `GetChild` ⇒ sender ! child } + }))) + a.tell(GetChild, testActor) + val child = expectMsgType[ActorRef] + child.asInstanceOf[LocalActorRef].underlying.props must be theSameInstanceAs Props.empty + system stop a + expectMsgType[Terminated] + val childProps = child.asInstanceOf[LocalActorRef].underlying.props + childProps must not be theSameInstanceAs(Props.empty) + childProps must be theSameInstanceAs ActorCell.terminatedProps + } + } + "An ActorRefFactory" must { implicit val ec = system.dispatcher "only create one instance of an actor with a specific address in a concurrent environment" in { diff --git a/akka-actor/src/main/scala/akka/actor/ActorCell.scala b/akka-actor/src/main/scala/akka/actor/ActorCell.scala index a48d5c4c45..d7d0338bd2 100644 --- a/akka-actor/src/main/scala/akka/actor/ActorCell.scala +++ b/akka-actor/src/main/scala/akka/actor/ActorCell.scala @@ -304,6 +304,8 @@ private[akka] object ActorCell { final val emptyBehaviorStack: List[Actor.Receive] = Nil final val emptyActorRefSet: Set[ActorRef] = immutable.TreeSet.empty + + final val terminatedProps: Props = Props(() ⇒ throw new IllegalActorStateException("This Actor has been terminated")) } //ACTORCELL IS 64bytes and should stay that way unless very good reason not to (machine sympathy, cache line fit) @@ -521,36 +523,35 @@ private[akka] class ActorCell( case _ ⇒ } + @tailrec private final def lookupAndSetField(clazz: Class[_], instance: AnyRef, name: String, value: Any): Boolean = { + if (try { + val field = clazz.getDeclaredField(name) + field.setAccessible(true) + field.set(instance, value) + true + } catch { + case e: NoSuchFieldException ⇒ false + }) true + else if (clazz.getSuperclass eq null) false + else lookupAndSetField(clazz.getSuperclass, instance, name, value) + } + + final protected def clearActorCellFields(cell: ActorCell): Unit = + if (!lookupAndSetField(cell.getClass, cell, "props", ActorCell.terminatedProps)) + throw new IllegalArgumentException("ActorCell has no props field") + final protected def clearActorFields(actorInstance: Actor): Unit = { setActorFields(actorInstance, context = null, self = system.deadLetters) currentMessage = null behaviorStack = emptyBehaviorStack } - final protected def setActorFields(actorInstance: Actor, context: ActorContext, self: ActorRef) { - @tailrec - def lookupAndSetField(clazz: Class[_], actor: Actor, name: String, value: Any): Boolean = { - val success = try { - val field = clazz.getDeclaredField(name) - field.setAccessible(true) - field.set(actor, value) - true - } catch { - case e: NoSuchFieldException ⇒ false - } - - if (success) true - else { - val parent: Class[_] = clazz.getSuperclass - if (parent eq null) throw new IllegalActorStateException(actorInstance.getClass + " is not an Actor since it have not mixed in the 'Actor' trait") - lookupAndSetField(parent, actor, name, value) - } - } + final protected def setActorFields(actorInstance: Actor, context: ActorContext, self: ActorRef): Unit = if (actorInstance ne null) { - lookupAndSetField(actorInstance.getClass, actorInstance, "context", context) - lookupAndSetField(actorInstance.getClass, actorInstance, "self", self) + if (!lookupAndSetField(actorInstance.getClass, actorInstance, "context", context) + || !lookupAndSetField(actorInstance.getClass, actorInstance, "self", self)) + throw new IllegalActorStateException(actorInstance.getClass + " is not an Actor since it have not mixed in the 'Actor' trait") } - } // logging is not the main purpose, and if it fails there’s nothing we can do protected final def publish(e: LogEvent): Unit = try system.eventStream.publish(e) catch { case NonFatal(_) ⇒ } diff --git a/akka-actor/src/main/scala/akka/actor/dungeon/FaultHandling.scala b/akka-actor/src/main/scala/akka/actor/dungeon/FaultHandling.scala index 053765296a..78100eae1a 100644 --- a/akka-actor/src/main/scala/akka/actor/dungeon/FaultHandling.scala +++ b/akka-actor/src/main/scala/akka/actor/dungeon/FaultHandling.scala @@ -195,9 +195,8 @@ private[akka] trait FaultHandling { this: ActorCell ⇒ * specific order. */ try if (a ne null) a.postStop() - catch handleNonFatalOrInterruptedException { e ⇒ - publish(Error(e, self.path.toString, clazz(a), e.getMessage)) - } finally try dispatcher.detach(this) + catch handleNonFatalOrInterruptedException { e ⇒ publish(Error(e, self.path.toString, clazz(a), e.getMessage)) } + finally try dispatcher.detach(this) finally try parent.sendSystemMessage(ChildTerminated(self)) finally try parent ! NullMessage // read ScalaDoc of NullMessage to see why finally try tellWatchersWeDied(a) @@ -205,7 +204,9 @@ private[akka] trait FaultHandling { this: ActorCell ⇒ finally { if (system.settings.DebugLifecycle) publish(Debug(self.path.toString, clazz(a), "stopped")) + clearActorFields(a) + clearActorCellFields(this) actor = null } } diff --git a/akka-actor/src/main/scala/akka/routing/Routing.scala b/akka-actor/src/main/scala/akka/routing/Routing.scala index 2d2a84c444..f09168ed73 100644 --- a/akka-actor/src/main/scala/akka/routing/Routing.scala +++ b/akka-actor/src/main/scala/akka/routing/Routing.scala @@ -33,9 +33,7 @@ private[akka] class RoutedActorRef(_system: ActorSystemImpl, _props: Props, _sup throw new ConfigurationException( "Configuration for " + this + " is invalid - you can not use a 'BalancingDispatcher' as a Router's dispatcher, you can however use it for the routees.") - } - - _props.routerConfig.verifyConfig() + } else _props.routerConfig.verifyConfig() override def newCell(old: UnstartedCell): Cell = new RoutedActorCell(system, this, props, supervisor).init(old.uid, sendSupervise = false)