Proposed changes to ProcessingBehaviours, Projects and Repos

[srpeirce]

Hi All,

Upon investigating the memory leak issue #37, I have some proposed changes to Pat.

I propose DefaultMessageProcessingBehaviour is no longer responsible for error handling, instead splitting this functionality into multiple separate behaviours to allow developer more control as to what happens when a subscriber fails to process a message.

RepublishFailedMessageWithDelayProcessingBehaviour
This will allow similar behaviour to existing peek lock timeout, except it will explicitly set the ScheduledEnqueueTime. This will need to manually increment the delivery count.

This will resolve the Memory Leak issue that we have at the moment where we do not complete a message (#37).

AbandonFailedMessageProcessingBehaviour
This is an alternative to RepublishFailedMessageWithDelayProcessingBehaviour whereby MessageReceiver.AbandonAsync will be called, this leads to message being put back onto the subscription and the delivery count will be incremented.
The message will be put back on the subscription and will be available to process by a subscriber without any delay.

https://docs.microsoft.com/en-us/dotnet/api/microsoft.azure.servicebus.core.messagereceiver.abandonasync?view=azure-dotnet

DeadletterFailedSerializationMessageProcessingBehaviour
This will be existing functionality extracted from DefaultMessageProcessingBehaviour whereby the message will be put on the DLQ when serialization fails.

---

For the RepublishFailedMessageWithDelayProcessingBehaviour we will need to send a message to ASB from Pat.Subscriber project, this is not something currently supported.

I propose that we achieve this by referencing Pat.Sender.

Rather than referencing via Nuget, I propose that we pull the Pat.Sender projects into this same repository and reference directly. This would mean Pat.Sender repository to be discontinued and Pat.Subscriber repository to be renamed.

Pat.Sender would still be a separate Nuget package for users who just need to send messages.

Subscribers would no longer need to explicitly reference Pat.Sender when they also send messages, meaning a simplified dev story.
This could lead to simplified configuration/logging story in the future.

With Pat.Subscribers project being able to send messages, potentially more advanced use cases would become available in the future…e.g. Sagas.

In this scenario I suggest that Pat.Subscriber and Pat.Sender version numbers would stay in sync.

---

Thoughts?

-Steve

[DaveAurionix]

@srpeirce I think the longer-term simpler dev story is a strong argument for merging these two. Also the integration tests can be refactored perhaps?

It might be a simple dev story if all three behaviours are plugged in, always. e.g. whenever deserialization fails, the message DLQs. Whenever processing fails, it is abandoned X times and then finally scheduled for later. If it is scheduled Y times, failing each time, then it eventually DLQs. Config is a case simply of overriding default numeric values of X and Y? I must admit, we seem to have arrived at NServiceBus's first and second level retries concept. It would be beneficial if we could dream up something simpler and keep Pat easier to understand and faster to get going with in a new project.

P.S. We might not want to DLQ immediately on deserialization faults. We might be in the middle of rolling out updated microservice instances across a cluster and we wouldn't want deadletters every time we upgrade. If a newer version publishes a message to itself but an older version picks up the message and fails deserialisation, we probably want the message retried in 60 seconds' time as it is likely that more instances have rolled-forward to the new version and can successfully handle the new message schema on retry.

I would therefore argue that there is nothing special about a deserialization fault and to gain simplicity we should treat it like any other exception during processing: i.e. consider retrying the message and allow it to follow the standard configured retry cycle.