Skip to content

Comments

Dead letter when remote call target is dead#989

Merged
yim-lee merged 4 commits intoapple:mainfrom
yim-lee:iss943
Jul 7, 2022
Merged

Dead letter when remote call target is dead#989
yim-lee merged 4 commits intoapple:mainfrom
yim-lee:iss943

Conversation

@yim-lee
Copy link
Member

@yim-lee yim-lee commented Jul 7, 2022

Resolves #943


return self.namingLock.withLock {
return try self.namingLock.withLock {
guard let managed = self._managedDistributedActors.get(identifiedBy: id) else {
Copy link
Member Author

@yim-lee yim-lee Jul 7, 2022

Choose a reason for hiding this comment

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

IIUC, only local resolve would reach here? In other words, it's not part of remote call code path?

do {
guard let actor = self.resolve(id: recipient) else {
self.log.error("Unable to resolve recipient \(recipient). Message will be dropped: \(invocation)")
self.deadLetters.tell(DeadLetter(invocation, recipient: recipient))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should the dead letter happen at sender, after getting back DeadLetterError? Or both sender and recipient? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

On the recipient I believe, since the recipient will has no function that will throw, so this dead letter informs that system that "someone is calling things on you, but they can't get through to the recipient actor".

And the caller has the function that they called that resulted in a dead letter, that function should throw as it received s the error reply from the dead-letter-ed remoteCall. This way the caller knows it happened, we don't need to log it there.


public struct DeadLetterError: DistributedActorSystemError, Codable {
public let recipient: ClusterSystem.ActorID
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

try await watcher.watch(greeter!)

greeter = nil
try p.expectMessage(prefix: "Received terminated: /user/Greeter")
Copy link
Member

Choose a reason for hiding this comment

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

Oh we're missing API to p.watch a DA from a TestProbe it seems? If you want that'd be a great thing to implement...

We'd want to be able to p.watch(watcher); p.expectTerminated()

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Created #990

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Tests and impl look good! Just a minor nitpick about not logging in one place.

LGTM, please merge at will 👍

yim-lee and others added 3 commits July 7, 2022 13:09
Co-authored-by: Konrad `ktoso` Malawski <konrad.malawski@project13.pl>
Co-authored-by: Konrad `ktoso` Malawski <konrad.malawski@project13.pl>
@yim-lee yim-lee merged commit 364e3dd into apple:main Jul 7, 2022
@yim-lee yim-lee deleted the iss943 branch July 7, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dead letter when a message arrives from remote call and target is not alive anymore

2 participants