He-Pin commented on code in PR #3652:
URL: https://github.com/apache/pekko/pull/3652#discussion_r4355885614
##########
remote/src/test/scala/org/apache/pekko/remote/artery/RemoteConnectionSpec.scala:
##########
@@ -31,14 +36,181 @@ class RemoteConnectionSpec extends
ArteryMultiNodeSpec("pekko.remote.retry-gate-
"Remoting between systems" should {
+ "handle uid collision when connection TO two systems with same uid" in {
+ val localProbe = new TestProbe(localSystem)
+ val localPort =
RARP(localSystem).provider.getDefaultAddress.getPort().get
+
+ val conflictUid = localSystem.asInstanceOf[ExtendedActorSystem].uid + 1
+ val echoName = "echoA"
+
+ val remotePort1 = freePort()
+ val remoteSystem1 =
+ newRemoteSystem(extraConfig = Some(s"""
+ pekko.test-only-uid = $conflictUid
+ pekko.remote.artery.canonical.port=$remotePort1
+ """))
+ val remote1Probe = new TestProbe(remoteSystem1)
+
+ val remotePort2 = freePort()
+ val remoteSystem2 =
+ newRemoteSystem(extraConfig = Some(s"""
+ // same uid as remoteSystem1
+ pekko.test-only-uid = $conflictUid
+ pekko.remote.artery.canonical.port=$remotePort2
+ """))
+ val remote2Probe = new TestProbe(remoteSystem2)
+
+ localProbe.expectNoMessage(2.seconds)
+ localSystem.actorOf(TestActors.echoActorProps, echoName)
+ remoteSystem1.actorOf(TestActors.echoActorProps, echoName)
+ remoteSystem2.actorOf(TestActors.echoActorProps, echoName)
+
+ val selectionFromLocalToRemote1 =
+
localSystem.actorSelection(s"pekko://${remoteSystem1.name}@localhost:$remotePort1/user/$echoName")
+ val selectionFromLocalToRemote2 =
+
localSystem.actorSelection(s"pekko://${remoteSystem2.name}@localhost:$remotePort2/user/$echoName")
+ val selectionFromRemote1ToLocal =
+
remoteSystem1.actorSelection(s"pekko://${localSystem.name}@localhost:$localPort/user/$echoName")
+ val selectionFromRemote2ToLocal =
+
remoteSystem2.actorSelection(s"pekko://${localSystem.name}@localhost:$localPort/user/$echoName")
+
+ selectionFromLocalToRemote1.tell("ping1a", localProbe.ref)
+ localProbe.expectMsg(500.millis, "ping1a")
+
+ selectionFromRemote1ToLocal.tell("ping1b", remote1Probe.ref)
+ remote1Probe.expectMsg(500.millis, "ping1b")
+
+ EventFilter[UidCollisionException]().intercept {
+ selectionFromLocalToRemote2.tell("ping2a", localProbe.ref)
+ // doesn't get through
+ localProbe.expectNoMessage()
+ }(localSystem)
+ RARP(localSystem).provider.transport
+ .asInstanceOf[ArteryTransport]
+ .association(RARP(remoteSystem2).provider.getDefaultAddress)
+ .associationState
+ .uniqueRemoteAddressState() shouldBe UidUnknown // handshake not
completed
+
+ EventFilter[UidCollisionException]().intercept {
+ selectionFromRemote2ToLocal.tell("ping2b", remote2Probe.ref)
+ // doesn't get through in other direction
+ remote2Probe.expectNoMessage()
Review Comment:
Please also assert delivery at the receiving actor. A missing echo can hide
a one-way delivery: with three ordinary local ActorSystems using test UIDs
1000/2000/2000, I observed the receiver remain UidUnknown for the colliding
peer while that peer knew the receiver. A message from the colliding peer
reached the receiver actor; its reply did not return. The healthy peer still
worked.
The receiver's collision path at `ArteryTransport.scala:789-793` invokes
quarantine while the new association has no known UID, so that call does not
exclude it. The inbound decoder resolves the duplicate origin UID to the
existing healthy association (`Codecs.scala:427-428`), allowing the message
onward. Please cover receiver-side non-delivery and fix this
accidental-collision reliability case while keeping the healthy association
intact. This is an exact-head observation; I have not established it as a
regression relative to main.
##########
remote/src/test/scala/org/apache/pekko/remote/artery/RemoteConnectionSpec.scala:
##########
@@ -31,14 +36,181 @@ class RemoteConnectionSpec extends
ArteryMultiNodeSpec("pekko.remote.retry-gate-
"Remoting between systems" should {
+ "handle uid collision when connection TO two systems with same uid" in {
+ val localProbe = new TestProbe(localSystem)
+ val localPort =
RARP(localSystem).provider.getDefaultAddress.getPort().get
+
+ val conflictUid = localSystem.asInstanceOf[ExtendedActorSystem].uid + 1
+ val echoName = "echoA"
+
+ val remotePort1 = freePort()
+ val remoteSystem1 =
+ newRemoteSystem(extraConfig = Some(s"""
+ pekko.test-only-uid = $conflictUid
+ pekko.remote.artery.canonical.port=$remotePort1
+ """))
+ val remote1Probe = new TestProbe(remoteSystem1)
+
+ val remotePort2 = freePort()
+ val remoteSystem2 =
+ newRemoteSystem(extraConfig = Some(s"""
+ // same uid as remoteSystem1
+ pekko.test-only-uid = $conflictUid
+ pekko.remote.artery.canonical.port=$remotePort2
+ """))
+ val remote2Probe = new TestProbe(remoteSystem2)
+
+ localProbe.expectNoMessage(2.seconds)
+ localSystem.actorOf(TestActors.echoActorProps, echoName)
+ remoteSystem1.actorOf(TestActors.echoActorProps, echoName)
+ remoteSystem2.actorOf(TestActors.echoActorProps, echoName)
+
+ val selectionFromLocalToRemote1 =
+
localSystem.actorSelection(s"pekko://${remoteSystem1.name}@localhost:$remotePort1/user/$echoName")
+ val selectionFromLocalToRemote2 =
+
localSystem.actorSelection(s"pekko://${remoteSystem2.name}@localhost:$remotePort2/user/$echoName")
+ val selectionFromRemote1ToLocal =
+
remoteSystem1.actorSelection(s"pekko://${localSystem.name}@localhost:$localPort/user/$echoName")
+ val selectionFromRemote2ToLocal =
+
remoteSystem2.actorSelection(s"pekko://${localSystem.name}@localhost:$localPort/user/$echoName")
+
+ selectionFromLocalToRemote1.tell("ping1a", localProbe.ref)
+ localProbe.expectMsg(500.millis, "ping1a")
+
+ selectionFromRemote1ToLocal.tell("ping1b", remote1Probe.ref)
+ remote1Probe.expectMsg(500.millis, "ping1b")
+
+ EventFilter[UidCollisionException]().intercept {
Review Comment:
This filter does not assert a collision:
`EventFilter[UidCollisionException]()` defaults to unlimited occurrences and
matches Error events with an exception, while `ArteryTransport.scala:790` emits
a Warning containing only `exc.getMessage`. The same issue appears at lines 94,
166, and 177. Please use a warning filter with an explicit expected count and a
controlled handshake attempt, so removing the warning makes the test fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
