Stream resumability support #947
Conversation
…reamableHttpHandler
…round job in sample app
Refactored `EventStoreCleanupService` to use a new `IEventStoreCleaner` interface, separating cleanup responsibilities from the `IEventStore` interface. - Introduced `IEventStoreCleaner` with a `CleanEventStore` method. - Updated `InMemoryEventStore` to implement `IEventStoreCleaner`. - Removed `CleanupEventStore` from `IEventStore`. - Registered `InMemoryEventStore` as both `IEventStore` and `IEventStoreCleaner` in DI. - Updated logging messages to reflect the new terminology. This change improves modularity and adheres to the Single Responsibility Principle.
There was a problem hiding this comment.
Thanks for contributing a PR to add resumability support!!
We have an issue tracking this at #510 and plan on adding client-side support for this too. I think resumability will likely get a lot more use in the future, especially if modelcontextprotocol/modelcontextprotocol#1699 is adopted which allows the server to ask the client to poll.
I know other SDK's have some support for this already, and so far, all of them have left the InMemoryEventStore as a sample which is certainly the easiest thing to do from the SDK's perpective, but I think it'd be nice for application developers if we were able to provide something that is production-ready shipped as part of the SDK. Perhaps something based on MemoryCache. I'm curious what @stephentoub thoughts are on that.
I think I'm going to take over this issue, but I do like your overall design. I'll be sure to consider your design and follow it where it makes sense while adding test coverage.
I'm curious why you split string? GetEventId(string streamId, JsonRpcMessage message) and StoreEvent(string streamId, SseItem<JsonRpcMessage?> messageItem). Why not just ValueTask<string> StoreEvent(string streamId, JsonRpcMessage messagge)? I also wonder if there's any way we could bind the eventId to the current user ID, so that even if the eventId was stolen by an otherwise unauthenticated attacker, they would be unable to exfiltrate messages via the resumability feature.
If there's a clean way we could leverage existing interfaces / concepts, like utilizing IDistributedCache, that'd be my preference, as then we already have a plethora of implementations available in the ecosystem (including in-memory) and we wouldn't need the concrete implementations in the SDK. |
@halter73 That's a good point. The reason I split those 2 method is because I am only storing selective events (messages that are of type And given the way current code changes are filtering which events to store in EventStore, I would either need to have those 2 separate methods or need to pass the pending client request Id to event store (I didn't wanted to do that). |
4 participants
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context