New retrystrategy seems incompatible with Azure Service Bus Native deadlettering

[hjalle]

Hi,

The old "SimpleRetryStrategy" never created a new RebusTransactionScope before dispatching the message to the errorHandler:

async Task MoveMessageToErrorQueue(IncomingStepContext context, ITransactionContext transactionContext, Exception exception)
    {
        var transportMessage = context.Load<OriginalTransportMessage>().TransportMessage.Clone();

        await _errorHandler.HandlePoisonMessage(transportMessage, transactionContext, exception);
    }

The new DefaultRetryStrategy does:

    async Task PassToErrorHandler(StepContext context, ExceptionInfo exception)
    {
        var originalTransportMessage = context.Load<OriginalTransportMessage>() ?? throw new RebusApplicationException("Could not find the original transport message in the current incoming step context");
        var transportMessage = originalTransportMessage.TransportMessage.Clone();

        using var scope = new RebusTransactionScope();
        await _errorHandler.HandlePoisonMessage(transportMessage, scope.TransactionContext, exception);
        await scope.CompleteAsync();
    }

In Rebus.AzureServiceBus the transactionContext.Items are read, but since its a new transactioncontext with the new retrystep, nothing is found:

public async Task HandlePoisonMessage(TransportMessage transportMessage, ITransactionContext transactionContext, ExceptionInfo exception)
        {
            if (transactionContext.Items.TryGetValue("asb-message", out var messageObject)
                && messageObject is ServiceBusReceivedMessage message
                && transactionContext.Items.TryGetValue("asb-message-receiver", out var messageReceiverObject)
                && messageReceiverObject is ServiceBusReceiver messageReceiver)
            {
....

which in turn just "elses out" and uses the default error handler.

I'm not really sure what the preferred way to solve this would be, thus making an issue instead of a PR.

[hjalle]

Any progress/ideas regarding this one?

[AndreaCuneo]

@mookid8000 I hit the same issue when using ASB NativeDeadlettering and new error handling.

As far as I can see, the issue stem from the decision of the RetryStep to create a 'nested' TransactionContext to handle the error. This makes sense if the ErrorHandling wants to Send a new Message to a separate Queue and thus Complete() that sub-transaction.

My suggestion is to move the nested transaction handling into the IErrorHandling responsabilities, depending on how ErrorHandling wants to, well, handle it. The DeadletterQueueErrorHandler would create its own Transaction, ASBNativeDeadletter would not.

The change is 'subtle' as IErrorHandling interface would not change, but parameters would change meanings:

1. the IErrorHandler.HandlePoisonMessage(TransportMessage transportMessage, ITransactionContext transactionContext, ExceptionInfo exception); interface would not change, but the RetryStep would pass the main Transaction.
2. DeadletterQueueErrorHandler would be changed to own using var scope = new RebusTransactionScope();

What still troubles me is that the ASBNativeDeadlettering does "complete" the original transaction implicitly as the OriginalMessage is lost. On top of the 2 points above, I'd suggest also to change the ASB NativeDeadlettering to register an handler on the TransactionContext to be executed when the RetryStep complete the Transaction, not immediatly.

If the approach is correct, I can prepare PR.

[hjalle]

Have you solved this in another way or are you also waiting for a solution @AndreaCuneo? I think your proposed solution might work. I'd like to see it solved somehow at least

[AndreaCuneo]

@hjalle Waiting for @mookid8000 to confirm before making a PR.

[hjalle]

Its a bit of a blocker for me upgrading to rebus 8 as I'm relying on dead lettering. Is it possible for @mookid8000 to perhaps take a glance at the suggestion or perhaps come up with another idea? I could give it a go but I guess it would be nice to hear if theres work in progress or if you have any ideas.

[mookid8000]

Hi @AndreaCuneo , sorry for being so slow to get back to this issue! 😓 I think your suggestion of letting DeadletterQueueErrorHandler create its own RebusTransactionScope for its own internal use is good! 👍 If you have it ready, please do send a PR and I'll review it quickly.

[AndreaCuneo]

Hi @mookid8000 no worries, thanks for the library, as always.

I can prepare the PR this weekend, quite busy myself too :)

[mookid8000]

.........and by "quickly" I mean now 😅 it's out as Rebus 8.3.0 on NuGet org now!

Thanks for fixing it!