jamesmh / coravel

Near-zero config .NET library that makes advanced application features like Task Scheduling, Caching, Queuing, Event Broadcasting, and more a breeze!
https://docs.coravel.net/Installation/
MIT License
3.63k stars 243 forks source link

Dispatcher.Broadcast doesn't work correctly when my listener class implements more than one IListener<> interfaces #378

Open Kermel opened 2 months ago

Kermel commented 2 months ago

Affected Coravel Feature Events boradcasting and Events handling

Describe the bug I have experimented with Events broadcasting and created a listener class to handle both the queue events:

internal class ExperimentalQueueTasksListener : IListener<QueueTaskStarted>, IListener<QueueTaskCompleted>
{
    public async Task HandleAsync(QueueTaskStarted broadcasted)
    {
        await Task.Run(() =>
        {
            Console.WriteLine($"Queue task started: {broadcasted.StartedAtUtc} - ({broadcasted.Guid})");
        });
    }

    public async Task HandleAsync(QueueTaskCompleted broadcasted)
    {
        await Task.Run(() =>
        {
            Console.WriteLine($"Queue task completed: {broadcasted.CompletedAtUtc} - ({broadcasted.Guid})");
        });
    }
}

and it doesn't do anything. In the background, I have found exceptions thrown by the Dispatcher talking about the ambiguous definition of the "HandleAsync" methods. This is because the code always goes throught the "else" path in the Dispatcher.Broadcast method. The reason is, that this piece of code: if (obj is IListener<TEvent> listener) is never true, because the typeof(IListener) says it is IListener (i.e. the most generic variant)

Expected behaviour Event handling works even for listeners capable of handling multiple events.

Suggested fix Actually, I didn't figure out how to exactly fix this as the dynamic casting to a generic type (with runtime-unknown generic type) is a problem. This might help: Dispatcher.cs line 62 containing the if (obj is IListener<TEvent> listener) is never true but this: if(obj.GetType().IsAssignableTo(typeof(IListener<TEvent>)) is OK and true as expected

(also, the items in the listeners collection have correct types because that's how they get there during subscription so the type check is not needed, just type casting...)

Maybe making the HandleAsync method generic with a generic parameter of the TEvent type? Then the whole if-else block might be dropped and the reflection invocation of the generic method could be used instead. Something like this:

var method = listenerType.GetMethod("HandleAsync");
var t = toBroadcast.GetType();
var genericMethoid = method.MakeGenericMethod(t);
genericMethod.Invoke(obj, new object[] { toBroadcast });

Then you might easily add the listener interfaces to my normal classes and be ready for handling many events and incorporating to the business logic

OR:

after verifying the type if(obj.GetType().IsAssignableTo(typeof(IListener<TEvent>))) cast the obj to dynamic and then call the HandleTask method of which we know it exists and is correct...

Kermel commented 2 months ago

Hmm after some more experiments I found out... that I cannot find out where the problem is. :/