fix: confusion deadletter while ask timeout #668 #664
Conversation
| @@ -617,7 +617,7 @@ private[pekko] final class PromiseActorRef( | |||
|
|
|||
| override def !(message: Any)(implicit sender: ActorRef = Actor.noSender): Unit = state match { | |||
| case Stopped | _: StoppedWithPath => | |||
| provider.deadLetters ! message | |||
| provider.deadLetters ! DeadLetter(message, if (sender eq Actor.noSender) provider.deadLetters else sender, this) | |||
There was a problem hiding this comment.
In fact, the sender will always be empty because in typed, the passing of the sender is changed from explicitly on method calling to optional choice as a field in the message, unless we can extract the value with the same field name (such as sender, replyTo) from the user's message and use it here.
There was a problem hiding this comment.
This breaks the Scala 3 build and possibly other builds.
actor/src/main/scala/org/apache/pekko/pattern/AskSupport.scala has been like this for years - changing it now seems like a bad idea.
I will not agree to unblock this change without major community support in favour of it.
have you read the original issue(29795) on akka yet? Even if it doesn't convert into a DeadLetter and send it, the Furthermore, in typed, this would result in ask from anywhere not showing the destination but instead showing deadLetterActor path. I don't think this is more helpful for troubleshooting. |
|
@pjfanning those line may help your understand what am i say and my motivation. |
|
Could you at least fix the build issue? This PR does not compile when Scala 3 is used. |
Do not ask a Pekko developer to look at Akka. We could be sued be for looking at Akka code. |
Of course, otherwise this PR would not have passed the checks. However, I need some time to set up my environment. It's already early morning in my timezone, so I will fix this PR during my free time tomorrow. |
I'm sorry, I am not aware of this. |
|
@Roiocam Would you like to openup an issue here, thanks and welcome |
|
The fix looks good but the tests need some update, and would be nice to have a issue to track the problem too. |
Roiocam
changed the title
|
@He-Pin PTAL, and why pekko PR action requires approval from a maintainer. |
Because this is the first time, it was in Akka too. |
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/DeadLetterSpec.scala
Outdated
Show resolved
Hide resolved
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/DeadLetterSpec.scala
Outdated
Show resolved
Hide resolved
| import org.apache.pekko.actor.typed.scaladsl.AskPattern.{ schedulerFromActorSystem, Askable } | ||
| import org.apache.pekko.actor.typed.scaladsl.Behaviors | ||
| import org.apache.pekko.actor.typed.scaladsl.Behaviors._ | ||
| import org.apache.pekko.util.Timeout |
There was a problem hiding this comment.
@mdedetrich How to format these imports as the style your suggested, is there any sbt command?
There was a problem hiding this comment.
It appears that the contribution guide mentions this, but sortImports sort too many files... that not correct behavior.
https://github.com/apache/incubator-pekko/blob/main/CONTRIBUTING.md#scala-style
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/DeadLetterSpec.scala
Outdated
Show resolved
Hide resolved
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/DeadLetterSpec.scala
Outdated
Show resolved
Hide resolved
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/DeadLetterSpec.scala
Outdated
Show resolved
Hide resolved
|
@Roiocam Could you fill in a CLA? With changes like this, it seems safer to have a CLA on file. |
|
|
||
| class ManualTerminatedTestSetup(workerCnt: Int) { | ||
| implicit val timeout: Timeout = 10.millis | ||
| val workerLatch = new CountDownLatch(workerCnt) |
There was a problem hiding this comment.
this is bad practice for unit tests, multiple tests in DeadLetterSpec may use this same immutable value.
|
@pjfanning I have signed my ICLA and sent it to [email protected], but I didn't receive any reply from it. Furthermore, those flaky test has been fixed, and need to trigger the CI run. |
Thanks @Roiocam - it may take a few days for ASF Secretary to respond, especially since it is the weekend. |
Got it, Thanks. Plz disregard duplicate review requests. |
|
@Roiocam HI, happy holidays, I just saw those Pull requests and issues are closed, is there any reason behind those？ I’m off this network because I back to my hometown, sorry for not reviewing it. |
|
@He-Pin I am sorry for that closed this PR for no reason, this PR is not only my contribution. |
|
@mdedetrich what do you think? In the end, this is probably what the code should have always done. Seems safe enough for a v1.1.x change to me. |
|
@Roiocam Thank you. |
Motivation
Attach the necessary information before sending the message to DeadLetter to correctly display the destination in the DeadLetter.
References: #668
How does this PR fix work?
In classic Actors, when an Actor dies, its Mailbox is "swap" to a
deadLetterMailboxMailbox.https://github.com/apache/incubator-pekko/blob/a3f48833c97501a0193f4135f99c5f6ea04a03dd/actor/src/main/scala/org/apache/pekko/dispatch/AbstractDispatcher.scala#L239-L245
When an Actor sends a message to another ActorRef, it essentially enqueues the message to that Actor's mailbox.
When an Actor sends a message to a dead Actor, it means that the message would enqueue to the "deadLetterMailbox",the dead mailbox will transform the message into a DeadLetter event and then publish it to the EventBus. This is how DeadLetters work in classic Actors.
In Typed actor, it basiclly same, but the
sendervariable cannot be passed implicitly.In the Ask pattern, there are some differences. When an Actor uses ask to request another Actor, it essentially creates a temporary PromiseActorRef to receive the response from the target Actor.
When the PromiseActorRef receives a
tellcall, it checks if the current Future has already completed and, if so, emits a DeadLetter event. This is done to ensure consistent behavior with the tell method (i.e., sending DeadLetter events to the EventBus via DeadLetterActorRef).However, the PromiseActorRef directly sends the message without transforming it into a DeadLetter event, resulting in distorted logs.
This PR converts the message into a DeadLetter event before sending it from PromiseActorRef to DeadLetterActorRef.