Azure-sdk-for-net: [Discussion] Service Bus - Track 2 - Processor

[JoshLove-msft]

_The intent of this issue is to solicit feedback from the community in advance of the GA of the next major version of the Service Bus library (which will follow several Preview releases).
As a reference, the .NET Azure SDK guidelines can be found here: https://azure.github.io/azure-sdk/dotnet_introduction.html
Terminology:
Track 0 refers to the WindowsAzure.ServiceBus library
Track 1 refers to the Microsoft.Azure.ServiceBus library
Track 2 refers to the new library, Azure.Messaging.ServiceBus, we are working on releasing._

Background
In the previous versions of the Service Bus library there were two models for receiving messages, which we will refer to here as "Push" and "Pull". In Track 1, pull was used with the Receive method in the MessageReceiver. Push could be used on the QueueClient/SubscriptionClient/MessageReceiver by calling the RegisterMessageHandler/RegisterSessionHandler methods and passing in callbacks.
The push methods provided a few features to make interacting with messages easier:

• automatically renew message/session locks
• option to automatically complete a message after the callback finishes executing.
• option to specify how many concurrent calls to the user callback to use

Processor
In the new version of the Service Bus library, we are offering support for both the push and pull modes of receiving messages. However, instead of intermixing these modes on the same type, we offer a separate dedicated type for the push model, called the ServiceBusProcessor. The same set of features for push-based processing that was available within the Track 1 library will be available in Track 2.
The pull operations are available on the ServiceBusReceiver.
```c#
var client = new ServiceBusClient("connectionString");
ServiceBusReceiver receiver = client.GetReceiver("myQueue");
// I can use the receiver to directly receive messages. I will need to
// handle completing the message, and renewing message locks myself.
IList messages = await receiver.ReceiveBatchAsync(maximumMessageCount: 10);

// available processing options set to their defaults
var options = new ServiceBusProcessorOptions
{
MaxConcurrentCalls = 1,
AutoComplete = true,
MaxAutoLockRenewalDuration = TimeSpan.FromMinutes(5),
ReceiveMode = ReceiveMode.PeekLock,
PrefetchCount = 0
};
ServiceBusProcessor processor = client.GetProcessor("myQueue", options);

The model of passing in callbacks to a method, has been amended to setting event handlers on the processor.
```c#
processor.ProcessMessageAsync += MessageHandler;
processor.ProcessErrorAsync += ExceptionHandler;

async Task MessageHandler(ProcessMessageEventArgs args)
{
   // if the handler returns without causing an exception to be thrown, the processor will 
   // automatically complete the received message

  // the message is available within the event args
  ServiceBusReceivedMessage message = args.Message;
  // the settlement methods are also available within the event args
  // for instance, if I wanted to deadletter this message for some reason, I would do so like this
  await args.DeadLetterAsync(message);
}

// the exception handler would be triggered if an exception is thrown at any point while receiving 
// and processing a message. This includes if the user message handler throws an exception.
Task ExceptionHandler(ProcessErrorEventArgs eventArgs)
{
  // the error source indicates at which point in the processing the error occurred
  // e.g. Receive/UserCallback/LockRenewal
    string errorSource = eventArgs.ErrorSource;
    Exception ex = eventArgs.Exception;
    string namespace = eventArgs.FullyQualifiedNamespace;
    string entityPath = eventArgs.EntityPath;

    // I can do some application logging with this information
    return Task.CompletedTask;
}

// now that we have registered our event handlers, we can actually start processing
await processor.StartProcessingAsync();

// to stop processing
await processor.StopProcessingAsync();

Session Processor
The configuration used for setting up processing for a session entity is very similar to that shown above for non-session entities.
```c#
// available processing options set to their defaults
var options = new ServiceBusProcessorOptions
{
// the default used for MaxConcurrentCalls is different when getting a session processor
MaxConcurrentCalls = 8,
AutoComplete = true,
// this refers to the session lock when used with a session processor, as opposed to a
// message lock, as in the case for the regular processor.
MaxAutoLockRenewalDuration = TimeSpan.FromMinutes(5),
ReceiveMode = ReceiveMode.PeekLock,
PrefetchCount = 0
};
// we get back a ServiceBusSessionProcessor as opposed to a ServiceBusProcessor
ServiceBusSessionProcessor processor = client.GetSessionProcessor("myQueue", options);
processor.ProcessMessageAsync += MessageHandler;
processor.ProcessErrorAsync += ExceptionHandler;

// note that the args type used for the message handler is different than
// it is for the regular processor
async Task MessageHandler(ProcessSessionMessageEventArgs args)
{
// we can perform session operations ...
var sessionState = Encoding.UTF8.GetBytes("my session state");
await args.SetSessionStateAsync(sessionState);
// in addition to the message settlement operations
await args.DeadLetterAsync(message);
}

// now that we have registered our event handlers, we can actually start processing
await processor.StartProcessingAsync();

// to stop processing
await processor.StopProcessingAsync();
```
One other difference under the hood is that when using concurrency (MaxConcurrentCalls > 1) with a session processor, we will have separate receiver links for each thread that is processing messages. Doing this allows us to process multiple sessions at the same time as each receiver link would be scoped to a different session. For the regular processor case, we are following the Track 1 pattern of sharing a single receiver link among the multiple threads.

[JoshLove-msft]

Adding some community members who have recently submitted issues/PRs for Service Bus 😃

/cc @nemakam @SeanFeldman @Einarsson @brandondahler @broloaugh @ankrause @GrimGadget @ShaneCourtrille @Pucmaster @Bnjmn83

[SeanFeldman]

@JoshLove-msft dual model was available in both track 0 and 1. Not just 1.

[SeanFeldman]

@JoshLove-msft what's other than Mesaage will be available via ProcessMessageEventArgs?

[JoshLove-msft]

@JoshLove-msft what's other than Mesaage will be available via ProcessMessageEventArgs?

ProcessMessageEventArgs will have the settlement methods that can be used on the message - Complete/abandon/defer/deadletter. The Processor itself doesn't expose the settlement methods as the settlement needs to happen on the link that the message was received on, which in the case of sessions at least, will be different for each thread. It also has the CancellationToken for the Processor. This cancellation token gets cancelled when the user calls StopProcessingAsync.

[SeanFeldman]

// the settlement methods are also available within the event args
// for instance, if I wanted to deadletter this message for some reason, I would do so like this
await args.DeadLetterAsync(message);

The settlement is happening via receiver. Period. Do not come up with an unnecessary magic. Expose the receiver via args.

[JoshLove-msft]

// the settlement methods are also available within the event args
// for instance, if I wanted to deadletter this message for some reason, I would do so like this
await args.DeadLetterAsync(message);

The settlement is happening via receiver. Period. Do not come up with an unnecessary magic. Expose the receiver via args.

The concern with doing this is that it would expose a bunch of other methods that wouldn't make sense for the user, such as Receive/Peek/etc. One concern was that would a user think they need to call args.Receiver.ReceiveAsync?

[SeanFeldman]

ProcessErrorEventArgs exposes Exception (👍) and ErrorSource. What's bothering latter provides? Also, I'd like to know the entity/fqdn that the error occurred on.

[JoshLove-msft]

ProcessErrorEventArgs exposes Exception (👍) and ErrorSource. What's bothering latter provides? Also, I'd like to know the entity/fqdn that the error occurred on.

Yep, it also has FullyQualifiedNamespace/EntityPath. The ErrorSource indicates where in the processing the error occurred. I will update the sample.

[SeanFeldman]

The concern with doing this is that it would expose a bunch of other methods that wouldn't make sense for the user, such as Receive/Peek/etc. One concern was that would a user think they need to call args.Receiver.ReceiveAsync?

The receiver part on the args doesn't have to be a full blown Receiver. interface-segregation principle (ISP) existed long ago Azure SDK principles where engraved.

[SeanFeldman]

Yep, it also has FullyQualifiedNamespace/EntityPath. The ErrorSource indicates where in the processing the error occurred. I will update the sample.

In such case, for the future reference, I would suggest to spell it out to prevent my unnecessary clarifications. It will also will help others understand better what's proposed. While everyone is working remotely, telepathy is not going to be a common nothing any time soon.

[JoshLove-msft]

The concern with doing this is that it would expose a bunch of other methods that wouldn't make sense for the user, such as Receive/Peek/etc. One concern was that would a user think they need to call args.Receiver.ReceiveAsync?

The receiver part on the args doesn't have to be a full blown Receiver. interface-segregation principle (ISP) existed long ago Azure SDK principles where engraved.

This is essentially what we are doing, but just not exposing a Receiver property. What is the benefit of exposing the Receiver here?

[SeanFeldman]

This is essentially what we are doing, but just not exposing a Receiver property. What is the benefit of exposing the Receiver here?

Args is your your entry point. It's solely there to provide access to the things the handler/callback can use. On its own it cannot settle anything. If you want to have developers following the service principles, give them the model they'll know - receiver to perform settlements. Otherwise, they might as well use a global receiver to accomplish the task.

[JoshLove-msft]

This is essentially what we are doing, but just not exposing a Receiver property. What is the benefit of exposing the Receiver here?

Args is your your entry point. It's solely there to provide access to the things the handler/callback can use. On its own it cannot settle anything. If you want to have developers following the service principles, give them the model they'll know - receiver to perform settlements. Otherwise, they might as well use a global receiver to accomplish the task.

Well, it's possible that developers would just use the Processor, in which case they wouldn't really need to know about the Receiver. But I think you raise a fair point.

[SeanFeldman]

On the assumption

Well, it's possible that developers would just use the Processor, in which case they wouldn't really need to know about the Receiver. But I think you raise a fair point.

Developers will always start with a receiver and not a processor because of the simplicity. A processor is a higher level concept that comes later in the game, with some knowledge about the service acquired. And at that point the logical connection between the receiver and settlement operations has already been established. C'est la vie.

[JoshLove-msft]

On the assumption

Well, it's possible that developers would just use the Processor, in which case they wouldn't really need to know about the Receiver. But I think you raise a fair point.

Developers will always start with a receiver and not a processor because of the simplicity. A processor is a higher level concept that comes later in the game, with some knowledge about the service acquired. And at that point the logical connection between the receiver and settlement operations has already been established. C'est la vie.

What are your thoughts on scoping down the available methods on such a hypothetical receiver? The downside to doing this is we have to introduce yet another type, whereas the benefit would be hiding what in all likelihood would be superfluous methods like Receive/Peek. However, I'm thinking if we were to go this route of exposing the Receiver, it might be simpler to just leave it as the normal receiver type.

[SeanFeldman]

On the assumption

Well, it's possible that developers would just use the Processor, in which case they wouldn't really need to know about the Receiver. But I think you raise a fair point.

Developers will always start with a receiver and not a processor because of the simplicity. A processor is a higher level concept that comes later in the game, with some knowledge about the service acquired. And at that point the logical connection between the receiver and settlement operations has already been established. C'est la vie.

What are your thoughts on scoping down the available methods on such a hypothetical receiver? The downside to doing this is we have to introduce yet another type, whereas the benefit would be hiding what in all likelihood would be superfluous methods like Receive/Peek. However, I'm thinking if we were to go this route of exposing the Receiver, it might be simpler to just leave it as the normal receiver type.

I do not share the same perspective as the Azure SDK team on the types. If those are required, then introduce them. It doesn't mean sprinkling on left and right, but if they help, the consideration should be their usability rather than the number of new types introduced. It's not a KPI that should be blindly followed.

I'll sum up my thoughts.

Overall, happy with the proposal. Especially about the CancellationToken. The settlement operations should not be on the ProcessMessageEventArgs but rather a receive part of the argument aggregate. Wherever it is the full client or its subset, doesn't matter.

[JoshLove-msft]

On the assumption

Well, it's possible that developers would just use the Processor, in which case they wouldn't really need to know about the Receiver. But I think you raise a fair point.

Developers will always start with a receiver and not a processor because of the simplicity. A processor is a higher level concept that comes later in the game, with some knowledge about the service acquired. And at that point the logical connection between the receiver and settlement operations has already been established. C'est la vie.

What are your thoughts on scoping down the available methods on such a hypothetical receiver? The downside to doing this is we have to introduce yet another type, whereas the benefit would be hiding what in all likelihood would be superfluous methods like Receive/Peek. However, I'm thinking if we were to go this route of exposing the Receiver, it might be simpler to just leave it as the normal receiver type.

I do not share the same perspective as the Azure SDK team on the types. If those are required, then introduce them. It doesn't mean sprinkling on left and right, but if they help, the consideration should be their usability rather than the number of new types introduced. It's not a KPI that should be blindly followed.

I'll sum up my thoughts.

Overall, happy with the proposal. Especially about the CancellationToken. The settlement operations should not be on the ProcessMessageEventArgs but rather a receive part of the argument aggregate. Wherever it is the full client or its subset, doesn't matter.

As always, thanks for the great feedback. We will take this under consideration.

[JoshLove-msft]

I'd like to propose having the AutoComplete default be false rather than true. This would diverge from the behavior in Track 1 where it is defaulted to True.

• AutoComplete will cause messages to be deleted from the service. This seems like something the user should opt into, rather than just doing it without their explicit consent.
• The general .NET guideline of having the default value for a property match the default of its type indicates that the default for bool properties should generally be False.
• It is the push model itself that is the major value proposition of using the processor, rather than the ability to autocomplete a message, so it shouldn't be necessary to nudge users to use AutoComplete by defaulting to True. A user can write a piece of code for processing a message without regards to how to retrieve the message or how to ensure the message is locked while they are processing it. The actual settlement of the message is trivial to add in the event handler.

[SeanFeldman]

Given that Trank 2 is a completely new implementation, it can have its default behaviour.
Saying that I'd like to highlight a flaw in the logic outlined above.

AutoComplete will cause messages to be deleted from the service. This seems like something the user should opt into, rather than just doing it without their explicit consent.

This is simply not true. The auto-completion is only happening when the callback is completed successfully and no exception is thrown. That is a contract. This is also a convenience for customers that know they don't have to do anything if no exception is thrown within the callback. Take Azure Functions for example. Similar model. A message is completed only if a Function has completed w/o exception thrown.

The actual settlement of the message is trivial to add in the event handler.

Correct, it's simple to add and adds another line of code _when needed_.
Do you have any evidence this convention was causing problems?
A convention that is working well is a convention that should not be changed.

[JoshLove-msft]

Given that Trank 2 is a completely new implementation, it can have its default behaviour.
Saying that I'd like to highlight a flaw in the logic outlined above.

AutoComplete will cause messages to be deleted from the service. This seems like something the user should opt into, rather than just doing it without their explicit consent.

This is simply not true. The auto-completion is only happening when the callback is completed successfully and no exception is thrown. That is a contract. This is also a convenience for customers that know they don't have to do anything if no exception is thrown within the callback. Take Azure Functions for example. Similar model. A message is completed only if a Function has completed w/o exception thrown.

The actual settlement of the message is trivial to add in the event handler.

Correct, it's simple to add and adds another line of code _when needed_.
Do you have any evidence this convention was causing problems?
A convention that is working well is a convention that should not be changed.

The fact that an exception will prevent the message from being completed doesn't really change the argument. We are still, in general, completing the message behind the scenes when a user hasn't explicitly opted in to this behavior.

Another concern I have with AutoComplete is that it also means users basically shouldn't do any message settlement at all. For instance, if you defer/deadletter/abandon a message in your handler, we would still attempt to autocomplete it and throw an exception. I think this behavior is okay, but it being the default behavior makes me uneasy.

I don't have concrete evidence that this is causing problems, but I don't think we are collecting any data on how often the default value is being changed.
With that being said, I'm open to the possibility that I am just plain wrong on this and we are better off leaving as is.

[SeanFeldman]

The fact that an exception will prevent the message from being completed doesn't really change the argument. We are still, in general, completing the message behind the scenes when a user hasn't explicitly opted in to this behavior.

I don't follow your logic. The exception doesn't explode and bubbles up. The SDK controls that and invokes user-provided error callback. If there's no exception, the message is automatically completed. I don't understand what does it mean "We are still, in general, completing the message behind the scenes when a user hasn't explicitly opted in to this behavior".

I don't have concrete evidence that this is causing problems, but I don't think we are collecting any data on how often the default value is being changed.
With that being said, I'm open to the possibility that I am just plain wrong on this and we are better off leaving as is.

I am not saying that you're wrong. We all entitled to our opinions. What I'm saying is that an abstraction that worked well for years (Track 0 and 1) should not be changed just because it "feels wrong" w/o some strong evidence. Is there an issue raised in the Track 1 repo to change this behaviour due to its destructive nature? Was there an issue raised with Track 0? Is there an issue with this mondo repo? How many support cases Azure support had that would indicate this abstraction has caused issues? If you can prove that it was at a significant percentage of customers that complained about, I'll agree with the proposed behaviour change. But until then, I'll stay with "if it's not broken, don't fix it".

[JoshLove-msft]

I don't follow your logic. The exception doesn't explode and bubbles up. The SDK controls that and invokes user-provided error callback. If there's no exception, the message is automatically completed. I don't understand what does it mean "We are still, in general, completing the message behind the scenes when a user hasn't explicitly opted in to this behavior".

I'm saying that we are still automatically completing the message in general. If user code throws an exception, we wouldn't automatically complete, but I wouldn't expect users to intentionally throw an exception in order to stop a message from being completed. If they were doing this, I would think using AutoComplete = false would be a better fit. An exception should therefore occur only under exceptional circumstances. So my point was that AutoComplete=true will still generally delete messages from the service without the user opting in to this behavior.

I am not saying that you're wrong. We all entitled to our opinions. What I'm saying is that an abstraction that worked well for years (Track 0 and 1) should not be changed just because it "feels wrong" w/o some strong evidence. Is there an issue raised in the Track 1 repo to change this behaviour due to its destructive nature? Was there an issue raised with Track 0? Is there an issue with this mondo repo? How many support cases Azure support had that would indicate this abstraction has caused issues? If you can prove that it was at a significant percentage of customers that complained about, I'll agree with the proposed behaviour change. But until then, I'll stay with "if it's not broken, don't fix it".

That's fair. I will do some digging on this. As a hypothetical, if there wasn't a Track 0 or Track 1, would you still prefer the default being True? And as usual, thanks for the great input!

Not definitive one way or the other, but I did a search for issues that contain MessageHandlerOptions - The majority of the issues are setting AutoComplete = false in the code snippets. It's true that we wouldn't expect to see AutoComplete = true much, since it is the default, but in the majority of the examples AutoComplete was not the only property being set. In other words, we could expect to still find examples where AutoComplete is omitted but MessageHandlerOptions is still specified for users wanting the default behavior.

[SeanFeldman]

I'm saying that we are still automatically completing the message in general. If user code throws an exception, we wouldn't automatically complete, but I wouldn't expect users to intentionally throw an exception in order to stop a message from being completed. If they were doing this, I would think using AutoComplete = false would be a better fit. An exception should therefore occur only under exceptional circumstances. So my point was that AutoComplete=true will still generally delete messages from the service without the user opting in to this behavior.

This is not aligned with the most real-world scenario. A normal flow would demand message completion if the processing does not fail. Otherwise, If there's a deviation such as a need to defer or dead-letter, it's an explicit operation and intentional.

That's fair. I will do some digging on this. As a hypothetical, if there wasn't a Track 0 or Track 1, would you still prefer the default being True? And as usual, thanks for the great input!

I am not using a user callback / handler / processor anymore. It's not providing me with what I need. The model of auto-completion by default and override that if you don't want to is very convenient until complex scenarios need to be handled. For Azure Functions, it requires the change to the host.json and opt into the model of completing the messages. If this was inconvenient, it would be changed long time ago to set autoComplete to false.

Not definitive one way or the other, but I did a search for issues that contain MessageHandlerOptions - The majority of the issues are setting AutoComplete = false in the code snippets.

Of course. When working on a repro code, you want to send a message and repeat the error rather than keep resending messages - I'd do exactly that. The intent was to find issues that explicitly raised about the default behaviour of the handler options, not scan how many times a code snippet has that property set to false 😄

If you have 10 or 20 customers that say it doesn't work for them, are you going to change the default and ignore thousands that didn't raise anything? If this was a bad default, do you think everyone would be silent? Don't think so.

[JoshLove-msft]

Of course. When working on a repro code, you want to send a message and repeat the error rather than keep resending messages - I'd do exactly that. The intent was to find issues that explicitly raised about the default behaviour of the handler options, not scan how many times a code snippet has that property set to false 😄

I understand, but there were no such issues. Ideally we could rely on how users are using the SDK to drive this decision. If the majority of users were setting AutoComplete to false, I think we could agree that this should be the default value. Alas, we don't have this data, nor do we have issues explicitly calling out the default setting as being problematic.

[SeanFeldman]

we don't have this data, nor do we have issues explicitly calling out the default setting as being problematic.

Yep.

[JoshLove-msft]

Proposal to expose SessionCompleted event - https://github.com/Azure/azure-sdk-for-net/issues/11349

[ShaneCourtrille]

Regarding the StopProcessingAsync() / CancellationToken interplay, does this lay to rest #10071? Is there a timeout that we'll have to abide by before being forcefully killed off or do we have a reasonable amount of time to finish up and gracefully end our processing (or just finish it up if we're right at the end).

And what is the expected behavior going to be if we raise an OperationCanceledException? Technically this is not supposed to be an exception which indicates failure but is it safe to assume that if AutoComplete is true then this will still be treated as an exception and completion will not occur?

[JoshLove-msft]

In the new library, StopProcessing will await all ongoing message callbacks. This means they will be allowed to finish no longer how long they take. You will have access to the CancellationToken in the callback event args so that you can cancel early when the processor is being stopped.

If the user callback throws, then the message will not be completed automatically.