Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Kermel opened this issue Mar 7, 2024 · 1 comment

Comments

@Kermel
Copy link

Kermel commented Mar 7, 2024

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
Copy link
Author

Kermel commented Mar 8, 2024

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

  • sometimes, the code goes through the "reflective - else" block and throws an exception, sometimes it goes through the expected "if" block, therefore I'm issuing a pull request to fix the reflective block to avoid calling for "GetMethod" - which throws and exception when there are more methods of the same name and instead to call directly "InvokeMember" with the given parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant