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

Fixed: IWrappedMessage + IDeadLetterSuppression handling #7414

Merged
merged 10 commits into from
Dec 20, 2024

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Dec 18, 2024

Changes

A Phobos customer noticed many DeadLetter messages 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 the ActorSystem is an IWrappedMessage and 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):

@Aaronontheweb Aaronontheweb changed the title WIP dead letter suppression testing Fixed: IWrappedMessage + IDeadLetterSuppression handling Dec 18, 2024
@Aaronontheweb Aaronontheweb added this to the 1.5.33 milestone Dec 18, 2024
Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Detailed my changes

Copy link
Member Author

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.

Copy link
Member Author

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

@@ -197,6 +197,18 @@ public static object Unwrap(object message)
}
return message;
}

internal static bool IsDeadLetterSuppressedAnywhere(object message)
Copy link
Member Author

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)
Copy link
Member Author

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

@@ -46,9 +46,13 @@ public DeadLetterMessageQueue(IActorRef deadLetters)
/// <param name="envelope">TBD</param>
public void Enqueue(IActorRef receiver, Envelope envelope)
{
if (envelope.Message is DeadLetter)
if (envelope.Message is AllDeadLetters)
Copy link
Member Author

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.

@@ -104,7 +104,7 @@ public sealed class SuppressedDeadLetter : AllDeadLetters
/// <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)
Copy link
Member Author

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

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM

@Arkatufus Arkatufus enabled auto-merge (squash) December 18, 2024 19:42
@Aaronontheweb Aaronontheweb enabled auto-merge (squash) December 18, 2024 23:07
@Aaronontheweb Aaronontheweb merged commit 0cf567a into akkadotnet:dev Dec 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants