Event handler improvements

[mtmk]

We should consider instead doing something along the lines of (genuinely apologize for late night napkin concept but wanted to have the thought out there, both for the sake of avoiding breaking things for current users as well as making it potentially easier for consumers of all types to wire up):

//again, forgive (probably some) incorrectness

//TODO: Need to allow for ordering of execution.
//NOTE: Due to ordering, may need an ordered dictionary with locks 
//           rather than ConcurrentDictionary
private readonly ConcurrentDictionary<object, Func<object, string,ValueTask> _disconnectHandlers = new();

    //Assumes AsyncEventHandler is now ValueTask delegate type.
    public event AsyncEventHandler<string>? ConnectionDisconnectedAsync
    {
        add { _disconnectHandlers.TryAdd(value, value); }
        remove { _disconnectHandlers.TryRemove(value, out _); }
    }
    public event EventHandler<string>? ConnectionDisconnected
    {
        add { _disconnectHandlers.TryAdd(value, 
                 v=> {
                            return  (send,arg) => 
                                    { 
                                        v(send,arg); 
                                        return ValueTask.CompletedTask}; 
                                    }
                         }
        remove 
        { 
            _disconnectHandlers.TryRemove(value, out _); 
        }    
    }
    //Could also have Task variant that converts to ValueTask

Again, I'm not certain whether this would have a perf benefit [0] but at least it would help avoid breaking changes of various sorts...

Not sure if doing such would break things like outside callers trying to raise but in general that is a questionable pattern.

[0] - It very well might, between avoiding the casts as well as the cost of .GetInvocationList()'s internal cloning of the list, as well as the ability to get rid of casts in the main event dispatch loop.

Originally posted by @to11mtm in #324 (review)

---

I also investigated if we could get rid of the GetInvocationList() copy (other than by caching the result), and came across this interesting dotnet/runtime#41849 - so it looks like this is something that can be improved in the future.

Originally posted by @mnmr in #324 (comment)

[jasper-d]

Sorry, I didn't follow the entire event handler discussion and it's maybe to late now, but did you consider exposing through a ChannelReader<T> instead? Then registrations wouldn't need to be managed and concurrency shouldn't be much a concern. There could be a public channel reader for external code and internal ones for JS/KV. Clients could then simply read the channel and do what they want. Channel would be closed when connection is disposed.

[mtmk]

it might be a little late unfortunately not too late though we're still in preview with this feature. It would also solve some of the concerns raised before regarding application code not removing the event handlers. The only downside I guess is events are well understood and having a channel instead might be somewhat confusing for some devs.

[to11mtm]

I think channels vs events is an interesting choice...

On one hand there's some value to the event handlers (even with wanting to have a better way to deal with invocation) as it is an easy way to wire up diag or other hacky bits with minimal ceremony...

On the other hand, providing a channel would make certain things easier, one can always provide a bootstrapper example if they need it (or provide some 'sugar' magic, but that is more effort)

//Napkin code, may need slight massaging to compile.
public class EventChannelBootstrapHelper
{
    public EventHandler<string> OnDisconnected;
    public EventChannelBootstrapHelper(NatsConnection nc, CancellationToken token)
    {
        Task.Run(async ()=>
        {
            await foreach (var item in nc.ConnectionEventsChannel.ReadAllAsync(token)
            {
                if (item.Item1 is EventType.ConnectionDisconnected)
                {
                     OnDisconnected.Invoke(item.item2,this);
                }
            }
        })
    }
}

(edit, hit post too early)
However that would be a breaking change. I should also admit admit I don't care for events personally but know sometimes they are the best option for a lot of things (again, even despite the footguns).

An additional challenge to consider, would be what we would want to do if -nobody- bothered to put a listener on. Trying to assume a bounded capacity is difficult and unbounded would have it's own problems, I suppose one could do some sort of 'attach' API but that could come with it's own footguns.

It's intriguing, I think it would need some good thinking out to make sure users get an API that doesn't have weird surprises; that said this tends to be a hard problem space. The biggest thought when it comes to that, is, again, I am not a fan of event however for this sort of problem, it does have the advantage of having a known, well documented set of footguns, vs the risk of introducing our own 'new' footguns with other paradigms.

Which... (I swear, I -do- dislike event!) also worth noting, with certain considerations on an custom invoker and holder, you -can- relax some footguns of default behavior (if one wants to get brave, and document appropriately)

[mnmr]

An additional challenge to consider, would be what we would want to do if -nobody- bothered to put a listener on. Trying to assume a bounded capacity is difficult and unbounded would have it's own problems, I suppose one could do some sort of 'attach' API but that could come with it's own footguns.

The queueing aspect of channels would be the primary difference between the two (in addition to the push vs pull mechanics), but I'm not convinced that (additional) buffering of these messages is really in anyones interest (internally or for consumers).

If listeners are unable to process notifications as they happen, any compensating action taken is likely to be equally outdated (e.g. connection may already have been restored). Slow consumers would thus likely be more interested in event aggregation/consolidation than buffering, but this problem space also comes with footguns.

I'm not loving event and think it mosly belongs in Windows/UI programming, but agree that it is a well known thing that most users will understand how to use. +1 for simplicity from me.

[mtmk]

An additional challenge to consider, would be what we would want to do if -nobody- bothered to put a listener on. Trying to assume a bounded capacity is difficult and unbounded would have it's own problems, I suppose one could do some sort of 'attach' API but that could come with it's own footguns.

This would be a problem. Would some sort of 'attach' easy to implement / maintain? Are there other options to make sure channel doesn't get out of hand, and we don't want to drop certain messages like 'MessageDropped' ... Unless we want 'MessageDroppedDropped' event 😃

[jasper-d]

I think limiting the channel size should be fine. I have never used the callbacks in nats.net v1 for anything else than logging/metrics and I wonder what else they could be used for.

In terms of API ergonomics/familiarity, I think outside of the mentioned WinForms, channels are by now much more prevalent compared events (which always felt weird to me). I've a hard time imagining how they could be used to shoot oneself.

[mtmk]

I think the main issue is how to decide if we should write the events to the event channel i.e. is the application code even interested in the events, or would there be more than one 'subscription'?

How about introducing a client option to take an event channel writer? So the application would be in full control of how the channel should behave.

[mtmk]

good points here: https://twitter.com/davidfowl/status/1766828710542655646