-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feedback #4
Comments
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.
|
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. |
@Lanayx, with your custom constructor, how do you manage scenarios where the same user (same |
@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 |
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 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 |
@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? |
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. |
@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. |
@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 |
@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 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 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 Let me clear my previous comment. From |
@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 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 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. |
@tpeczek Right, so if I send my GroupId over with the 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 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 |
@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. |
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 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 |
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. |
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. |
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 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. |
@tpeczek How is it possible to gracefully close the connection from the server? |
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. |
Thanks @tpeczek. |
@alexiwonder That 204 status code is a tricky thing in Server-Sent Events. The correct flow to use it goes like this:
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 |
Hi, @tpeczek |
Hi @Luigi6821, There is an extensibility point in the library which will most likely be suitable for your need - Client Identifier Provider. It allows you to implement a class which will be available for providing client id based on There is a little sample available here: https://tpeczek.github.io/Lib.AspNetCore.ServerSentEvents/articles/disconneting-clients.html#client-identifier-provider The one potential issue I can see here are errors if it happens that client attempts to establish multiple parallel connections with the same |
Hi, @tpeczek Is not present in http context of customized ClientIdProvider I am sure I am doing something wrong. Thanks again |
Hi @Luigi6821, It looks like you might be confusing endpoints. Any controller endpoint you may have in your application is not related to any SSE endpoint you might have. Requests are also separated. This means that you must properly define the route when you are registering your SSE endpoint: public class Startup
{
...
public void Configure(IApplicationBuilder app, IHostEnvironment env)
{
...
app
...
.UseRouting()
.UseEndpoints(endpoints =>
{
endpoints.MapServerSentEvents("/sse-notifications/{appKey}");
...
});
}
} You also need to make sure that the client side code is providing the value while creating var source = new EventSource("/sse-notifications/app-key"); |
Hi @tpeczek , Regards |
Hi @tpeczek, In this way The OnConnected SSE service event receives a QueryString containing supplied appKey and also the client related to it. |
The |
Yes, this frontend change is in general correct - you need to provide appKey from model to EventSource constructor. You can do this with query string, but the approach with using OnConnected is a little bit hacky. I would suggest route base approach with client identifier provider. But, ultimately, both will work so you can choose the one which suits you better. |
Is there a way to run this when there's no default authentication scheme? I'm on an App that still doesn't use the standard authentication mechanism in ASP.NET Core.. so when a client tries to connect to the SSE endpoint, it gets tripped up in the System.InvalidOperationException: No authenticationScheme was specified, and there was no DefaultChallengeScheme found. The default schemes can be set using either AddAuthentication(string defaultScheme) or AddAuthentication(Action configureOptions). The authorization code being custom, I need |
The That said, I wouldn't advise using |
I don't plan to do this but as a stopgap solution. The old mechanism with pushing data over a Chunked Channel (in a way, rather similar to SSE - but not standardizes) doesn't seem to work anymore in ASP.NET Core, the three attempts I made to fix all failed, so I figured since I'm already using your lib in two other products, why not go down that route. It's for dev clients only in a dev environment. Prod Clients won't use SSE for now - it's just that swapping out the custom (10+ year old) auth scheme is more a 'in a sprint' undertaking, than a 'in a few hours'.. |
Understood. In that case I would suggest one more approach to consider (as |
Do you have an example I could build upon for that middleware? Might be worth a look. |
This post of mine should get you started. It goes too far for your needs (because it drags in https://www.tpeczek.com/2019/01/aspnet-core-middleware-and-authorization.html |
well, tried with your first approach. Added my own IAuthorizationService too.. so that calling await authorizationService.AuthorizeAsync return Success. Yet, calling the /sse-endpoint now returns in a 404 My Startup.Cs
If I set |
Ok, some thoughts on this snippet. If you are defining a policy (it seems that you do), you shouldn't be needing an additional middleware. This should be enough:
If you want an additional middleware, you should register Server-Sent Events in that branched pipeline without authorization.
Picking one of those most likely will get you the right result (unless there is some additional important context not present in the snippet). |
I tried the second approach first and it works :) |
That sounds good, just triple check everything because you know - security ;) |
Hi. Why the library in nuget is not strongly named? |
Hi @Droni, The honest answer is that there was never a compelling reason to do so. In case of .NET Core and .NET, strong-naming has no benefits - the runtime never validates the strong-name signature and doesn't use the strong-name for assembly binding. The only remaining case where there is a potential value is ASP.NET Core 2.1 on .NET Framework, but there are also drawbacks and usage for that specific case was never high enough to justify them. |
Hm. Thanks for your reply. I was currently using your library in my project which uses strong names and I got a compilation error. To solve this problem, various ways are proposed, including decompiling and rebuilding the library. But I think it would be better to use strong names from the very beginning :) P.S. I use .Net 8 |
An interesting question to ask would be why the project is requiring strong-naming. In case of .NET Framework strong-naming was enabling relying on assemblies in GAC and avoiding assemblies conflicts, but in .NET there is none of that. I could start signing the library but, as it is considered a breaking change, I would need a compelling reason to do that. This is the first time when the subject has been even raised. |
All commercial products use a signature. This is more to show that the library is not fake and clearly indicates its ownership. For example, when Microsoft started using NewtonSoft.Json, they couldn't do it if the library wasn't signed. If you look into the future and if you want your development to become more popular, then sooner or later you will have to think about it. |
Well that's not entirely true (but mostly was back in .NET Framework days). First of all, strong naming doesn't indicate ownership, it just creates an identity. In fact the Microsoft recommendation on strong naming is CONSIDER not DO. At the same time the recommendation is that if you do, you should keep the key in source control so maintainers can recompile with the same one. Also, as I previously written, strong naming has no value in case of .NET Core and .NET and can be easily suppressed. This is coming straight from Open-Source Library Guidance : https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming And from experience, strong naming in current days is a consideration only for projects which have legacy ties to .NET Framework. If such projects will start looking for broader adoption of this library, or there will be specific project with a compelling need, I will be happy to consider that change. |
is there a particular reason why we can modify the reconnect interval on Also, is there a way to disable SSE again at runtime? I figure use |
Hi @ssteiner, If you are asking for a capability to modify keepalive after the endpoint has been spin up, than it is not supported because it would have strong consequences on the hosted service which is performing the job (it essentially would have to be reinitialised with different setup). From broader perspective, keepalives are an infrastructure concept - they are there to mitigate any intermediaries you may have configured along the path, so there shouldn't be a need to change that configuration at runtime. Regarding the second question, I'm not sure I fully understand what you are trying to achieve there and not sure how can I guide you. Removing endpoints while application is working sounds unusual, so while disconnecting clients is supported, removing the endpoint is not backed in any way. |
Here's what I'm trying to do: I'd like to be able to turn on/off the SSE functionality at runtime. So, initially, SSE is off, meaning the sse endpoint uri would return 404. Then, somebody decides we want to use SSE now (which we use for dynamic GUI updates), so they turn it on (at runtime), and from that point on, the sse endpoint uri would be accessible. So I tried something like this
But, regardless of the the value of My thinking was: why spin up something that isn't being used. |
Technically you may be able to achieve what you want, but it's completely outside of the scope of the library internals. Instead of using There also may be unforeseen consequences you will have to resolve. |
Okay, I guess it's not mean to be used like that so I'll revert to the static activation/configuration/deactivation at startup. |
This issue is for general feedback regarding this project.
The text was updated successfully, but these errors were encountered: