Handling exception thrown by handlers

[Kryptos-FR]

I am evaluating the library for an internal use at my company.

One issue I have currently is that if any handler throws an exception, it will prevent other handlers from receiving the same event. While that is currently the same behavior of normal .NET events, it can cause issues.

Would you consider an overloaded method for WeakEventSource<TEventArgs>.Raise that takes a delegate that can handle any exception and decide whether event notification should continue or not.

Something like:

public void Raise(object? sender, TEventArgs args, Func<Exception, bool> exceptionHandler)
{
    var validHandlers = GetValidHandlers(_handlers);
    foreach (var handler in validHandlers)
    {
        try
        {
            handler.Invoke(sender, args);
        }
        catch (Exception ex)
        {
            if (exceptionHandler(ex))
                continue;
            throw;
        }
    }
}

I was also thinking of a more advanced handler that returns a state so that bad subscriber can be automatically unsubscribed:

[Flags]
public enum ErrorHandlingFlags
{
    None = 0,
    NotThrow = 1,
    Unsubscribe = 2,
}

public void Raise(object? sender, TEventArgs args, Func<Exception, ErrorHandlingFlags> exceptionHandler)
{
    var validHandlers = GetValidHandlers(_handlers);
    foreach (var handler in validHandlers)
    {
        try
        {
            handler.Invoke(sender, args);
        }
        catch (Exception ex)
        {
            var flag = exceptionHandler(ex);
            if (flag.HasFlag(ErrorHandlingFlags.Unsubscribe))
            {
                // find a way to unsubscribe
            }

            if (flag.HasFlag(ErrorHandlingFlags.NotThrow))
                continue;

            throw;
        }
    }
}

But I'm not sure how the unsubscription can work without the original EventHandler<TEventArgs>.

[thomaslevesque]

Hi @Kryptos-FR,

Thanks for your interest!

It's an interesting idea. One of the design goals of this project was to mimic the behavior of normal .NET events as closely as possible, so I never really considered this. But there's a major difference: unlike normal .NET events, you can't access the list of individual handlers of a WeakEventSource, so there's currently no way to catch errors thrown by individual handlers. So I think your idea would be a useful addition.

I was also thinking of a more advanced handler that returns a state so that bad subscriber can be automatically unsubscribed:
...
But I'm not sure how the unsubscription can work without the original EventHandler<TEventArgs>.

Yes, it would be an interesting option. However I can't think of a good way to do that... I'll look into it.

[thomaslevesque]

Oh, by the way, how do you see this working for async events? Would the exceptionHandler be asynchronous as well?

[Kryptos-FR]

Oh, by the way, how do you see this working for async events? Would the exceptionHandler be asynchronous as well?

I would probably not suggest having the exceptions handler be asynchronous. I don't know the exact impact but async in catch block probably adds quite a lot of codegen, and is probably not cheap. That aside, what would be the case for the handler to be async? I believe a good handler should return as quickly as possible.

I was not considering that scenario since I don't need it for my case, so I might be missing something.

[thomaslevesque]

I don't know the exact impact but async in catch block probably adds quite a lot of codegen, and is probably not cheap.

Minimal impact. There's no codegen involved. Only drawback is that I can't use an exception filter in the async case, but that's not a major issue.

That aside, what would be the case for the handler to be async?

Maybe you need to log the error to a database, or something like that. I expect most exception handlers to return synchronously, but it's annoying when you're in a situation where you need to do something asynchronous, and you can't because you must return synchronously.

I could make it return a ValueTask to reduce the cost when the exception handler returns synchronously.

I believe a good handler should return as quickly as possible.

That's a good point.

On one hand, making the exception handler synchronous would probably limit the cases where this feature is usable... On the other hand, there's an easy workaround if the handler is synchronous:

List<Exception> errors = null;
await _weakEventSource.RaiseAsync(sender, args, ex =>
{
    (errors ??= new List<Exception>()).Add(ex);
    return ExceptionHandlingFlags.Handled | ExceptionHandlingFlags.Unsubscribe;
});
if (errors != null)
{
    // Asynchronously process the errors
}

I need to think more about it...

[thomaslevesque]

Anyway, the good news is that I found a way to unsubscribe automatically (still need to test, though). It involves significant internal changes, but I think they're good ones.

[Kryptos-FR]

@thomaslevesque Do you have an ETA?

I can always build my own version, but I'd preferred to keep an "official" package.
Take your time though 😃. I just need to make a decision on that by the end of next week. So if you already know that it won't be possible for you, I will go ahead a make that private version.

Thanks.

[thomaslevesque]

Hi @Kryptos-FR,

I have most of the code sitting on my laptop, but I haven't had time to come back to it since last week. I'll try to finish it in the next few days.

[thomaslevesque]

@Kryptos-FR I just published a beta (4.1.0-beta.1). Could you please try it and see if it works for you?

The changes are quite extensive (mostly because of the "unsubscribe" feature), so I think a beta phase is needed to ensure I didn't break anything.

I ended up adding 2 overloads for AsyncWeakEventSource: one with a sync exception handler, and one with an async one.

[thomaslevesque]

I'm having second thoughts about the unsubscribe feature... It can cause incorrect behavior in some scenarios.

For instance, when subscribing with a multicast delegate:

• you subscribe with a multicast delegate ABC (made up of handlers A, B, and C
• the event is raised, and handler B throws. The exception handler returns Handled | Unsubscribe, so B is removed from the handlers.
• now the handler list contains only AC
• you try to unsubscribe ABC: but the handler list no longer contains ABC, it just contains AC, so nothing happens (because of the change implemented in Change Unsubscribe to match the behavior of Delegate.Remove #13). So you can no longer unsubscribe ABC. You'd have to unsubscribe A and C separately, but you can't do that, because you don't know that ABC is no longer in the list.
  So basically, the "unsubscribe on throw" feature means that multicast delegates can be "broken" if one of their individual handlers throw.

Another issue is that the event publisher shouldn't remove subscribers unilaterally, because the subscriber wouldn't know it's been unsubscribed.

So I'm going to remove the "unsubscribe" part of this feature. It's not released yet (except in a beta), so there's no harm done. The exception handler will just return true to indicate that it handled the exception.

[Kryptos-FR]

@thomaslevesque Awesome. I don't have access to the code outside of the office, but I will test it first thing next Monday.

About your "second thought" comment. Could that be explained in the documentation instead? If one doesn't use multicast delegate, the ability to unsubscribe is nice to have.

[Kryptos-FR]

Beta-1 with unsubscription feature better fit my needs. So I will stick with it for the time being. Thanks for your hard work!

[thomaslevesque]

Hi @Kryptos-FR,

Do what you like, but keep in mind that I won't maintain the unsubscribe feature, so if there's something wrong with it, you're on your own 😉

About your "second thought" comment. Could that be explained in the documentation instead?

What kind of explanation would you expect? It's the same as with normal events, really; a subscriber typically doesn't expect to be removed by the publisher just because it threw an exception. The subscriber should catch its own exception, and unsubscribe itself if appropriate.

If one doesn't use multicast delegate, the ability to unsubscribe is nice to have.

I know, multicast is a bit of a corner case, and probably not many people use it. But I don't want to introduce a feature that I know won't work in some cases...