add unit test protect ExtractEntityId can be shared safely

cluster-sharding-typed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala

@@ -20,6 +20,7 @@ import java.util.concurrent.CompletionStage
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.concurrent.Future
+import scala.runtime.AbstractPartialFunction
 
 import org.apache.pekko
 import pekko.actor.ActorRefProvider
@@ -172,10 +173,16 @@ import pekko.util.JavaDurationConverters._
       allocationStrategy: Option[ShardAllocationStrategy]): ActorRef[E] = {
 
     val extractorAdapter = new ExtractorAdapter(extractor)
-    val extractEntityId: ShardRegion.ExtractEntityId = {
-      // TODO is it possible to avoid the double evaluation of entityId
-      case message if extractorAdapter.entityId(message) != null =>
-        (extractorAdapter.entityId(message), extractorAdapter.unwrapMessage(message))
+    // !!!important is only applicable if you know that isDefinedAt(x) is always called before apply(x) (with the same x)
+    val extractEntityId: ShardRegion.ExtractEntityId = new AbstractPartialFunction[Any, (String, Any)] {
+      var cache: String = _
+
+      override def isDefinedAt(msg: Any): Boolean = {
+        cache = extractorAdapter.entityId(msg)
+        cache != null
+      }
+
+      override def apply(x: Any): (String, Any) = (cache, extractorAdapter.unwrapMessage(x))
     }
     val extractShardId: ShardRegion.ExtractShardId = { message =>
       extractorAdapter.entityId(message) match {

cluster-sharding/src/main/scala/org/apache/pekko/cluster/sharding/ClusterSharding.scala

@@ -19,6 +19,7 @@ import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.immutable
 import scala.concurrent.Await
+import scala.runtime.AbstractPartialFunction
 import scala.util.control.NonFatal
 
 import org.apache.pekko
@@ -429,15 +430,26 @@ class ClusterSharding(system: ExtendedActorSystem) extends Extension {
       typeName,
       _ => entityProps,
       settings,
-      extractEntityId = {
-        case msg if messageExtractor.entityId(msg) ne null =>
-          (messageExtractor.entityId(msg), messageExtractor.entityMessage(msg))
-      },
+      extractEntityId = extractEntityIdFromExtractor(messageExtractor),
       extractShardId = msg => messageExtractor.shardId(msg),
       allocationStrategy = allocationStrategy,
       handOffStopMessage = handOffStopMessage)
   }
 
+  // !!!important is only applicable if you know that isDefinedAt(x) is always called before apply(x) (with the same x)
+  private def extractEntityIdFromExtractor(
+      messageExtractor: ShardRegion.MessageExtractor): ShardRegion.ExtractEntityId =
+    new AbstractPartialFunction[Any, (String, Any)] {
+      var cache: String = _
+
+      override def isDefinedAt(msg: Any): Boolean = {
+        cache = messageExtractor.entityId(msg)
+        cache != null
+      }
+
+      override def apply(x: Any): (String, Any) = (cache, messageExtractor.entityMessage(x))
+    }
+
   /**
    * Java/Scala API: Register a named entity type by defining the [[pekko.actor.Props]] of the entity actor
    * and functions to extract entity and shard identifier from messages. The [[ShardRegion]] actor
@@ -612,11 +624,12 @@ class ClusterSharding(system: ExtendedActorSystem) extends Extension {
       dataCenter: Optional[String],
       messageExtractor: ShardRegion.MessageExtractor): ActorRef = {
 
-    startProxy(typeName, Option(role.orElse(null)), Option(dataCenter.orElse(null)),
-      extractEntityId = {
-        case msg if messageExtractor.entityId(msg) ne null =>
-          (messageExtractor.entityId(msg), messageExtractor.entityMessage(msg))
-      }, extractShardId = msg => messageExtractor.shardId(msg))
+    startProxy(
+      typeName,
+      Option(role.orElse(null)),
+      Option(dataCenter.orElse(null)),
+      extractEntityId = extractEntityIdFromExtractor(messageExtractor),
+      msg => messageExtractor.shardId(msg))
 
   }
 

cluster-sharding/src/test/scala/org/apache/pekko/cluster/sharding/ShardRegionSpec.scala

@@ -18,12 +18,17 @@ import java.io.File
 import com.typesafe.config.ConfigFactory
 import org.apache.commons.io.FileUtils
 import org.apache.pekko
+import org.apache.pekko.Done
+import org.apache.pekko.cluster.sharding.ShardRegion.MessageExtractor
+import org.apache.pekko.stream.scaladsl.{ Sink, Source }
 import pekko.actor.{ Actor, ActorLogging, ActorRef, ActorSystem, PoisonPill, Props }
 import pekko.cluster.{ Cluster, MemberStatus }
 import pekko.cluster.ClusterEvent.CurrentClusterState
 import pekko.testkit.{ DeadLettersFilter, PekkoSpec, TestProbe, WithLogCapturing }
 import pekko.testkit.TestEvent.Mute
 
+import scala.concurrent.{ ExecutionContext, Future }
+
 object ShardRegionSpec {
   val host = "127.0.0.1"
   val tempConfig =
@@ -54,6 +59,7 @@ object ShardRegionSpec {
   val shardTypeName = "Caat"
 
   val numberOfShards = 3
+  val largerShardNum = 20
 
   val extractEntityId: ShardRegion.ExtractEntityId = {
     case msg: Int => (msg.toString, msg)
@@ -66,11 +72,37 @@ object ShardRegionSpec {
     case _                           => throw new IllegalArgumentException()
   }
 
+  val messageExtractor: MessageExtractor = new MessageExtractor {
+    override def entityId(message: Any): String = message match {
+      case msg: Int => msg.toString
+      case _        => throw new IllegalArgumentException()
+    }
+
+    override def shardId(message: Any): String = message match {
+      case msg: Int => (msg % largerShardNum).toString
+      case _        => throw new IllegalArgumentException()
+    }
+
+    override def entityMessage(message: Any): Any = message
+  }
+
   class EntityActor extends Actor with ActorLogging {
     override def receive: Receive = {
       case msg => sender() ! msg
     }
   }
+
+  class IDMatcherActor extends Actor with ActorLogging {
+    override def receive: Receive = {
+      case msg =>
+        val selfEntityId = self.path.name
+        val msgEntityId = messageExtractor.entityId(msg)
+        if (selfEntityId != msgEntityId) {
+          throw new IllegalStateException(s"EntityId mismatch: $selfEntityId != $msgEntityId")
+        }
+        sender() ! msg
+    }
+  }
 }
 class ShardRegionSpec extends PekkoSpec(ShardRegionSpec.config) with WithLogCapturing {
 
@@ -183,4 +215,43 @@ class ShardRegionSpec extends PekkoSpec(ShardRegionSpec.config) with WithLogCapt
     }
   }
 
+  "ExtractEntityId" must {
+    "can be safely share to multiple shards" in {
+      implicit val ec: ExecutionContext = system.dispatcher
+
+      Cluster(sysA).join(Cluster(sysA).selfAddress) // coordinator on A
+      awaitAssert(Cluster(sysA).selfMember.status shouldEqual MemberStatus.Up, 1.second)
+
+      within(10.seconds) {
+        awaitAssert {
+          Set(sysA).foreach { s =>
+            Cluster(s).sendCurrentClusterState(testActor)
+            expectMsgType[CurrentClusterState].members.size shouldEqual 2
+          }
+        }
+      }
+
+      val shardTypeName = "Doog"
+      val region = ClusterSharding(sysA).start(
+        shardTypeName,
+        Props[IDMatcherActor](),
+        ClusterShardingSettings(system),
+        messageExtractor)
+
+      val total = largerShardNum * 100
+      val source = Source(1 to total)
+
+      val flow = source.mapAsync(parallelism = largerShardNum) { i =>
+        Future {
+          region.tell(i, p1.ref)
+        }
+      }
+
+      val result = flow.runWith(Sink.ignore)
+
+      result.futureValue shouldEqual Done
+      p1.receiveN(total, 10.seconds)
+    }
+  }
+
 }