The implementation of the AsyncEventingBasicConsumer is not safe when there are multiple handlers attached

[quixoticaxis]

The invocations of the events in AsyncEventingBasicConsumer are not safe when there are multiple handlers attached in a sense that they only await the handler that had been attached last.
For example, the Received event is invoked like this:

await (Shutdown?.Invoke(this, reason) ?? Task.CompletedTask).ConfigureAwait(false);

This invocation is starting the handlers one by one, and, as soon as one returns a Task, it moves to the next one. When it reaches the last handler, it grabs its resulting task and awaits only this single Task (or Task.CompletedTask if the last handler happens to return null). I suppose this is not intended.

If the idea is to await all of the handlers (and, considering the remarks that state that "Handlers must copy or fully use delivery body before returning", it is), something along the lines of

var received = Received;
await Task.WhenAll(received.GetInvocationList()
    .Select(handler => handler.DynamicInvoke(
        this,
        new BasicDeliverEventArgs(
            consumerTag,
            deliveryTag,
            redelivered,
            exchange,
            routingKey,
            properties,
            body)) as Task
        ?? Task.CompletedTask))
        .ConfigureAwait(false);

may fix the issue.

P.s. I've initially asked the question in the mailing lists, but I guess it's more comfortable to discuss the code on github.

[lukebakken]

I think I understand what is being asked here. This project is open-source and we would welcome a pull request that implements a solution with a test that demonstrates the problem with the current implementation. Thank you.

[bording]

While I'm not sure why you'd want multiple handlers attached to the the events, you are correct that the the way they are being invoked right now prevents that from working properly.

I wouldn't to see LINQ introduced into this code path, though, so a foreach loop over the GetInvocationList results probably makes more sense.

[bording]

Microsoft does have an MIT licensed InvokeAsync extension method that could potentially be used as inspiration, but I'll leave it to @lukebakken to determine if there are any licensing considerations around that.

[stebet]

Good catch!