improve safety of Props.create by allowing Creator<T>, see #3377

Props constructors need to be deprecated instead of being mutated
because we cannot just start throwing exceptions in people’s existing
code. Props.withCreator is deprecated for similar reasons, but also
because Props are about the creators, so replacing that after the fact
is not good style.

akka-actor-tests/src/test/java/akka/actor/ActorCreationTest.java

@@ -0,0 +1,68 @@
+/**
+ *   Copyright (C) 2009-2013 Typesafe Inc. <http://www.typesafe.com>
+ */
+
+package akka.actor;
+
+import static org.junit.Assert.*;
+import org.junit.Test;
+
+import akka.japi.Creator;
+
+public class ActorCreationTest {
+
+  @Test
+  public void testWrongCreator() {
+    try {
+      Props.create(new Creator<Actor>() {
+        @Override
+        public Actor create() throws Exception {
+          return null;
+        }
+      });
+      assert false;
+    } catch(IllegalArgumentException e) {
+      assertEquals("cannot use non-static local Creator to create actors; make it static or top-level", e.getMessage());
+    }
+  }
+  
+  static class C implements Creator<UntypedActor> {
+    @Override
+    public UntypedActor create() throws Exception {
+      return null;
+    }
+  }
+  
+  static class D<T> implements Creator<T> {
+    @Override
+    public T create() {
+      return null;
+    }
+  }
+  
+  static class E<T extends UntypedActor> implements Creator<T> {
+    @Override
+    public T create() {
+      return null;
+    }
+  }
+  
+  @Test
+  public void testRightCreator() {
+    final Props p = Props.create(new C());
+    assertEquals(UntypedActor.class, p.actorClass());
+  }
+  
+  @Test
+  public void testParametricCreator() {
+    final Props p = Props.create(new D<UntypedActor>());
+    assertEquals(Actor.class, p.actorClass());
+  }
+  
+  @Test
+  public void testBoundedCreator() {
+    final Props p = Props.create(new E<UntypedActor>());
+    assertEquals(UntypedActor.class, p.actorClass());
+  }
+  
+}

akka-actor/src/main/scala/akka/actor/Props.scala

@@ -12,9 +12,12 @@ import akka.util.Reflect
 import scala.annotation.varargs
 import Deploy.{ NoDispatcherGiven, NoMailboxGiven }
 import scala.collection.immutable
-
 import scala.language.existentials
 import java.lang.reflect.Constructor
+import java.lang.reflect.Modifier
+import scala.annotation.tailrec
+import java.lang.reflect.ParameterizedType
+import java.lang.reflect.TypeVariable
 
 /**
  * Factory for Props instances.
@@ -40,16 +43,16 @@ object Props {
    */
   final val defaultDeploy = Deploy()
 
-  /**
-   * The default Props instance, uses the settings from the Props object starting with default*.
-   */
-  final val default = new Props()
-
   /**
    * A Props instance whose creator will create an actor that doesn't respond to any message
    */
   final val empty = Props[EmptyActor]
 
+  /**
+   * The default Props instance, uses the settings from the Props object starting with default*.
+   */
+  final val default = new Props()
+
   /**
    * INTERNAL API
    *
@@ -85,7 +88,7 @@ object Props {
   /**
    * The deprecated legacy constructor.
    */
-  @deprecated("give class and arguments instead", "2.2")
+  @deprecated("use Props.withDispatcher and friends", "2.2")
   def apply(
     creator: () ⇒ Actor = Props.defaultCreator,
     dispatcher: String = Dispatchers.DefaultDispatcherId,
@@ -98,6 +101,13 @@ object Props {
     if (d2 != Props.defaultDeploy) p.withDeploy(d2) else p
   }
 
+  /**
+   * The deprecated legacy extractor.
+   */
+  @deprecated("use three-argument version", "2.2")
+  def unapply(p: Props)(dummy: Int = 0): Option[(() ⇒ Actor, String, RouterConfig, Deploy)] =
+    Some((p.creator, p.dispatcher, p.routerConfig, p.deploy))
+
   /**
    * Scala API: create a Props given a class and its constructor arguments.
    */
@@ -108,6 +118,30 @@ object Props {
    */
   @varargs
   def create(clazz: Class[_], args: AnyRef*): Props = apply(defaultDeploy, clazz, args.toVector)
+
+  /**
+   * Create new Props from the given [[Creator]].
+   */
+  def create[T <: Actor](creator: Creator[T]): Props = {
+    if ((creator.getClass.getModifiers & Modifier.STATIC) == 0)
+      throw new IllegalArgumentException("cannot use non-static local Creator to create actors; make it static or top-level")
+    val cc = classOf[Creator[_]]
+    val ac = classOf[Actor]
+    @tailrec def findType(c: Class[_]): Class[_] = {
+      c.getGenericInterfaces collectFirst {
+        case t: ParameterizedType if cc.isAssignableFrom(t.getRawType.asInstanceOf[Class[_]]) ⇒
+          t.getActualTypeArguments.head match {
+            case c: Class[_] ⇒ c // since T <: Actor
+            case v: TypeVariable[_] ⇒
+              v.getBounds collectFirst { case c: Class[_] if ac.isAssignableFrom(c) && c != ac ⇒ c } getOrElse ac
+          }
+      } match {
+        case Some(x) ⇒ x
+        case None    ⇒ findType(c.getSuperclass)
+      }
+    }
+    apply(defaultDeploy, classOf[CreatorConsumer], findType(creator.getClass) :: creator :: Nil)
+  }
 }
 
 /**
@@ -183,7 +217,7 @@ final case class Props(deploy: Deploy, clazz: Class[_], args: immutable.Seq[Any]
    *             non-serializable inner classes, making them also
    *             non-serializable
    */
-  @deprecated("use Props.create", "2.2")
+  @deprecated("use Props.create()", "2.2")
   def this(factory: UntypedActorFactory) = this(Props.defaultDeploy, classOf[UntypedActorFactoryConsumer], Vector(factory))
 
   /**
@@ -227,6 +261,7 @@ final case class Props(deploy: Deploy, clazz: Class[_], args: immutable.Seq[Any]
    *
    * The creator must not return the same instance multiple times.
    */
+  @deprecated("use Props(...).withDeploy(other.deploy)", "2.2")
   def withCreator(c: ⇒ Actor): Props = copy(clazz = classOf[CreatorFunctionConsumer], args = (() ⇒ c) :: Nil)
 
   /**
@@ -239,8 +274,8 @@ final case class Props(deploy: Deploy, clazz: Class[_], args: immutable.Seq[Any]
    *             non-serializable inner classes, making them also
    *             non-serializable
    */
-  @deprecated("use Props.create(clazz, args ...) instead", "2.2")
-  def withCreator(c: Creator[Actor]): Props = copy(clazz = classOf[CreatorConsumer], args = c :: Nil)
+  @deprecated("use Props.create(clazz, args ...).withDeploy(other.deploy) instead", "2.2")
+  def withCreator(c: Creator[Actor]): Props = copy(clazz = classOf[CreatorConsumer], args = classOf[Actor] :: c :: Nil)
 
   /**
    * Returns a new Props with the specified creator set.
@@ -248,7 +283,7 @@ final case class Props(deploy: Deploy, clazz: Class[_], args: immutable.Seq[Any]
    * @deprecated use Props.create(clazz) instead; deprecated since it duplicates
    *             another API
    */
-  @deprecated("use Props(clazz, args).withDeploy(other.deploy)", "2.2")
+  @deprecated("use Props.create(clazz, args).withDeploy(other.deploy)", "2.2")
   def withCreator(c: Class[_ <: Actor]): Props = copy(clazz = c, args = Nil)
 
   /**
@@ -342,8 +377,8 @@ private[akka] class CreatorFunctionConsumer(creator: () ⇒ Actor) extends Indir
 /**
  * INTERNAL API
  */
-private[akka] class CreatorConsumer(creator: Creator[Actor]) extends IndirectActorProducer {
-  override def actorClass = classOf[Actor]
+private[akka] class CreatorConsumer(clazz: Class[_ <: Actor], creator: Creator[Actor]) extends IndirectActorProducer {
+  override def actorClass = clazz
   override def produce() = creator.create()
 }
 

akka-actor/src/main/scala/akka/actor/UntypedActor.scala

@@ -166,5 +166,5 @@ abstract class UntypedActor extends Actor {
 /**
  * Factory closure for an UntypedActor, to be used with 'Actors.actorOf(factory)'.
  */
-@deprecated("use Props.create(clazz, args) instead", "2.2")
+@deprecated("use Creator<T> instead", "2.2")
 trait UntypedActorFactory extends Creator[Actor] with Serializable

akka-actor/src/main/scala/akka/actor/dsl/Creators.scala

@@ -14,7 +14,6 @@ import java.util.concurrent.TimeoutException
 import java.util.concurrent.atomic.AtomicInteger
 import akka.pattern.ask
 import scala.reflect.ClassTag
-import akka.dispatch.{ UnboundedDequeBasedMailbox, RequiresMessageQueue }
 
 trait Creators { this: ActorDSL.type ⇒
 

akka-actor/src/main/scala/akka/japi/JavaAPI.scala

@@ -48,7 +48,8 @@ trait Effect {
 /**
  * A constructor/factory, takes no parameters but creates a new value of type T every call.
  */
-trait Creator[T] {
+@SerialVersionUID(1L)
+trait Creator[T] extends Serializable {
   /**
    * This method must return a different instance upon every call.
    */

akka-docs/rst/java/code/docs/actor/UntypedActorDocTest.java

@@ -49,6 +49,7 @@ import akka.actor.IndirectActorProducer;
 import akka.actor.OneForOneStrategy;
 //#import-props
 import akka.actor.Props;
+import akka.japi.Creator;
 //#import-props
 import akka.actor.SupervisorStrategy;
 import akka.actor.SupervisorStrategy.Directive;
@@ -88,14 +89,35 @@ public class UntypedActorDocTest {
 
   private final ActorSystem system = actorSystemResource.getSystem();
 
+  //#creating-props-config
+  static class MyActorC implements Creator<MyActor> {
+    @Override public MyActor create() {
+      return new MyActor("...");
+    }
+  }
+  
+  //#creating-props-config
+
   @SuppressWarnings("unused")
   @Test
   public void createProps() {
     //#creating-props-config
     Props props1 = Props.create(MyUntypedActor.class);
     Props props2 = Props.create(MyActor.class, "...");
+    Props props3 = Props.create(new MyActorC());
     //#creating-props-config
   }
+  
+  //#parametric-creator
+  static class ParametricCreator<T extends MyActor> implements Creator<T> {
+    @Override public T create() {
+      // ... fabricate actor here
+      //#parametric-creator
+      return null;
+      //#parametric-creator
+    }
+  }
+  //#parametric-creator
 
   @SuppressWarnings("deprecation")
   @Test

akka-docs/rst/java/untyped-actors.rst

@@ -51,12 +51,20 @@ dispatcher to use, see more below). Here are some examples of how to create a
 .. includecode:: code/docs/actor/UntypedActorDocTest.java#import-props
 .. includecode:: code/docs/actor/UntypedActorDocTest.java#creating-props-config
 
-The last line shows how to pass constructor arguments to the :class:`Actor`
+The second line shows how to pass constructor arguments to the :class:`Actor`
 being created. The presence of a matching constructor is verified during
 construction of the :class:`Props` object, resulting in an
 :class:`IllegalArgumentEception` if no or multiple matching constructors are
 found.
 
+The third line demonstrates the use of a :class:`Creator<T extends Actor>`. The
+creator class must be static, which is verified during :class:`Props`
+construction. The type parameter’s upper bound is used to determine the
+produced actor class, falling back to :class:`Actor` if fully erased. An
+example of a parametric factory could be:
+
+.. includecode:: code/docs/actor/UntypedActorDocTest.java#parametric-creator
+
 Deprecated Variants
 ^^^^^^^^^^^^^^^^^^^
 

akka-docs/rst/project/migration-guide-2.1.x-2.2.x.rst

@@ -18,17 +18,57 @@ Deprecated Closure-Taking Props
 are usually created in-line and thus carry a reference to their enclosing
 object; this is not well known among programmers, in particular it can be
 surprising that innocent-looking actor creation should not be serializable,
-e.g. if the enclosing class is an actor.
+e.g. if the enclosing class is an actor. Another issue which came up often
+during reviews is that these actor creators inadvertedly close over the Actor’s
+``this`` reference for calling methods on it, which is inherently unsafe.
 
-Thus we have decided to deprecate ``Props(new MyActor(...))`` and
-:class:`UntypedActorFactory` in favor of basing :class:`Props` on a
-:class:`Class` and a sequence of constructor arguments. This has the added
-benefit of allowing easier integration with dependency injection frameworks,
-see :ref:`actor-create-factory`.
+Another reason for changing the underlying implementation is that Props now
+carries information about which class of actor will be created, allowing the
+extraction of mailbox type requirements (e.g. when using the Stash) before
+trying to create the actor. Being based on the actor class and a list of
+constructor arguments also allows these arguments to be serialized according to
+the configured serializer bindings instead of mandating Java serialization
+(which was used previously).
 
-The deprecated methods will be retained until the possibility of reintroducing
-a similar syntax in a safe fashion has been properly researched (in case of
-Scala it might be possible to use macros to this effect).
+What changes for Java?
+----------------------
+
+A new method ``Props.create`` has been introduced with two overloads::
+
+  Props.create(MyActor.class, arg1, arg2, ...);
+  // or
+  Props.create(new MyActorCreator(args ...));
+
+In the first case the existence of a constructor signature matching the
+supplied arguments is verified at Props construction time. In the second case
+it is verified that ``MyActorCreator`` (which must be a ``akka.japi.Creator<?
+extends Actor>``) is a static class. In both cases failure is signaled by
+throwing a :class:`IllegalArgumentException`.
+
+The constructors of :class:`Props` have been deprecated to facilitate migration.
+
+The :meth:`withCreator` methods have been deprecated. The functionality is
+available by using ``Props.create(...).withDeploy(oldProps.deploy());``.
+
+:class:`UntypedActorFactory` has been deprecated in favor of the more precisely
+typed :class:`Creator<T>`.
+
+What changes for Scala?
+-----------------------
+
+The case class signature of Props has been changed to only contain a
+:class:`Deploy`, a :class:`Class[_]` and an immutable :class:`Seq[Any]` (the
+constructor arguments for the class). The old factory and extractor methods
+have been deprecated.
+
+Properly serializable :class:`Props` can now be created for actors which take
+constructor arguments by using ``Props(classOf[MyActor], arg1, arg2, ...)``.
+In a future update—possibly within the 2.2.x timeframe—we plan to introduce a
+macro which will transform the by-name argument to ``Props(new MyActor(...))``
+into a call to the former.
+
+The :meth:`withCreator` methods have been deprecated. The functionality is
+available by using ``Props(...).withDeploy(oldProps.deploy)``.
 
 Immutable everywhere
 ====================

akka-docs/rst/scala/actors.rst

@@ -85,18 +85,21 @@ for a migration period):
 
 .. includecode:: code/docs/actor/ActorDocSpec.scala#creating-props-deprecated
 
-The last one is deprecated because its functionality is available in full
-through :meth:`Props.apply()`.
+The first one is deprecated because the case class structure changed between
+Akka 2.1 and 2.2.
 
-The first three are deprecated because the captured closure is a local class
-which means that it implicitly carries a reference to the enclosing class. This
-can easily make the resulting :class:`Props` non-serializable, e.g. when the
-enclosing class is an :class:`Actor`. Akka advocates location transparency,
-meaning that an application written with actors should just work when it is
-deployed over multiple network nodes, and non-serializable actor factories
-would break this principle. In case indirect actor creation is needed—for
-example when using dependency injection—there is the possibility to use an
-:class:`IndirectActorProducer` as described below.
+The two variants in the middle are deprecated because :class:`Props` are
+primarily concerned with actor creation and thus the “creator” part should be
+explicitly set when creating an instance. In case you want to deploy one actor
+in the same was as another, simply use
+``Props(...).withDeploy(otherProps.deploy)``.
+
+The last one is not technically deprecated, but it is not recommended because
+it encourages to close over the enclosing scope, resulting in non-serializable
+:class:`Props` and possibly race conditions (breaking the actor encapsulation).
+We will provide a macro-based solution in a future release which allows similar
+syntax without the headaches, at which point this variant will be properly
+deprecated.
 
 There were two use-cases for these methods: passing constructor arguments to
 the actor—which is solved by the newly introduced

akka-docs/rst/scala/code/docs/actor/ActorDocSpec.scala

@@ -243,19 +243,19 @@ class ActorDocSpec extends AkkaSpec(Map("akka.loglevel" -> "INFO")) {
     //#creating-props
 
     //#creating-props-deprecated
-    // DEPRECATED: encourages to close over enclosing class
+    // DEPRECATED: old case class signature
     val props4 = Props(
       creator = { () ⇒ new MyActor },
       dispatcher = "my-dispatcher")
 
-    // DEPRECATED: encourages to close over enclosing class
+    // DEPRECATED due to duplicate functionality with Props.apply()
     val props5 = props1.withCreator(new MyActor)
 
-    // DEPRECATED: encourages to close over enclosing class
-    val props6 = Props(new MyActor)
-
     // DEPRECATED due to duplicate functionality with Props.apply()
-    val props7 = props1.withCreator(classOf[MyActor])
+    val props6 = props1.withCreator(classOf[MyActor])
+
+    // NOT RECOMMENDED: encourages to close over enclosing class
+    val props7 = Props(new MyActor)
     //#creating-props-deprecated
   }
 

project/AkkaBuild.scala

@@ -136,7 +136,8 @@ object AkkaBuild extends Build {
     dependencies = Seq(testkit % "compile;test->test"),
     settings = defaultSettings ++ scaladocSettings  ++ Seq(
       publishArtifact in Compile := false,
-      libraryDependencies ++= Dependencies.actorTests
+      libraryDependencies ++= Dependencies.actorTests,
+      testOptions += Tests.Argument(TestFrameworks.JUnit, "-v", "-a")
     )
   )
 
@@ -972,7 +973,7 @@ object Dependencies {
 
   val testkit = Seq(Test.junit, Test.scalatest)
 
-  val actorTests = Seq(Test.junit, Test.scalatest, Test.commonsCodec, Test.commonsMath, Test.mockito, Test.scalacheck, protobuf)
+  val actorTests = Seq(Test.junit, Test.scalatest, Test.commonsCodec, Test.commonsMath, Test.mockito, Test.scalacheck, protobuf, Test.junitIntf)
 
   val remote = Seq(netty, protobuf, uncommonsMath, Test.junit, Test.scalatest)