Improve event publishing in DefaultBasicConsumer

[antiduh]

See:

rabbitmq-dotnet-client/projects/client/RabbitMQ.Client/src/client/api/DefaultBasicConsumer.cs

Line 104 in 13a9567

public event EventHandler<ConsumerEventArgs> ConsumerCancelled

and

rabbitmq-dotnet-client/projects/client/RabbitMQ.Client/src/client/api/DefaultBasicConsumer.cs

Line 194 in 13a9567

public virtual void OnCancel()

In C#, events are already thread safe. No lock protection is needed to subscribe to events, or to fire events. See section 15.8.2 "Field-like events" in the spec, https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-334.pdf

Prior to C#6, the code for the ConsumerCancelled event would be:

public event EventHandler<ConsumerEventArgs> ConsumerCancelled;

public virtual void OnCancel()
{
    IsRunning = false;
    var handler = ConsumerCancelled;

    if( handler != null ) 
    {
        handler(this, new ConsumerEventArgs(ConsumerTag);
    }
}

After C#6 (supported by Visual Studio 2015). you can further simplify the OnCancel() method with:

public virtual void OnCancel()
{
    IsRunning = false;
    ConsumerCancelled?.Invoke( this, new ConsumerEventArgs(ConsumerTag);
}

[lukebakken]

We would be happy to review a pull request for this. Thank you.

[antiduh]

I'll put one together in a day or two. Assign this to me, if you'd like.
Which minimum C# syntax version would you like me to support? C# 6?

[michaelklishin]

5.x series still support .NET 4.5.1.

[bording]

5.x series still support .NET 4.5.1.

For the most part , C# version and .NET Framework version aren't related. Most C# features work on all versions of the .NET Framework.

[michaelklishin]

I'm all for using the latest version of the language as long as we don't have to bump the minimum required version (at least for 5.x).

[antiduh]

The question really is which version of Visual Studio (if any) you expect everybody to compile with. VS 2015 is the first version that supports C# 6 syntax (like MyEvent?.Invoke())

[michaelklishin]

@antiduh that sounds like a reasonable baseline to me. @bording would you agree?

[bording]

The project already requires Visual Studio 2017 because it's using the new SDK-style project system and has netstandard2.0 targets.

Taking a quick look at the project settings, it appears that it's currently targeting C# 7.0.

[antiduh]

So, I think I found a bunch more problems.

Connection exposes events that aren't fired from inside the class; they're unused as far as Connection is concerned - RecoverySucceeded and ConnectionRecoveryError, and their backing fields, are never fired by Connection.

It looks like they're defined in Connection because they're exposed through the IConnection interface on behalf of AutorecoveringConnection.

You might as well replace them with:

        public event EventHandler<EventArgs> RecoverySucceeded
        {
            add { }
            remove { }
        }

To show that they're unused by Connection (while making the compiler happy).

Also, I noticed that AutorecoveringConnection does some janky stuff.

As an example, consider this:

        public event EventHandler<ShutdownEventArgs> ConnectionShutdown
        {
            add
            {
                lock (m_eventLock)
                {
                    m_recordedShutdownEventHandlers += value;
                    m_delegate.ConnectionShutdown += value;
                }
            }
            remove
            {
                lock (m_eventLock)
                {
                    m_recordedShutdownEventHandlers -= value;
                    m_delegate.ConnectionShutdown -= value;
                }
            }
        }

So what happens here is when some external class subscribes to the AutorecoveringConnection.ConnectionShutdown event, ARC forwards the event handler to its internal Connection instance (m_delegate), while still saving off a copy.

It seems this is done so that when the m_delegate instance is replaced, you can reconfigure the connection subscriptions to external users (RecoverConnectionShutdownHandlers et al).

What would be a lot easier is to have AutorecoveringConnection blindly subscribe itself to all of the events of Connection, then implement the IConnection interface, and have its handlers for Connection simply fire its own version of the event.

For instance:

public event EventHandler<ShutdownEventArgs> ConnectionShutdown;

public event EventHandler<EventArgs> ConnectionUnblocked;

// ... 
        
private void ConnectEvents( Connection connection )
{
    m_delegate.ConnectionShutdown += Delegate_ConnectionShutdown;
    m_delegate.ConnectionUnblocked += Delegate_ConnectionUnblocked;
    // ... 
}

private void DisconnectEvents( Connection connection)
{
    m_delegate.ConnectionShutdown -= Delegate_ConnectionShutdown;
    m_delegate.ConnectionUnblocked -= Delegate_ConnectionUnblocked;
    // ... 
}

private void MakeConnection( IFrameHandler fh )
{
    if( m_delegate != null )
    {
        DisconnectEvents( m_delegate );
    }

    m_delegate = new Connection( m_factory, false, fh, this.ClientProvidedName );

    ConnectEvents( m_delegate );
}

private void Delegate_ConnectionShutdown( object sender, ShutdownEventArgs e )
{
    OnConnectionShutdown();
}

private void Delegate_ConnectionUnblocked( object sender, EventArgs e )
{
    OnConnectionUnblocked();
}
        
// ... 

private void OnConnectionShutdown()
{
    ConnectionShutdown?.Invoke();
}

private void OnConnectionUnblocked()
{
    ConnectionUnblocked?.Invoke();
}

// ... 

[michaelklishin]

Connection is unaware of recovery, so AutorecoveringConnection wraps it and delegates most things but keeps track of those that can/should be recovered. This includes event handlers.

I don't see how the provided example would make recovery possible since after recovery the delegate is replaced with a brand new object with no prior state or configuration applied to it. So it may be "less janky" but completely fails to do what AutorecoveringConnection was created to do?

Any changes that are non-breaking are welcome, breaking ones can only go into 6.0 and should be weighted against their actual benefit.

[michaelklishin]

@antiduh I suggest that we take things one step at a time. Let's get to a pull request that addresses what was originally reported here and get it past QA. Any other changes can be discussed in a separate PR. Thank you.