DefaultRetryStep failure handling moved to before processing message …

[hjalle]

In reference to #1212, this is what I believe might be a solution, to avoid destroying the transactionscope that may be enlisted to.

---

Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

[CLAassistant]

All committers have signed the CLA.

[hjalle]

I see tests regarding 2nd level is failing, Ill take a look at that

[hjalle]

@mookid8000 any possibilities for something like this?

[hjalle]

A last ping can never hurt.. @mookid8000 , is this considered dead or how should we treat it?

[mookid8000]

hi @hjalle , sorry for the long silence here 😅

I have been pondering over this particular thing in the back of my mind since you first wrote, and I still really haven't reached a conclusion. Maybe we can talk about it here and somehow approach something together?

The thing is this: All versions of Rebus < 8 had the error tracker check BEFORE dispatching the message, which was originally so because of MSMQ message queue transactions. From Rebus 8, all transports have worked the same way:

1. Try to handle message – buffer all outgoing messages in memory

2. If successful: Send outgoing messages and ACK incoming message
   If it fails: 
       If it hasn't failed too many times: NACK incoming message
       It it has failed too many times: Move message to error queue and ACK incoming message

BUT as you have discovered, transaction scopes and the SQL transport cause this model to fail, because it effectively emulates the old behavior of MSMQ where all the queue interactions are enlisted in the same transaction.

Honestly, I am more inclined to remove the transaction scope support, because it messes with the try-to-handle-message-ACK-or-NACK-model.

What are your thoughts?

[hjalle]

Yes, I do see the point, but its kind of frustrating right now. We, and others I presume, have not yet fully migrated away from "DTC"-thinking.

Our current workaround is essentially that we decorate the SQL-transport and have a in-memory delivery count, which will then "fail early" in the DefaultRetryStep, as that logic resides at the top.. I'm not sure if that has any drawbacks but at least it solves the issue with endless retry-loops. I don't really see it as a viable solution right now since it relies on the internals of the DefaultRetryStep that one as well, although it of course makes a lot more sense to have that logic at the beginning of the retry rather than at the end of the "last" delivery. Its also not a generic solution as v8 with enlistInAmbient, sql and transactionscopes will result in endless retries for all existing users. If the SQL-transport would add delivery counting, it would probably solve this issue, I guess..?

[GMacAussie]

First off, we are still on Rebus 6 because we have our own outbox/transaction scope handling and didn't want entanglement, so therefore this comment will be most likely completely offbase...

We have had in the past the need to separate out (isolate) transaction scopes (for various reasons). We do that using:

        /// <summary>
        /// We usually use Read Committed isolation and default timeout
        /// </summary>
        static readonly TransactionOptions ReadCommitted = new TransactionOptions {
            Timeout = TransactionManager.DefaultTimeout,
            IsolationLevel = IsolationLevel.ReadCommitted
        };

        /// <summary>
        /// Create a suppress read committed transaction scope. By default the isolation level is Serializable
        /// which is not the database uses, which is Read Committed.
        /// 
        /// A suppress transaction scope is used to separated transaction scope from an existing transaction
        /// scope. It will not be an ambient transaction scope.
        /// 
        /// Make sure TransactionScopeAsyncFlowOption is enabled to allow the transaction scope to cross async/await.
        /// </summary>
        /// <returns>The suppress read comminited transaction scope</returns>
        public static TransactionScope CreateSuppressReadCommittedScope() {
            return new TransactionScope(TransactionScopeOption.Suppress, ReadCommitted, TransactionScopeAsyncFlowOption.Enabled);
        }

Just throwing this out there...