HandleAsync not supported

[pedrollanes]

When you use EventAggregator you can't handle async. I think that it's important because in the handler, if you want to call to async method you are force to use GetAwaiter, and in some cases I have deathlocks. Are you going to develop HandleAsync for eventAggregator?

[canton7]

Are you going to develop HandleAsync for eventAggregator?

I've got no plans to, as noone has asked for it 🙂

You're welcome to contribute this feature: it should be relatively straightforward, once you've decided on a design.

The open questions are:

1. What should happen to the Task which is returned? Should EventAggregator wait until the Task returned from one handler is completed before invoking the next handler?
2. What should happen if the Task returned from a handler completes with an exception: should that be rethrown to the caller, or something else?

Be careful of cases where you need EventAggregator to gain PublishAsync methods, and you need to start worrying about what should happen if someone calls Publish when there are async handlers.

Note that your handler methods can be async void: they're essentially the same as event handlers, so it makes sense that they do their own error-handling, and don't report status to their caller. This is probably the most straightforward approach.

[pedrollanes]

Nowadays I have no time for it, but in the future maybe I'll try. Thanks for you reply

[alexhelms]

Moving over from #238, I didn't see this one.

I'd be willing to work on this if we can agree on some answers to the questions asked by @canton7.

I think we should follow MediatR's way of doing notifications. I found a thread here talking about notifications and exceptions and I agree with this post: LuckyPennySoftware/MediatR#237 (comment)

So just to clarify:

1. What should happen to the Task which is returned? I propose we await it.
2. What should happen if the Task returned from a handler completes with an exception? I propose we let it bubble up because events should a) have their own error handling, or b) ensure they don't throw exceptions.

Thoughts?

[canton7]

I suspect there probably won't be that much in common: the signatures of most of the methods, locking strategy, etc etc is all going to have to change. Perhaps a base/helper class if it helps.

I do feel strongly that the publish method shouldn't wait for the first subscriber to completely finish before invoking the second: a single mis-behaving subscriber will block all subsequent subscribers, and it opens up the door for subscribers to receive events out-of-order. E.g:

class Subscriber1 : IAsyncHandle<Event1>
{
    public Task Handle(Event1 e1) => Task.Delay(1000):
}

class Subscriber2 : IAsyncHandle<Event1>, IAsyncHandle<Event2>
{
    public async Task Handle(Event1 e1) => Console.WriteLine("E1");
    public async Task Handle(Event2 e2) => Console.WriteLine("E2");
}

var t1 = aggregator.PublishAsync(new Event1());
await aggregator.PublishAsync(new Event2());

... might print "E2 E1" which is wrong on all counts.

It also makes thread-safety a lot harder: if Subscriber2 unsubscribes itself while the aggregator is waiting for Subscriber1 to finish, you need to be careful it isn't subsequently invoked.

In order words, I think Publish needs to effectively do await Task.WhenAll(subscribers.Select(x => x.PublishAsync(event)).

[canton7]

Another Q: should the publish method allow cancellation? And if so should that be handled by passing a CT to subscribers?

[alexhelms]

I agree but I also think sequentially awaiting each handler is also useful. I guess this is why MediatR has publish strategies 😄 I'm fine with Task.WhenAll for awaiting when all handlers are complete.

In your example t1 is not being awaited. That tells me firing Event1 is a "fire and forget" and you don't care about what happens to the task and getting "E2 E1" would be possible. I think that responsibility lies on the developer to not do that. They should have awaited the publish and that situation wouldn't have happened. If it were awaited the AsyncEventAggregator would have fired off all subscribed handlers and await them with a Task.WhenAll and returned when Subscriber1 and Subscriber2 were complete. Without the await the result would be determined by what the subscribers do.

Regarding cancellation, I think it would need to be passed through in order to stay consistent with normal async functions. The example below I think shows what you'd expect for using WhenAll and cancellation.

public class AsyncEventAggregator : IAsyncEventAggregator
{
    public Task PublishAsync(object message, CancellationToken token = default)
    {
        var tasks = new List<Task>();
        for (var handler in _handlers)
        {
            tasks.Add(handler.HandleAsync(message, token));
        }
        return Task.WhenAll(tasks);
    }
}

public class Subscriber1 : IHandleAsync<Event1>
{
    public Task HandleAsync(Event1 e, CancellationToken token)
    {
        return Task.CompletedTask;
    }
}

[canton7]

In your example t1 is not being awaited. That tells me firing Event1 is a "fire and forget" and you don't care about what happens to the task and getting "E2 E1" would be possible. I think that responsibility lies on the developer to not do that. They should have awaited the publish and that situation wouldn't have happened.

await Task.WhenAll(aggregator.Publish(new Event1()), aggregator.Publish(new Event2())) then. The fact that E1 and E2 can arrive in the wrong order is going to catch people out. It's all very well saying "the developer should not do that", but you're not the one spending your evenings fielding the support requests!

Yeah I'm inclined to agree on the cancellation bit: it's annoying having to mandate that all subscribers handle cancellation, but someone's going to ask for it at some point and there's no way of putting it in after-the-fact.

[canton7]

Just to set expectations: any implementation of an async event aggregator needs to support the following before I can merge it:

1. Channels (using the same behaviour as the current non-async event aggregator)
2. Weak subscriptions
3. Publishing on different dispatchers
4. Thread-safety:
   a. Handlers can be added and removed at any point from any thread, including while an event is being fired.
   b. Two events can be fired simultaneously from different threads.
   c. This is going to be harder with the async version than with the sync version...
5. Handle non async subscribers
6. We should do something sensible if someone subscribes to the non async aggregator with an async handler
7. Full unit test coverage
8. Doc updates

I'm happy to do bits as well of course, but all those things need to be in place eventually.

[bradencross]

I am not sure if this is a related issue or not but I am running into a situation where the EventAggretator. Publish will indefinitely hang. Even if the subscribing handle is empty and not executing any code. Basically I am publishing an event in one viewmodel and consuming it in another. When this hang happens I can pause the debugger and can see it is stuck on the EventAggretor.Publish line. It does not happen every time but often enough that it is an issue. If anyone had some insight in what might be causing this would be helpful.

[canton7]

Turn off Just My Code and look at the call stack window. If you click through it, VS should download Stylet's source so you can see where exactly it's stuck. Without more info or code a self-contained example which reproduces the issue, it's hard to help I'm afraid!

[bradencross]

Fair enough! I have tried turning off Just my code but for some reason Stylet's source code isn't downloading properly (I will look into this). In the meantime I can now see that the lockup is in the WaitHelper method of the SynchronizationContext class. Specifically WaitHandle.WaitMultipleIgnoringSyncContext(waitHandles, waitAll, millisecondsTimeout); Thanks for the fast response, I will dig into this some more and get back to you. It appears that something in my code combined with EventAggregator.Publish is causing a deadlock.

[canton7]

That's weird, as EventAggregator.Publish doesn't do any threading: handlers are invoked synchronously on the calling thread.

The source is part of the symbols package, so you'll need "NuGet.org Symbol Server" enabled under Debug -> Options -> Symbols.

Please open a new discussion, with the full stack trace you have available, and the code which is invoking EventAggregator.Publish. Thanks!

[deralbert]

Hello @canton7. Is it known when will the asynchronous version of the EventAggregator be available? I understand that there is a lot of work behind this and I just want to know what the rough time frame for the introduction of the asynchronous EventAggregator is (and whether this introduction will take place at all).

Something similar happened to the Caliburn Micro. For a long time there were no asynchronous publishers and handlers and then sometime in 2018 they were introduced in such a way that you completely lost the possibility of using synchronous versions of Publish and Handle methods. It is also possible that the messages are processed in a different order than the order in which they were sent.

Taking into account what has already been discussed here with @alexhelms, the asynchronous version of EventAggregator next to the synchronous version would be a really nice addition to Stylet. I would like to help with the implementation, but I do not have a precise plan of which steps have to be taken first in which order and I have not found any open issues for AsyncEventAggregator.

[alexhelms]

I was hoping to do some work on async publishers for the past couple months but I haven't had time. So while I would still like to, I'm not able to commit to anything. Just wanted to throw that out there.

[canton7]

I'm not working on anything in the medium term, so no ETA.

Things like the event aggregator are pretty independent from the rest of Stylet: it's provided by Stylet only as a convenience. If you need an async event aggregator, I'd encourage you to write one which meets your needs. Use the existing synchronous one as a basis as much as you need. You could probably use C.M's if you need, since it's MIT.

If you end up with something stable which meets your needs, please consider making a note of your learnings (why you had to change approach, for example), and contributing it to Stylet.

Thanks!