tpeczek/Lib.AspNetCore.ServerSentEvents

Last-Event-ID and disconnection not optimized for some use cases

JesperTreetop opened this issue · 5 comments

I wanted to build an SSE endpoint which takes a query string parameter to specify an ID of a stream of information and then, once that stream has ended, replied with 204 as is customary to signal that the client should stop reconnecting.

This library sort of fights you here. Wanting to be able to disconnect requires implementing a client ID provider that maps to and from a client Guid. And there's no way to have logic at a very early stage that can pick out parameters and return an error if it's wrong or 204 if everything was right, but the stream has ended.

To make the example more concrete:

client: GET /follow-stream?id=42
server: checks parameter and gets stream information from somewhere
if parameter error: sends error code
if valid: starts stream, which is sent until stream ends, at which point the response can be disconnected
if stream has ended: sends status 204

This way of using SSE does not require keeping track of a client's session, and handles the reconnection properly.

I think it's possible that hashing the URL of the endpoint into a Guid in some way might let you map it to a "session", even if it makes it correspond to everyone's session to that endpoint with that parameter. Closing everyone's connection to the same resource wouldn't be the worst thing in this case, but doing so by aborting the connection could be troublesome timing-wise, if 1000 connected sessions are about to send out the final message on the stream, because some of them might be cut off in the middle of or before writing it. Not to mention that if this mapping assumes there will only be one session but there are 1000 sessions listening to the same endpoint, there will be trouble.

I would propose a patch but it appears to go against the grain of many of the decisions that have already been made. My ideal solution for this type of endpoint (which is all I have to write for this scenario, but may not be what everyone else has to write), would look more or less like mapping an endpoint in recent ASP.NET Core APIs. You provide an async method which gets the context, can check query string parameters, pull in services and so on. It could also get the Last Event ID, which would be null for an initial connection. Then you can either reply with status code for a validation error or 204 to signal that this thing is done, or you can continue with sending the events. And then you could also close the connection. In my case I loop through an IAsyncEnumerable of events to relay, so for me it can all be fit into one place.

Because of how far this seems from the pattern used in the samples, I may end up basically doing a fork, taking only the things I need. But I still think it would be a good idea to be able to handle disconnection in a different way, as I don't think the type of endpoint I describe is that much out of the ordinary.

Hi @JesperTreetop,

If I've understood your need correctly, it doesn't stand against what is there currently in the library. For me it sounds more like missing functionality.

What you are describing sounds to me like a stream level filter or a "validity check". Strictly technically speaking this logic could be exposed by a possibility of registering an optional DI service, which would be used to perfoming this logic after AuthorizeAsync in the middleware

if (!await AuthorizeAsync(context, policyEvaluator))
{
return;
}
Guid clientId = _serverSentEventsClientIdProvider.AcquireClientId(context);

Let me know if I'm interpreting this correctly and if yes I think such functionality could be added.

That's basically it, at least a big part of it. That's being able to 204 a connection attempt (be it a reconnection or not). There are a few other shards of functionality, like being able to end the original request in a controlled way without having the client ID/Guid mapping logic. I may attempt a patch and pull request to fit these bits in now.

I thought the current model for how Disconnect works on the IServerSentEventsClient would be incompatible - if everyone could call Disconnect without having a client ID/Guid mapping, that would be a break and possible issue to people who wanted the current session-like behavior. But on the other hand it blocks the simpler handling of Disconnect.

Made a first attempt at a patch in #50 including perfunctory tests. It hooks into the place you requested in middleware but there are many other details that could differ.

Thank you :). I might not be able to review this before weekend, but I'll try to do my best.

No need to hurry on my account. With that change, I have something that works for my use case for the time being (and if it doesn't I might update the pull request).