[FEATURE REQ] Azure Service Bus Enforce mandatory processor arguments on CreateProcessor

[danielmarbach]

Right now I can do the following

await using var receiver = serviceBusClient.CreateProcessor(destination, optionalOptions);
receiver.ProcessMessageAsync += async processMessagesEventArgs =>
{
    // processing logic
};

but if I try to start this I get a runtime exception telling me

Unhandled exception. System.InvalidOperationException: Cannot begin processing without ProcessErrorAsync handler set.
   at Azure.Messaging.ServiceBus.ServiceBusProcessor.ValidateErrorHandler()
   at Azure.Messaging.ServiceBus.ServiceBusProcessor.StartProcessingAsync(CancellationToken cancellationToken)

given that only ever one handler can be assigned, which is enforced by

azure-sdk-for-net/sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusProcessor.cs

Line 289 in 6e4c8ea

throw new NotSupportedException(Resources.HandlerHasAlreadyBeenAssigned);

why not enforce the two required handler parameters as input to CreateProcessor? How useful is it that those two are exposed as "event handler delegate" that are not really event handler delegates (because normally you could have multiple, here you can't) and in all the cases you always have to set both?

I think a similar API to the following would be more intention revealing and avoid runtime errors:

public ServiceBusProcessor CreateProcessor(otherMandatoryParams..., Func<ProcessMessageEventArgs, Task> messageHandler, Func<ProcessErrorEventArgs> errorHandler, ServiceBusSessionReceiverOptions options = default)

Once you have that signature it becomes clear for me as a user of the API that I always have to specify both. Then it might also become an option to rename the parameter from *EventArgs so something different because really those handlers are not real multicast .NET events.

[danielmarbach]

// cc @jongio @JoshLove-msft @ramya-rao-a @SeanFeldman

[SeanFeldman]

Track 2 had this as an overload. When supplied, it would be called. Otherwise, the exception would bubble up. While it was not a trivial signature it did reveal the intent. A mandatory error handler should be required at construction time if the processor has to have it.

[JoshLove-msft]

Hi @danielmarbach,
Thanks for submitting this issue. A good deal of debate went into this topic. Our approach in both Event Hubs and Service Bus relies on the knowledge gained from user studies that most .NET users are more comfortable with event handler semantics than other callback semantics (as mentioned in the Framework Design Guidelines). Also it would still be possible to construct a multicast delegate even with a Func by chaining them, so the fact that only one handler should ever be specified doesn't necessarily mean that Funcs are more appropriate.

[JoshLove-msft]

/cc @jsquire @KrzysztofCwalina

[KrzysztofCwalina]

The reasons we prefer events (in this case) are:

1. IDEs provide completion help for hooking up event handlers.
2. Short constructors and properties/events help people learn APIs gradually, i.e. learning step 1: there is a message handler, learning step 2: there is an error handler I need to think about. We have a lot of evidence from usability studies that such APIs (APIs that allow users to learn "progressively") are better received by broad range of customers.

BTW, whether something is a Func delegate or event does not change how many handlers can be chained, i.e.

using System;

class Program
{
    static void Main() 
    {
        Func<bool> handler = () => { return true; };
        handler += () => { return false; }; // last delegate wins, i.e. handler() == false
        var processor = new Processor(handler);
    }    
}

class Processor
{
    public Processor(Func<bool> handler) { }
}

[danielmarbach]

Fair enough. This is my personal feedback from my fiddling around with the new API. That certainly doesn't invalidate all the previous user studies. Mine is just an opinion I wanted to share. Happy to close after seeing the explanation.

My mind was kind of caught up in finding a solution that would lead me to use it the proper way without having to be told with a runtime exception

[danielmarbach]

@KrzysztofCwalina correct chaining is possible. Although the syntax you showed here is not exactly the canonical way how I've seen developers adding multiple event handlers.

Most of the time I've seen people doing

thing.Event ±= Handler1;
thing.Event += Handler2;

[danielmarbach]

Thanks for sharing the background info. Learned a ton.

[KrzysztofCwalina]

I agree that chaining delegates is not something that people do. Many don't even know that it's possible. But in real world, not many people chain event handlers either. So, in the end, the issue of chaining multiple handlers/delegates is a red herring IMO. The real tradeoff is compile enforcement vs runtime enforcement of the requirement to have two handlers. I think compile time enforcement is better when the error/exception (caused by API misuse) is likely to not happen in testing. Runtime enforcement is better if the exception is going to happen in testing. In the case of the processor, no processing will run unless both handlers are provided (~100% chance of exception when testing).