Proposal: Full async channel interface (IModel)

[bollhals]

I've drafted a proposal for a new IModel interface that I tried to keep as slim as possible while changing everything relevant to Async.
This proposal is in no way finalized and should serve as a discussion base. (I'll update the proposal accordingly)
Please also verify the naming of the members and methods, I think with the breaking change that we do anyway, we should make sure the new names are exactly what we want.

A few things to highlight:

• I renamed the interface to IChannel to match other languages.
• I changed the naming generally to reflect what it is doing away from the protocol names used previously (I think they're confusing for a user without the background of the protocol).
• I named the methods in a style where it begins with a verb and all async methods have the Async Postfix. Events in past tense.
• All Async methods return ValueTask or ValueTask
• All passive methods were replaced by an additional parameter "throwOnMismatch"
• All noWait methods were replaced by an additional parameter "waitForConfirmation"
• A few methods were removed (comments suggested deprecation or I felt they are not useful)
• I've added several todos at places where things are still unclear / need discussion
• I've tried to group them for better visibility.

Prerequisites:

• Used ValueTuple => Not possible for net461
• Used IAsyncDisposable => Not possible for net461, Reference Microsoft.Bcl.AsyncInterfaces or target .net core 3.1
• Used nullable reference types => Not possible for net461, Workaround possible for .netstandard 2.0

As a reference, here's the current interface.

    public interface IChannel : IDisposable, IAsyncDisposable
    {
        int ChannelNumber { get; }
        bool IsOpen { get; }
        TimeSpan ContinuationTimeout { get; set; }

        /****************************
         * Various methods
         ****************************/
        ValueTask SetQosAsync(uint prefetchSize, ushort prefetchCount, bool global);
        // exception, context-detail
        event Action<Exception, Dictionary<string, object>?>? UnhandledExceptionOccurred;
        // active, TODO ask is this still needed?
        event Action<bool>? FlowControlChanged;

        /****************************
         * Message retrieval methods
         ****************************/
        ValueTask<string> ActivateConsumerAsync(IBasicConsumer consumer, string queue, bool autoAck, string consumerTag = "", bool noLocal = false, bool exclusive = false, IDictionary<string, object>? arguments = null);
        ValueTask CancelConsumerAsync(string consumerTag, bool waitForConfirmation = true);
        // TODO return value depends on how we represent messages
        ValueTask<BasicGetResult?> RetrieveSingleMessageAsync(string queue, bool autoAck);
        ValueTask AckMessageAsync(ulong deliveryTag, bool multiple);
        ValueTask NackMessageAsync(ulong deliveryTag, bool multiple, bool requeue);
        // TODO ask is this still needed?
        ValueTask RejectMessageAsync(ulong deliveryTag, bool requeue);

        /****************************
         * Message publication methods
         ****************************/
        event MessageDeliveryFailedDelegate? MessageDeliveryFailed;
        ValueTask PublishMessageAsync(string exchange, string routingKey, IBasicProperties? basicProperties, ReadOnlyMemory<byte> body, bool mandatory = false);
        // Depends on #972 
        ValueTask PublishMessageAsync(ReadOnlyMemory<byte> exchange, ReadOnlyMemory<byte> routingKey, IBasicProperties? basicProperties, ReadOnlyMemory<byte> body, bool mandatory = false);
        ValueTask ActivatePublishAcksAsync();
        // delivery tag, multiple, isAck
        event Action<ulong, bool, bool>? PublishAckReceived;
        event Action<ulong>? NewPublishTagUsed;
        ValueTask<ulong> PublishAckableMessageAsync(string exchange, string routingKey, IBasicProperties? basicProperties, ReadOnlyMemory<byte> body, bool mandatory = false);

        /****************************
         * Close methods
         ****************************/
        // CloseReason
        event Action<ShutdownEventArgs>? Shutdown;
        ShutdownEventArgs? CloseReason { get; }
        ValueTask AbortAsync(ushort replyCode, string replyText);
        ValueTask CloseAsync(ushort replyCode, string replyText);

        /****************************
         * Exchange methods
         ****************************/
        ValueTask DeclareExchangeAsync(string exchange, string type, bool durable, bool autoDelete, bool throwOnMismatch = true, IDictionary<string, object>? arguments = null, bool waitForConfirmation = true);
        ValueTask DeleteExchangeAsync(string exchange, bool ifUnused = false, bool waitForConfirmation = true);
        ValueTask BindExchangeAsync(string destination, string source, string routingKey, IDictionary<string, object>? arguments = null, bool waitForConfirmation = true);
        ValueTask UnbindExchangeAsync(string destination, string source, string routingKey, IDictionary<string, object>? arguments = null, bool waitForConfirmation = true);

        /****************************
         * Queue methods
         ****************************/
        ValueTask<(string QueueName, uint MessageCount, uint ConsumerCount)> DeclareQueueAsync(string queue, bool durable, bool exclusive, bool autoDelete, bool throwOnMismatch = true, IDictionary<string, object>? arguments = null);
        ValueTask DeclareQueueWithoutConfirmationAsync(string queue, bool durable, bool exclusive, bool autoDelete, IDictionary<string, object>? arguments = null);
        ValueTask<uint> DeleteQueueAsync(string queue, bool ifUnused = false, bool ifEmpty = false);
        ValueTask DeleteQueueWithoutConfirmationAsync(string queue, bool ifUnused = false, bool ifEmpty = false);
        ValueTask BindQueueAsync(string queue, string exchange, string routingKey, IDictionary<string, object>? arguments = null, bool waitForConfirmation = true);
        ValueTask UnbindQueueAsync(string queue, string exchange, string routingKey, IDictionary<string, object>? arguments = null);
        ValueTask<uint> PurgeQueueAsync(string queue);

        /****************************
         * Transaction methods
         ****************************/
        ValueTask ActivateTransactionsAsync();
        ValueTask CommitTransactionAsync();
        ValueTask RollbackTransactionAsync();
    }

    // Delegate definitions due to "in" modifier
    public delegate void MessageDeliveryFailedDelegate(in Message message, string replyText, ushort replyCode);
 
    // Auxiliary types
    public readonly struct Message
    {
        public readonly string Exchange;
        public readonly string RoutingKey;
        public readonly IBasicProperties Properties;
        public readonly ReadOnlyMemory<byte> Body;

        public Message(string exchange, string routingKey, IBasicProperties properties, ReadOnlyMemory<byte> body)
        {
            Exchange = exchange;
            RoutingKey = routingKey;
            Properties = properties;
            Body = body;
        }
    }

    // Extension methods
    public static class ChannelExtensions
    {
        // Queue methods
        ValueTask<(string QueueName, uint MessageCount, uint ConsumerCount)> DeclareQueueAsync(this Channel channel, string queue = "", bool durable = false, bool exclusive = true, bool autoDelete = true, bool throwOnMismatch = true, IDictionary<string, object>? arguments = null);
        ValueTask<uint> GetQueueMessageCountAsync(this Channel channel, string queue);
        ValueTask<uint> GetQueueConsumerCountAsync(this Channel channel, string queue);

        // Close methods
        ValueTask CloseAsync(this Channel channel);
        ValueTask AbortAsync(this Channel channel);
    }

The WaitForConfirms methods I plan to replace with a new class that can be wrapped around IChannel, so customer can decide if they want to use our basic implementation or build something on their own.

    public sealed class PublishAckAwaiter : IValueTaskSource<bool>
    {
        public PublishAckAwaiter(IChannel channel) { }

        ValueTask<bool> WaitForAckAsync(ulong publishTag, CancellationToken cancellation) { }
        ValueTask<bool> WaitForAckOrCloseAsync(ulong publishTag, CancellationToken cancellation) { }
        ValueTask<bool> WaitForAllAcksAsync(CancellationToken cancellation) {}
        ValueTask<bool> WaitForAllAcksOrCloseAsync(CancellationToken cancellation) {}
    }

FYI removed methods:

    public interface IModel : IDisposable
    {
        // Does anyone really use it? Removed due to not known by me.
        IBasicConsumer DefaultConsumer { get; set; }
        // not needed if we have IsOpen & ShutdownReason
        bool IsClosed { get; }
        // replaced by return value on publish method
        ulong NextPublishSeqNo { get; }

        // deprecated according to comments
        event EventHandler<EventArgs> BasicRecoverOk;
        void BasicRecover(bool requeue);
        void BasicRecoverAsync(bool requeue);

        // removed from interface, can be delivered as extension methods
        void Abort();
        void Close();

        // "new BasicProperty()" / "new BasicPublishBatch()" => no need to hide them behind these methods
        IBasicPublishBatch CreateBasicPublishBatch();
        IBasicPublishBatch CreateBasicPublishBatch(int sizeHint);
        IBasicProperties CreateBasicProperties();
    }

[stebet]

This is looking pretty good i.m.o.

Here's an additional suggestion from me:
I'm wondering if a better implementation than having the events is to have a "handler" interface that can be registered in the channels, so instead of events like FlowControlChanged etc. there would be an interface called IChannelHandler, that could be registered like consumers. These handlers would exposes the standard methods:

public interface IChannelHandler
{
  ValueTask OnFlowControl(IChannel channel, bool active);
  ValueTask OnPublicAckReceived(IChannel channel);
  ...
}

That way it's easy to override whatever logic to take care of in a class (logging, waiting for publish confirms etc.) and then register it with the channel interface channel.RegisterHandler(myCustomHandler);.

The channel can maintain those handlers in a list and execute them (in parallel if needed) as events happen. That way those implementing the handlers can also do things asynchronously with minimal effort and the channel code would be free of all the async event workarounds we currently have. Logging and all sorts of instrumentation or special logic can then be added to augment the channel interface, especially if the handlers can have a reference to the Channel objects themselves and access other things.

Similar things could be done for Connections to augment them in some way if applicable.

[bording]

I think we should consider if we want to continue with using interfaces as the public API. They make evolving the API in a non-breaking way difficult, unless we can be sure of support for default interface members in all targeted frameworks.

[stebet]

I think we should consider if we want to continue with using interfaces as the public API. They make evolving the API in a non-breaking way difficult, unless we can be sure of support for default interface members in all targeted frameworks.

I think all options should be considered. What would make sense in your mind? Do you mean mostly for the core functionality or also for other extensibility? Looking at comparable things in .NET Core I find it's mostly extensibility such as handlers and eventing becomes interface based but Core functionality is more rigid.

[bording]

I think all options should be considered. What would make sense in your mind? Do you mean mostly for the core functionality or also for other extensibility? Looking at comparable things in .NET Core I find it's mostly extensibility such as handlers and eventing becomes interface based but Core functionality is more rigid.

I'm thinking that, by default, we should just use regular classes to provide functionality, and only in extensibility scenarios where we explicitly expect users of the library to provide their own implementations should we provide an interface or abstract class as a base. In addition, unless we can count on the use of default interface members to be able to version interfaces in a non-breaking way, we should opt for abstract classes in any extensibility scenarios.

[bollhals]

I'm wondering if a better implementation than having the events is to have a "handler" interface that can be registered in the channels, so instead of events like FlowControlChanged etc. there would be an interface called IChannelHandler, that could be registered like consumers. These handlers would exposes the standard methods:

public interface IChannelHandler
{
  ValueTask OnFlowControl(IChannel channel, bool active);
  ValueTask OnPublicAckReceived(IChannel channel);
  ...
}

I do like the extensibility idea, but I don't see a lot of benefits compared to events.

• What if you're only interested in parts of these methods, you'd have to implement them all and the lib would have to call all of them.
• (IMO) Performance wise this way would also be less efficient as it might calls unnecessary and also has to make virtual calls due to the interface.

What benefits do you see in this approach?

I think all options should be considered. What would make sense in your mind? Do you mean mostly for the core functionality or also for other extensibility? Looking at comparable things in .NET Core I find it's mostly extensibility such as handlers and eventing becomes interface based but Core functionality is more rigid.

I'm thinking that, by default, we should just use regular classes to provide functionality, and only in extensibility scenarios where we explicitly expect users of the library to provide their own implementations should we provide an interface or abstract class as a base.

Absolutely agree with this.

[bollhals]

Updated initial post with removed methods and reason. (Also I'm working on making them nullable aware)
Additionally I've added the section of prerequisites:

Prerequisites:
Used ValueTuple => Not possible for net461
Used IAsyncDisposable => Not possible for net461, Reference Microsoft.Bcl.AsyncInterfaces or target .net core 3.1
Used nullable reference types => Not possible for net461, Workaround possible for .netstandard 2.0

[bording]

Any reason not to move this from interface IChannel to class Channel?

[stebet]

I do like the extensibility idea, but I don't see a lot of benefits compared to events.

• What if you're only interested in parts of these methods, you'd have to implement them all and the lib would have to call all of them.
• (IMO) Performance wise this way would also be less efficient as it might calls unnecessary and also has to make virtual calls due to the interface.

Abstract classes/virtual methods would also work, and could provide the option of only overriding the methods one is interested in.

The reason I don't like events is because they are error prone, require bookkeeping and are not straightforward/ugly when you want them to be async. However, it should be easy for users to create a handler that exposed the OnXXX methods as events if that's needed, without forcing them into to specific custom AsyncEvent delegates. A basic handler that does that would also be easy to ship with the library.

I'm not really worried about performance of virtual dispatch for these scenarios, since events aren't much cheaper anyways and they are also harder to run in parallel.

[JanEggers]

Any reason not to move this from interface IChannel to class Channel

yes. it forces this library to provide the implementation in that class.

For me it is a step in the wrong direction to go from interface to class. You dont have any benifit from using a class.

If you have a breaking change it will break the class the same way the interface breaks.

If you want to reduce the impact of breaking changes the interface(s) need to be smaller and more focused on a single thing.

regarding IChannelHandler:

my personal opinion is that if you want to have a handler interface instead of event pattern you should use IObservable instead of rolling your own interface. IObservable is an interface designed for events with existing tooling around it.

[stebet]

regarding IChannelHandler:

my personal opinion is that if you want to have a handler interface instead of event pattern you should use IObservable instead of rolling your own interface. IObservable is an interface designed for events with existing tooling around it.

IObservable<T> does not support running asynchronously if needed and require bookkeeping like events do. Handlers that expose IObservable<T> methods can however be provided just like handlers that expose events. Abstract classes or interfaces are the best foundational blocks i.m.o to expose event based pattern, with events/observable being able to be implemented as derived things from them.

This is pretty much most extensibility works for example in ASP.NET Core.

[bollhals]

Any reason not to move this from interface IChannel to class Channel

yes. it forces this library to provide the implementation in that class.

For me it is a step in the wrong direction to go from interface to class. You dont have any benifit from using a class.

If you have a breaking change it will break the class the same way the interface breaks.

There is a performance benefit from using classes vs interfaces (Inlining & non virtual method calls).
What benefits do interfaces have for this specific type?

• There won't be different implementation for it.
• There isn't any benefit when we have breaking changes.
• We want to provide an implementation of this class anyway.

I see the point of interface usage in other areas, but I don't see it for this type. Could you elaborate a bit more about this @JanEggers ?

Abstract classes/virtual methods would also work, and could provide the option of only overriding the methods one is interested in.

The reason I don't like events is because they are error prone, require bookkeeping and are not straightforward/ugly when you want them to be async. However, it should be easy for users to create a handler that exposed the OnXXX methods as events if that's needed, without forcing them into to specific custom AsyncEvent delegates. A basic handler that does that would also be easy to ship with the library.

I'm not really worried about performance of virtual dispatch for these scenarios, since events aren't much cheaper anyways and they are also harder to run in parallel.

I think from the implementation point of view you kind of have to do the same, e.g you got to store them somewhere (List vs eventA, eventB) and you got to invoke them (non task returning ones are simpler with events, task returning ones are basically implemented the same (iterate over all and call them).
What differs to me is how the users of this library have to interact with the library (eventhandler vs overwriting some methods (abstract or interface)).

What I question is,
a) how obvious is it to first have to call a "RegisterChannelHandler" method to then receive updates within the implementing class vs seeing the events on the Channel ready to go
b) about abstract class vs interface boils down to how would you most likely consume these invokes. Meaning are these most likely self implemented single purpose classes or would you want to register a "handle all the rabbitmq channel stuff"-class that might not be able to inherit from the abstract class. We could also provide both, yes, but then again the visibility suffers as there are multiple ways to achieve the same goal (which is nice, don't get me wrong, but it also possibly confuses some users)

I think the number of use cases for the currently exposed events are too low to warrant the ChannelHandler class. If we would expose more events and possibly group them for different consumer groups (e.g. one for acks, one for managing the connection, one for exception handling, ...) I think the handler approach would make more sense, but for now, I'm not sure whether it complicates things more than it helps... 🤔

[JanEggers]

Could you elaborate a bit more about this

I dont know any more facts that benifit interfaces over classes. Maybe mocking is easier. It just my opinion that interfaces are the way to express public api's.

Microsoft even goes one step further an provides ***.abstractions.dll that mostly provide Interfaces and utility classes that build uppon these interfaces.

And again as we both agree it does not make much of a difference in case there is a breaking changes.

Also if the is a measurable performance benifit in using a class here im all for it as performance is more important for me.

[michaelklishin]

Moving to 8.0 so that we can ship a 7.0 with a smaller set of breaking API changes if necessary.