-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fixed: IWrappedMessage + IDeadLetterSuppression handling
#7414
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
Conversation
IWrappedMessage + IDeadLetterSuppression handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detailed my changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reproduction specs for SuppressedDeadLetter production in the case of IWrappedMessages that may themselves be marked as IDeadLetterSuppression or perhaps some of their contents might be marked that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for the WrappedMessage class itself
| return message; | ||
| } | ||
|
|
||
| internal static bool IsDeadLetterSuppressedAnywhere(object message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function to check, up and down the stack, if a message was marked as IDeadLetterSuppression anywhere within
| if (message == null) throw new InvalidMessageException("Message is null"); | ||
| var i = message as Identify; | ||
| if (i != null) | ||
| switch (message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refomatting - nothing of significance here
| public void Enqueue(IActorRef receiver, Envelope envelope) | ||
| { | ||
| if (envelope.Message is DeadLetter) | ||
| if (envelope.Message is AllDeadLetters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important fix - now that we can produce SupressedDeadLetters more easily, this safety check has to be expanded to encompass AllDeadLetters and their derived types, otherwise this will produce a StackOverflowException.
| /// <exception cref="ArgumentNullException"> | ||
| /// This exception is thrown when either the sender or the recipient is undefined. | ||
| /// </exception> | ||
| public SuppressedDeadLetter(IDeadLetterSuppression message, IActorRef sender, IActorRef recipient) : base(message, sender, recipient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking API change here, kind of - needed to relax this type constraint otherwise we'd lose data in the SuppressedDeadLetter logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
A Phobos customer noticed many
DeadLettermessages appearing only when Phobos was enabled, but not when it was disabled - as it turns out, this is because Phobos' payload for propagating trace information transparently through theActorSystemis anIWrappedMessageand the dead letter logging infrastructure in Akka.NET doesn't account for that accurately.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):