Lib.AspNetCore.ServerSentEvents icon indicating copy to clipboard operation
Lib.AspNetCore.ServerSentEvents copied to clipboard

Feedback

Open tpeczek opened this issue 8 years ago • 66 comments

This issue is for general feedback regarding this project.

tpeczek avatar Aug 06 '17 06:08 tpeczek

Hi! First of all want to thank you for this great library. I'm pretty happy with it's functionality, there are just several things that I missed to use it as a nuget package.

  1. Logging. I'd like to configure logging not only on connect/disconnect events, but on send events as well. Also added Description field in ServerSentEventsClient for logging purpose as well, since Id is not enough
  2. More flexible client initialization. In my case I want to use string instead of Guid, and take it from user's Claims after authentication, not just doing Guid.NewGuid(). String is more flexible because many people use numbers as UserId or other techniques, or generics could be used there as well. Here is how I modified the constructor:
internal ServerSentEventsClient(HttpContext context)
{
    User = context.User ?? throw new ArgumentNullException(nameof(context.User));
    Id = User.Claims.First(claim => claim.Type == JwtRegisteredClaimNames.Sub).Value;
    Description = context.Request.Headers["User-Agent"];
    _response = context.Response ?? throw new ArgumentNullException(nameof(context.Response));
    IsConnected = true;
}

Lanayx avatar Mar 04 '19 08:03 Lanayx

Hi @Lanayx

I'm happy you find this useful regardles if you use it through NuGet or your own fork :).

Thank you for your feedback. I'll give some thinking to your remarks. My intial thought is that those things best fit outside the library, but maybe after further consideration I'll see a nice place for them.

tpeczek avatar Mar 04 '19 09:03 tpeczek

... In my case I want to use string instead of Guid, and take it from user's Claims after authentication, not just doing Guid.NewGuid(). ...

@Lanayx, with your custom constructor, how do you manage scenarios where the same user (same sub claim from your JWT) opens multiple, simultaneous connections? Did you also make an adjustment to the ConcurrentDictionary keys in ServerSentEventsService to support multiple, simultaneous connections?

seniorquico avatar Mar 18 '19 16:03 seniorquico

@seniorquico thanks for raising the question. In my case only one last connection is used (last active tab), but I guess this should be configurable and one may want to have notifications for each tab

Lanayx avatar Mar 20 '19 19:03 Lanayx

I'm new-ish to this library and SSE. I'm wanting to create more of a real-time collaboration app where a group of users subscribe to a Redis pub/sub channel. I wouldn't want to send a Publish event to every user; only certain subsets at a time (each user subscribes to a document ID basically).

Not sure if I could tackle that with this library. I'm trying to figure out if I'd need dynamically mapped SSE events (outside of the .MapServerSentEvents<NotificationsServerSentEventsService>() or if i'd need to do something else.

Currently looking through the code, but any advice in how to use/extend your library for that would be helpful. Or if it really wasn't meant to do this.

EDIT: Ahh, maybe a better option might be to create NotificationsService which holds a dictionary of ServerSentEventsServices and create a new one for each document session. Then just pick the right SseEventsService from the dictionary when I need to publish. Based on a short review of your articles/source code.

henry-chris avatar Mar 27 '19 08:03 henry-chris

@henry-chris My gut tells that under you scenario hides a requirement for some kind of groups functionality, which would be a nice thing to add.

But that will take time, so first I would like to try help you with hat library currently provides. Can you elaborate on how subscribing to different documents happens? Different URLs?

tpeczek avatar Mar 28 '19 18:03 tpeczek

In our application, we allow clients to subscribe to/unsubscribe from many topics/groups at any time. We initially thought about exploring how to wire up the middleware with a parameterized URL to identify the topic/group identifier. That would've allowed clients to subscribe and unsubscribe by simply starting and stopping different EventSources. However, after some initial testing, we found a significant number of our clients still connecting with user agents that don't support HTTP/2 (which unlocks multiplexed requests and responses). As such, we chose to find a solution that worked with a single EventSource.

We created a controller that exposes CRUD-based methods for a client's topic/group subscriptions (simply because SSE provides only a simplex comm channel after the initial setup). The controller uses all of the normal ASP.NET tools to apply auth checks/permissions. A separate backend application is used to keep track of the client's topic/group subscriptions in addition to which ASP.NET Core server the client is connected. When any of the microservices in our application push an event to a topic/group, the event is relayed to the appropriate ASP.NET Core server(s) and on to the appropriate client(s).

The backend application is built on Microsoft Orleans, which already provides full pub/sub event streaming infrastructure. Our ASP.NET Core servers subscribe to a single Orleans stream identified by a random, server-specific GUID. We model each client as an actor. When the controller method is called to subscribe to a topic/group, the actor subscribes to the associated Orleans stream. When the actor receives an event, it simply relays the message to the stream identified by the server-specific GUID. (I'm actively working on releasing this implementation as an open source project on GitHub.)

The idea of managing subscriptions via a CRUD controller could be adapted to an in-memory dictionary or Redis storage. However, our stream topology may not be a great fit for Redis streams. You could look at the Redis backplane for SignalR for some implementation inspiration.

seniorquico avatar Mar 28 '19 19:03 seniorquico

@tpeczek I have been looking at this off and on and "Yes" it seems to need some type of group functionality. I'm still working out the details, but we handle several different storage repositories and the underlying document metadata is different in most of them. Best I can tell though, the ID would end up being a concatenation of two GUIDs that would be supplied from the client/browser as either a number or a string (doesn't really matter which). I could not use straight document URLs, but they could be part of the ID.

Everytime a user opens our app we would load up the document and an accompanying 'Markup' document in the '.xfdf' format. This is basically a PDF/Office document markup service. At the same time we would register an EventSource to either a single Web API endpoint or a parameter-ized one.

We would send the GROUP ID along with email and/or any other data needed. This would be cached in redis in some way (as a Set/List under the GROUP ID maybe).

Then when a user adds/modifies/deletes any markups a "Command" is sent to the server which contains the diff + operation of the markups. We would send the GROUP ID with it. We'd then want to sent the SSE out to only that specific group and send the "Command" as the message.

So, I was thinking each GROUP ID would be a redis channel with associated pub/sub. The problem seems to be generating the redis channels dynamically and in the correct way. I took a little time to toy with that but didn't get very far, yet.

@seniorquico Thanks for the input. Yes, a single EventSource seems to be the way to go. I was looking into building a shared service/web worker, also, as some of our customers have multiple tabs open (So we don't hit the ~6 connection limit).

I honestly haven't seen Orleans. I will be sure to take a look at it. Also, I'm hoping not to use SignalR if possible. Just a personal preference.

henry-chris avatar Mar 28 '19 22:03 henry-chris

@seniorquico Looks like you have put together a nice piece of software, really impresive.

@henry-chris From SSE perspective it looks like you shoudl be able to build what you need based on ClientConnected event. The ServerSentEventsClientConnectedArgs provides a HttpRequest for the connecting user so you can use it to apply any criteria you need and build your own grouping on top of that. From that point it boils down to sending to correct group (which of course is a separate issue).

tpeczek avatar Apr 01 '19 08:04 tpeczek

@tpeczek I'm not sure how to parse your comment. Building a group is not really a problem. The problem is how to only send specific events to specific groups using your package (which I think you alluded to at the end?).

I'm currently looking into this:

Seems like the main thing that would need to be re-written is the ForAllClientsAsync method. Instead of doing it for all clients, have a separate method that takes a list of clients to send. This seems like it would need the least change for now as only my service and the `SseKeepaliveService' may need to be modified. I'd just need to keep a mapping of Group->Clients to parse all clients of a group on a certain server.

As you stated maybe I could use the ClientConnected/ClientDisconnected events to add/remove from my mappings and register a single other service which keeps those mappings and can be registered through DI.

May have holes in my logic here. This also requires rewriting some internal functions/classes. I'm also on Azure, so I'll likely need the KeepAliveService.

If I didn't re-write any internal code I can't see a good way to use the library. It would almost be easier to write new services/middleware.

The If & How of possibly working some of this into the library is another matter. I may be going about it the wrong way.

henry-chris avatar Apr 02 '19 16:04 henry-chris

@henry-chris Let me clear my previous comment. From ClientConnected events you have access to IServerSentEventsClient and HttpRequest. Managing groups means maintaining a set of IServerSentEventsClient collections, where each collection represents a different a group. When you need to send to a specific group you just iterate a specific collection and call IServerSentEventsClient.SendEventAsync for all clients in that group.

tpeczek avatar Apr 02 '19 17:04 tpeczek

@henry-chris It sounds like you understand the need to build your own grouping infrastructure and some possible implementation ideas. Great! Next, expose a method to get the list of client identifiers that belong to a target group (maybe by some "IGroupManager" that's also in your DI container). Then, use something like the following extension methods to distribute an event to the needed clients:

public static Task SendEventAsync(this IServerSentEventsService service, string text, IEnumerable<Guid> clientIds) =>
    service.SendEventAsync(text, clientIds, CancellationToken.None);

public static Task SendEventAsync(this IServerSentEventsService service, string text, IEnumerable<Guid> clientIds, CancellationToken cancellationToken) =>
    Task.WhenAll(
        clientIds
            .Select(clientId => service.GetClient(clientId))
            .Where(client => client != null)
            .Select(client =>client.SendEventAsync(data, cancellationToken)),
        cancellationToken);

Simply get the needed services from the DI container and call the extension method:

var service = services.GetService<IServerSentEventsService>();
var groupManager = services.GetService<IGroupManager>();

IEnumerable<Guid> clientIds = groupManager.GetClientsInGroup(...);
service.SendEventAsync("group message", clientIds);

No changes to the library are necessary for the above. (But, they could be added to the library or even directly to the IServerSentEventsService API as an enhancement.) If you're sending events at a super high-frequency, the dictionary lookups in GetClient could become a problem (but I'd test to first confirm). If that's the case, then you may want to go "lower level". Instead of looking up the client from a list of Guid, just save lists of clients (the IServerSentEventsClient instances). Then, you don't even need the above extension methods:

var groupManager = services.GetService<IGroupManager>();

IEnumerable<IServerSentEventsClient> clients = groupManager.GetClientsInGroup(...);
Task.WhenAll(clients.Select(client => client.SendEventAsync("group message")));

However, all of the above is good only for distributing events to clients connected directly to the local server. If you live in a single-server environment, then you're good to go. If you're in a multi-server environment, then that's where Redis (or some other messaging infrastructure) can come into play. Start with a non-optimized solution:

Each ASP.NET Core server subscribes to a single Redis pub/sub channel at startup (use a singleton service). When an ASP.NET Core server receives a message from the Redis channel, relay it as an event to the IServerSentEventsService. When you want to send an event, instead of using IServerSentEventsService directly, publish it as a message to the Redis channel. (The message should include the target client identifier or target group identifier.)

After that's working, look into optimizations that fit your usage patterns. Again, I'd recommend looking into how SignalR leverages Redis as a backplane for inspiration. (I wasn't suggesting above that you use Redis 😄.) The basic premise is close to the above, but they get fancy with multiple channels.

seniorquico avatar Apr 02 '19 17:04 seniorquico

@tpeczek Right, so if I send my GroupId over with the HttpRequest I can catch both the Client and Group with the Connected Event. That's what I'm thinking at least. After that I can either setup my own service/classes to send events through OR Fork your library and build it into it. From your responses, it's looking like the former is the best option up front.

I had questions about the KeepAlive service (I didn't want to spawn a bunch of them), but looking over the code it seems that only a Singleton is spawned.

That seems to be the only real caveat as nothing else is actually a background task.

henry-chris avatar Apr 02 '19 18:04 henry-chris

@henry-chris Please go through @seniorquico comment. It's a very detailed description of what I had in mind. It gives you the exact functionality you need. Of course it would be much better if that would be built in the library and I've created an issue for it (#20), but as my resources are limited it will take some time.

Regarding KeepAlive, that shouldn't cause any issues. You don't need multiple IServerSentEventsService (most likely one will do) so there will also be only one ServerSentEventsKeepaliveService. Also ServerSentEventsKeepaliveService is not something you manage by yourself, it's managed under the hood by library based on the options you pass to AddServerSentEvents.

tpeczek avatar Apr 02 '19 18:04 tpeczek

@tpeczek, @seniorquico Thanks, yes my trouble was finding the right way to go about this. I wanted to make sure I wasn't spawning a bunch of Background services that weren't needed or would cause overhead. At first I was just trying to debug through the library to see where/how I could hook into. It does look like @seniorquico has a very simple solution that I did not see right away. I will focus on that.

Yes I'm on a distributed App Service in Azure. Just starting to use Redis to deal with this specific use case. I was hoping to stay away from more abstractions like SignalR until I learned more about the internals of how everything works at a base level.

I'll let you know how it goes.

henry-chris avatar Apr 02 '19 19:04 henry-chris

Just as an update. Everything is working well. I went with a similar approach to what you two suggested. Working well with Redis over 3-4 server nodes with document specific groupings.

I've forked the repo locally and have been making small changes to integrate groups a little more. It'll probably be awhile, but I'll at least give you a link when it's done (if you are interested)

I've just got to figure out how to integrate OIDC IdentityServerAuthentication middleware for Authorization against the open connection stream. Haven't messed with that much yet, but it looks like I can build a PolicyEvaluator of some sort.

Thanks for the help.

henry-chris avatar Apr 25 '19 02:04 henry-chris

@henry-chris Happy to hear that things are working out for you. Please share your code when it will be ready - it might be useful for me.

Regarding authorization, you are not the first with this question and I have an answer for you here: https://www.tpeczek.com/2019/01/aspnet-core-middleware-and-authorization.html

tpeczek avatar Apr 25 '19 18:04 tpeczek

I do have one other question.

What is the reason that this library primarily uses middleware for the notifcation routing? I suppose you couldn't have a NuGet package that has built in controllers.

I ask because it seems that I am adding multiple middlewares which are evaluated during every request, whereas a controller with a variable endpoint could (I believe) spawn the long-running SseClients just as well. It would also only evaluate authorization if a route/controller was hit, as well as being much easier to tie into the built in Authorization.

I could be wrong though.

henry-chris avatar Apr 26 '19 04:04 henry-chris

@henry-chris

Although possible, handling things like SSE in controller would be incorrect from framework philosophy/architecture perspective. Controller are ment to handle standard request/response flow, preferable in API and view scenrios (the view scenerio is slowly becoming less important with grow of Razor Page and SPA). Middlewares have been introduced to give flexibility for other scenarios like WebSockets (and SSE is much more like WebSockets than API) as they have already proven this capability in Node.js.

Controllers are often seen as first place to implement something, because MVC is nicely coupled with routing, authentication, authroization etc. The ASP.NET Core team has noticed htat issue and in result as part of 3.0 those mechanism will be decoupled from MVC and put in front of middleware (Endpoint Routing) to easy scenarios like the one being context for the post I've gave you.

Now I'm not sure what you mean by "multiple middlewares which are evaluated during every request", but if you have set up everything correctly (SSE has its own path and you are branching pipeline) everything should run only when needed.

tpeczek avatar Apr 26 '19 09:04 tpeczek

@henry-chris

One more thign regardign authorization. I've directed you to that blog post with assumption that you need something very specific. But this library has built in support for typical authorization scenarios. in case it happened that you've missed it, here is the relevant part of documentation: https://tpeczek.github.io/Lib.AspNetCore.ServerSentEvents/articles/authorization.html

tpeczek avatar Apr 26 '19 09:04 tpeczek

@tpeczek Understood. I have seen SSE and Websockets implemented via controllers as a jump off point, so naturally I'm asking. I do see the benefit in Middleware over API approach here, but since it's less supported it seems easier to just easier to take the API route. I don't see much actual downside, though I'm no expert. Good to know that 3.0 will change this somewhat.

I was unaware of the ability to branch the pipeline. I expected all middleware to be ran through on each request. I will look more deeply into that, thanks.

Yes, I've checked the blog post and the html article. Sadly, we use header based JWT Bearer tokens which makes it more difficult to use SSE. Native implementations do not support custom headers, but some polyfills do. I'm attempting to test performance/reliability of some polyfills now and am working out the best approach to take depending on those results.

henry-chris avatar Apr 26 '19 10:04 henry-chris

@tpeczek How is it possible to gracefully close the connection from the server? Thanks.

alexiwonder avatar Aug 28 '20 14:08 alexiwonder

Hi @alexiwonder,

This hs been raised before in #27.

TL;DR SSE philosophy doesn't forsee such an operation as server is supposed to be a never ending stream of evetns, but if you want you can create a "logical" operation for close.

tpeczek avatar Aug 30 '20 19:08 tpeczek

Thanks @tpeczek. Before I raised the question, I was wondering how sending 204 No-Content to signal the client stop re-connecting is ever possible in ASP.Net Core when a connection is already established. I gave it a a go and found that it's not doable to change the http status code once the first response has been flushed. (As I'm new to .Net Core, can you please confirm?). So yes! the best bet is to implement a logical operation for close.

alexiwonder avatar Aug 31 '20 06:08 alexiwonder

@alexiwonder That 204 status code is a tricky thing in Server-Sent Events. The correct flow to use it goes like this:

  • Client connects and receives events
  • Server decides to disconnect the client and terminates the connection
  • Due to auto-reconnect client automatically makes connection the server
  • Server responds with 204 which prevents client from further attempts to reconnect

So this is a form of a disconnecting mechanic, but it requires a logical mechanism to detect that disconnected client is attempting to reconnect.

I was thinking some time ago about trying to build some API for it, but the more I thought about the more complicated it was becoming (how to identify clients in a way to information is retained for long enough but not too long so client can connect later etc.)

For future reference I've created an issue to track that the idea exists: #39

tpeczek avatar Sep 01 '20 07:09 tpeczek

@tpeczek Brilliant answer! Thanks.

alexiwonder avatar Sep 02 '20 08:09 alexiwonder

@tpeczek , thanks. As I see, interaction with the client is carried out through HttpResponse, in addition, there is no mechanism for identifying connections. This is probably not a problem if we are talking about using HTTP/1.1, in which a new TCP connection will be created for each request. If we talk about HTTP/2, then a multiplexing mechanism is available in it, that is, within one client, only one TCP connection will be created through which data can be received and sent.

And here for me the situation becomes somewhat incomprehensible. If communication takes place over a single TCP connection, do we need to store all HttpResponse instances? Or it is enough for us to store only one of them identifying them by using a token (instead of Guid.NewGuid()) that is unique within the "session".

0UserName avatar Feb 07 '21 10:02 0UserName

@0UserName The first important thing, which should help to untangle things a little bit here, is that TCP and HTTP are different layer protocols. TCP is Layer 4 (Transport) while HTTP is Layer 7 (Application). This, on very high level, allows you to be ignorant of how HTTP uses TCP, if you are delivering business functionality on top of HTTP (like Server Sent Events). Of course when you are dealing with specific scenarios you care about that knowledge, but I'm digressing here.

So, when we are talking about a "connection" in context of Server Sent Events, we are talking about single HTTP request/response interaction (SSE in the end is just a never ending HTTP response). So a single HttpResponse represents a single client and a single connection.

Regarding #39, I have it on my "hope to do" list for February/March (the intent is to deliver general mechanism and single affinity provider - probably cookie based) but I can't promise anything. Unfortunately last year left me less flexibility when it comes to putting tie in my free side projects :(.

tpeczek avatar Feb 09 '21 16:02 tpeczek

Hi and happy new year! First thanks for that library. Would it be possible to send a message to specific client?

jmdavid avatar Dec 30 '21 17:12 jmdavid

Hi @jmdavid and happy new year to you as well!

In general, yes it's possible: https://tpeczek.github.io/Lib.AspNetCore.ServerSentEvents/articles/getting-started.html#sending-to-specific-client. Do you have any specific scenario which gives you an issue?

tpeczek avatar Dec 30 '21 18:12 tpeczek