Make the EventSource-class extensible about dotnet-eventsource HOT 12 CLOSED

As for why the class isn't currently sealed, that's a fair point. It looks to me like that was for historical reasons due to the way some unit tests were written early on in development: in order to test the EventSource functionality separately from EventSourceService, the tests were subclassing EventSource and overriding the GetEventSourceService method to return a mock version of EventSourceService instead.

I would not use that approach now, and in fact the tests haven't actually been using it for some time. But making the class sealed at this point would technically be a breaking change, so I would save that for the next major version of this library, 5.0.0. And we don't currently have any other major changes in mind that would justify a 5.0.0 release, so I'd prefer to wait on that.

from dotnet-eventsource.

We've tried to keep to standard language idioms for these libraries, and event handlers are standard in .NET. I'm not sure what problem you're trying to solve— you mentioned what you want to do, but not why.

Also, we've avoided providing overridable methods on the general principle that object-oriented inheritance is often a problematic pattern. That's a philosophical judgment call of course, and a can of worms that I'd rather not get into here, but from my point of view as a maintainer of the library, the main issues are 1. it creates constraints on us that may make it hard to maintain the code, because now we are locked into always having this public method that behaves in this way; 2. inheritance requires the subclass to call the base class constructor, so if we want to add any features that would involve changes to the constructor, the subclass will not be able to use them.

So I think it's safe to say that if we were going to implement some kind of customizability like you're suggesting, we would want to do it with composition rather than inheritance (e.g. passing in a function). But it's still unclear to me what the goal is.

from dotnet-eventsource.

Hi @eli-darkly,

Thanks for responding to my question. Of course I understand that any choice in computer programming is ultimately a philosophical one - and, when it comes to that, I do really like the approach taken in this library, with the difference that I would normally have made the methods firing events virtual, exactly for extensibility purposes, or otherwise have marked the class sealed to make my intentions clear. Call me old-school, however, but I still very much prefer object-inheritance over object-composition (although there are certainly uses for the latter), something I believe is also echo by standard .NET.

Philosophical matters aside, though, as I have no doubt we could delve into this topic endlessly (such as that I think that the generic way the constructors are implemented now is likely to lead only to addition of overrides and/or parameters, if at all, not their removal), the reason I wanted to extend the class' functionality is to split out message processing. Currently, there's only one event handler that'll fire whenever a message is received, irrespective of the server-event that triggered it. Rather than implementing the MessageReceived-handler as a message-router that invokes individual message processing methods depending on server-event received - code that I believe would, in a general sense, be duplicated whenever a new EventSource-implementation is written - or even - god forbid - writing all message processing in one event-handler, however, I would like the EventSource to automagically route events to the correct handler for me. My solution to this is to implement listeners for individual events, following the observer-pattern. Now I know this is mostly a Java (hence Android) and JavaScript pattern, but it is more extensible than simple event handling by, in a way, using object-composition principles.

Here's my class implementation as a quick (and hence not necessarily perfect) example:

    public class CustomEventSource : EventSource
    {
        private readonly IDictionary<string, Action<MessageEvent>> _listeners =
            new Dictionary<string, Action<MessageEvent>>(StringComparer.OrdinalIgnoreCase);

        public CustomEventSource(Configuration configuration)
            : base(configuration)
        {
            MessageReceived += OnMessageReceived;
        }

        public CustomEventSource(Uri uri)
            : base(uri)
        {
            MessageReceived += OnMessageReceived;
        }

        public CustomEventSource AddListener(string eventName, Action<MessageEvent> action)
        {
            _ = action ?? throw new ArgumentNullException(nameof(action));
            _listeners.Add(GetTrueEventName(eventName), action);

            return this;
        }

        public CustomEventSource RemoveListener(string eventName)
        {
            _listeners.Remove(GetTrueEventName(eventName));

            return this;
        }

        private void OnMessageReceived(object sender, MessageReceivedEventArgs e)
        {
            var eventName = GetTrueEventName(e.EventName);
            if (_listeners.TryGetValue(eventName, out Action<MessageEvent> action))
            {
                action(e.Message);
            }
        }

        private string GetTrueEventName(string eventName)
            => string.IsNullOrWhiteSpace(eventName) ? "message" : eventName.Trim();
    }

This is then used as follows:

            var client = new CustomEventSource(configuration.Build())
                .AddListener("eventName", message =>
                {
                    // event processing
                });

Of course this could even be extended to automatically perform deserialization for the listener, although this would then likely require some form of reflection to make it work, and therefore be less desirable.

Anyway, this was/is my consideration, which I hope is a bit clearer now...

Lastly, as another interesting philosophical matter, I believe the EventSource would lend itself perfectly for the use of observables through Reactive Extensions. That way you might actually be able to easily split out different server-events for separate processing outside of the class itself, by simply publishing events as they come onto the chain, and then filtering for specific events on each subsequent chain...

from dotnet-eventsource.

With all the above, though, let me also express that after having looked at a number of SSE client libraries, yours came out as the best for what I need to do ;)

from dotnet-eventsource.

If all you want to do is to provide the equivalent of a message router... then I don't think this is even a philosophical judgment call; composition is unquestionably better than inheritance for this kind of thing. Here's why.

You aren't really trying to change the behavior of EventSource in any way except to say "when you're going to call the MessageReceived event handler— call this other logic instead." But there already is a composable hook that is perfectly suited to "calling this other logic": the event handler. There's no need for any different behavior in EventSource itself— you can already tell it to call anything you want.

In other words, I think the right solution is to simply implement a message router exactly like you did above, but implement it in a separate class, and then plug its OnMessageReceived method into the MessageReceived event of EventSource:

public class MessageRouter
{
    private readonly IDictionary<string, Action<MessageEvent>> _listeners =
        new Dictionary<string, Action<MessageEvent>>(StringComparer.OrdinalIgnoreCase);

    public MessageRouter AddListener(string eventName, Action<MessageEvent> action)
    { ... }

    public MessageRouter RemoveListener(string eventName)
    { ... }

    private void OnMessageReceived(object sender, MessageReceivedEventArgs e)
    { ... }
}

var mr = new MessageRouter();
mr.AddListener( ... );
myEventSource.MessageReceived = mr.OnMessageReceived;

You'll see that your proposed approach would really only save two lines (the creation of mr and the setting of MessageReceived), at the cost of putting constraints on the code that are completely unnecessary:

• You have to add a constructor to your class that delegates to the base class constructor, even though the functionality of those constructors has absolutely nothing to do with the feature you're trying to add.
• Much worse: your subclass must by definition derive from the real concrete implementation of EventSource, meaning that if you want to write some tests that use a mock of IEventSource instead, your subclass can't be used. Whereas if you split it out as in my example, you can easily use a MessageRouter with any IEventSource.

from dotnet-eventsource.

As you've already shown, writing the message router is easy, but we could still implement something like it as a nice-to-have feature in the library. I would just strongly oppose using inheritance to do it.

I think the fact that what you're after is pretty much the exact equivalent of a request router in an HTTP server framework illustrates why this is so. Universally as far as I know, HTTP frameworks have a basic concept of a request handler, and then there may be any number of implementations of routing which are written as implementations of that basic request handler interface, which are plugged into the server just like any other request handler would be. The router has no need to modify any aspect of the server behavior except for "how do we handle this request", and that concept is already entirely described by the idea of a request handler.

from dotnet-eventsource.

Also, a couple of minor notes on your code there, in terms of how you're treating the event name:

1. I'm not sure it's a good idea to make it case-insensitive as you're doing (StringComparer.OrdinalIgnoreCase). The SSE protocol doesn't have any opinion about the meaning of event names, but there's nothing to prevent an SSE server from using "thing" and "THING" to mean two different things.
2. EventSource already takes care of setting the event name to "message" if none was provided, so your defaulting logic for that in GetTrueEventName is redundant.
3. In fact I don't think you need GetTrueEventName at all, because its only other purpose is to trim whitespace, which again is not a thing the SSE protocol says you should do. There already is built-in trimming in one sense: event:<space>thing is exactly equivalent to event:thing, i.e. a single leading space is ignored. But if the server provides event:<space><space>thing<space> then technically the server is telling you that the event name is "<space>thing<space>" rather than "thing".

from dotnet-eventsource.

I wouldn't pay overly much attention to how the event names are being processed in my message router implementation. These are just implementation details for a very specific case I'm working on, where event names have been communicated to us upfront. The case-insensitivity and trimming is just simply a way to try and recover from mistakes made in input as best as possible. It's good to know, though, that your library takes care of assigning the default "message" event-name...

As to your proposal concerning the message router, I'll give that a try. The only concern I have with this approach, however, is that it makes memory management somewhat more difficult, as there's a risk of the message router and EventSource instances being stored in different parent classes. If this happens, object disposal/garbage collection may not properly take place if the message router is released from memory, but the EventSource is not, since the latter will then continue to hold a reference to the message router in memory by virtue of the event handler. I don't think this is a big issue if both EventSource and message router are stored in the same parent class and all three of them share the same life cycle. But with it being likely that the EventSource outlasts the life cycle of the message router, I see a potential for a memory leak... This is not the case with an inheritance-based implementation...

from dotnet-eventsource.

I don't understand your stated concern there. "If the message router is released from memory, but the EventSource is not "... that isn't a scenario that makes any sense to me, because they would inherently be used together and there would be no reason to want to dispose of the message router while the EventSource is still active. Nor would there be any reason for some other class to be retaining a reference to the message router, since its one and only purpose is to act as a handler. The usage pattern would be just what I showed in my example: create the router, attach the MessageReceived event to it, and you're done. What else would you ever do with it?

Forget about the message router for a moment and just think more generally about a message handler - an event handler that can get attached to the MessageReceived event. You could say the exact same thing about that: if the object that is providing the message handler was being referenced somewhere else, and then you drop that reference, it still won't be garbage-collected because EventSource is holding a reference to it. And... that's correct! That's not a leak— it's exactly how garbage collection is supposed to work. The object stays in memory because, if EventSource wants to fire an event, that is where it'll be fired to, because that's what you told it to do when you set MessageReceived.

In fact, your argument would seem to imply that every use of the event pattern in .NET code is problematic— that event handlers should instead be implemented by subclassing whatever class is sending the event. I don't think this discussion is really leading to any possible improvement in the EventSource library and I'd prefer to let it rest at this point.

from dotnet-eventsource.

The scenario I had in mind is one in which the EventSource is turned into a kind of re-usable service, whereas message routing may be implemented on individual view models (to allow for slight variations in how the data is processed), leading to message routers being connected to the EventSource multiple times.

And while you're right in that, even when all other references to an object but that used as an event handler are released, the object providing the handler will keep on existing, as per language design, not having any other reference to that object does present an issue, since you will never be able to remove the handler again.

Now I agree these scenarios may seem far-fetched, especially where a server-environment is considered. But event handlers are a real concern in mobile development - especially something like Xamarin Forms - where pages (local lingo for views) and their associated view models are frequently released without invocation of the Dispose()-method, leaving a lot of resources tied up without any mechanism to release them. This does lead to memory leaks over time. Hence my emphasis on how event handlers may interact with event sources and other objects, as well as to how memory is released again.

The scenarios, at present, may seem far fetched, and I currently am working on a server-implementation. But I can foresee use of the library in a mobile environment in the future as well, where design requirements are different from those on a server platform.

But, I agree. It may not be worth discussing it at this time.

from dotnet-eventsource.

Again, as you just acknowledged, you are talking about a very general issue with the use of event handlers in a garbage-collected language; I think your issue is more with the .NET platform and C# language than with this library. If you want to build something on top of EventSource to entirely change how its API works, you're welcome to do so, and you can easily do it without subclassing: just create a class X that owns an EventSource instance and sets its MessageReceived event to point to a method of X. Presto, you're done: now if you dispose of your X instance, it can ensure that the EventSource is also disposed of.

That usage pattern may make sense for your application but I don't see reason to think that it's also what other developers will want, so again I think the best choice is to stick with a simple model here and allow people to plug into it whatever they want. And, again, that's basically what every HTTP server framework and every other kind of library out there that has a general message handler model does. If you raised an issue like this for an HTTP server library and suggested that it'd be better to subclass the server than to implement a router separately, I'm pretty sure you would get an identical response.

from dotnet-eventsource.

Just to clear up one possible point of confusion: having a separate message router would not at all make it impossible to release references to other objects further downstream. You would simply call RemoveListener on the router. And once there are no more listeners registered on the router, it really doesn't matter if the router instance continues to exist and is not garbage-collected due to the EventSource still existing, because the router isn't holding any resources other than itself and its dictionary— a tiny amount of storage that is almost exactly equivalent to what your proposed EventSource subclass would be holding anyway. There is no leak there, you're just in effect saying that an active EventSource now uses slightly more memory than it did before, because you have added a new feature to it.

from dotnet-eventsource.