Skip to content

Conversation

rogeralsing
Copy link
Contributor

Fix for #1646

In the RemoteDeadletterActorRef there is some missing code to propagate deadletters to the base implementation:

.With<DeadLetter>(
    deadLetter =>
    {
        // else ignore: it is a reliably delivered message that might be retried later, and it has not yet deserved
        // the dead letter status
        var deadSend = deadLetter.Message as EndpointManager.Send;
        if (deadSend != null && deadSend.Seq == null)
        {
            base.TellInternal(deadSend.Message, deadSend.SenderOption ?? ActorRefs.NoSender);
        }

        //##############
       // the issue here was that the With<DeadLetter> consumed _all_ dead letters, 
       // when this clause was just intended to consume those containing EndpointManager.Send
    })

replace patternmatch
base.TellInternal(send.Message, send.SenderOption ?? ActorRefs.NoSender);
}
}
else if (deadLetter?.Message is EndpointManager.Send)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced the pattern matching code with normal if blocks, this specific check only consumes deadletters containing EndpointManager.Send messages now.
(unlike before where it incorrectly consumed all deadletters)

@rogeralsing
Copy link
Contributor Author

See Scala code for ref implementation

override def !(message: Any)(implicit sender: ActorRef): Unit = message match {
      case Send(m, senderOption, _, seqOpt) 
        // else ignore: it is a reliably delivered message that might be retried later, and it has not yet deserved
        // the dead letter status
        if (seqOpt.isEmpty) super.!(m)(senderOption.orNull)
      case DeadLetter(Send(m, senderOption, recipient, seqOpt), _, _) 
        // else ignore: it is a reliably delivered message that might be retried later, and it has not yet deserved
        // the dead letter status
        if (seqOpt.isEmpty) super.!(m)(senderOption.orNull)
      case _  super.!(message)(sender)
    }

@Aaronontheweb
Copy link
Member

I've reviewed this and yep, the bug is pretty clearly a greedy match statement and @rogeralsing's fix matches the JVM source. Merging it in prior to 1.0.6.

@Aaronontheweb Aaronontheweb reopened this Jan 18, 2016
Aaronontheweb added a commit that referenced this pull request Jan 18, 2016
@Aaronontheweb Aaronontheweb merged commit 2a178f8 into akkadotnet:dev Jan 18, 2016
@Aaronontheweb
Copy link
Member

Crap, hit the wrong button the first time

@rogeralsing rogeralsing deleted the remote-watch-fix1646 branch January 18, 2016 19:06
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.

3 participants