You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "Roiocam (via GitHub)" <gi...@apache.org> on 2024/02/27 08:02:09 UTC
[PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Roiocam opened a new pull request, #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156
self-explanatory
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1505388460
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
I don't think it's necessary to verify the `pathPrefixs` several times, because this is not a flaky behavior.
After investigation, I think it is a clear bug, and the unit test explicitly verifies this. Let me explain:
The reason why the path is correct after adding STDOUT is because `toString` is called `path`
https://github.com/apache/incubator-pekko/blob/d884540c92d0d38797c32f217107d97e15cdafbe/actor-typed/src/main/scala/org/apache/pekko/actor/typed/internal/ActorRefImpl.scala#L51-L52
Which initialized the path with refPathPrefix(correctly path):
https://github.com/apache/incubator-pekko/blob/d884540c92d0d38797c32f217107d97e15cdafbe/actor/src/main/scala/org/apache/pekko/pattern/AskSupport.scala#L593-L597
But when toString is not called in the `messageFactory` function apply, all this will not happen. When the Actor terminated, the method call to the path will generate a path without refPathPrefix (that is the place of the bug, and the place where the PR is fixed)
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1505388460
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
I don't think it's necessary to verify the `pathPrefixs` several times, because this is not a flaky behavior.
After investigation, I think it is a clear bug, and the unit test explicitly verifies this. Let me explain:
The reason why the path is correct after adding STDOUT is because `toString` is called `path`
https://github.com/apache/incubator-pekko/blob/d884540c92d0d38797c32f217107d97e15cdafbe/actor-typed/src/main/scala/org/apache/pekko/actor/typed/internal/ActorRefImpl.scala#L52C5-L52C62
Which initialized the path with refPathPrefix(correctly path):
https://github.com/apache/incubator-pekko/blob/d884540c92d0d38797c32f217107d97e15cdafbe/actor/src/main/scala/org/apache/pekko/pattern/AskSupport.scala#L593-L597
But when toString is not called in the `messageFactory` function apply, all this will not happen. When the Actor terminated, the method call to the path will generate a path without refPathPrefix (that is the place of the bug, and the place where the PR is fixed)
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof merged PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#issuecomment-1966106013
Better with a bug report here too.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#issuecomment-1968357165
Once this gets merged, should backport to 1.0.x too.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1504176438
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
random str is ok?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1505391320
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
We completed the ask future(which made PromiseActor terminated) on the Actor message handler and then sent PromiseActorRef to the probe for saving reference.
I think the test was enough for this bug. What do you think? @laglangyue @He-Pin
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#issuecomment-1966016614
LGTM.
I only use Pekko streams. I have read the `path` method, some code of the class and the related test cases.
I am not an expert in Pekko core, so hope more LGTM.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1504126672
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
For this tests, I think we would better test it several times with some generated pathPrefixs.
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
For this test, I think we would better test it several times with some generated pathPrefixs.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1505391320
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
This test was enough for this bug. We completed the ask future(which made PromiseActor terminated) on the Actor message handler, and then sent PromiseActorRef to the probe for saving reference.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#discussion_r1505401371
##########
actor-tests/src/test/scala/org/apache/pekko/pattern/AskSpec.scala:
##########
@@ -266,6 +266,29 @@ class AskSpec extends PekkoSpec("""
val (promiseActorRefForSelection, "ask") = p.expectMsgType[(ActorRef, String)]: @unchecked
promiseActorRefForSelection.path.name should startWith("_user_myName")
}
+
+ "proper path when promise actor terminated" in {
+ implicit val timeout: Timeout = Timeout(300 millis)
+ val p = TestProbe()
+
+ val act = system.actorOf(Props(new Actor {
+ def receive = {
+ case _ =>
+ val senderRef: ActorRef = sender()
+ senderRef ! "complete"
+ p.ref ! senderRef
+ }
+ }), "pathPrefix")
Review Comment:
for me, it's enough for the bugfix.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#issuecomment-1968388653
> Once this gets merged, should backport to 1.0.x too.
I am not sure that this would get classified as a critical fix, @pjfanning @raboof wdyt?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] fix: proper path when promise actor terminated quickly [incubator-pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on PR #1156:
URL: https://github.com/apache/incubator-pekko/pull/1156#issuecomment-1968445532
I don't see a strong reason to backport in this case
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org