Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: proper path when promise actor terminated quickly #1156

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Feb 27, 2024

solve #1157
self-explanatory

@laglangyue
Copy link
Contributor

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.

@pjfanning pjfanning added the bug Something isn't working label Feb 27, 2024
@He-Pin
Copy link
Member

He-Pin commented Feb 27, 2024

Better with a bug report here too.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me!

senderRef ! "complete"
p.ref ! senderRef
}
}), "pathPrefix")
Copy link
Member

@He-Pin He-Pin Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this test, I think we would better test it several times with some generated pathPrefixs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random str is ok?

Copy link
Member Author

@Roiocam Roiocam Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@Roiocam Roiocam Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me, it's enough for the bugfix.

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but it would be better with some generated data other than "pathPrefix" in test

@He-Pin
Copy link
Member

He-Pin commented Feb 28, 2024

Once this gets merged, should backport to 1.0.x too.

@mdedetrich
Copy link
Contributor

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?

@raboof
Copy link
Member

raboof commented Feb 28, 2024

I don't see a strong reason to backport in this case

@raboof raboof merged commit 2fdf7c9 into apache:main Feb 28, 2024
18 checks passed
@Roiocam Roiocam deleted the proper-path-when-promise-actor-stop branch February 28, 2024 08:24
@He-Pin He-Pin added this to the 1.1.0-M1 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants