-
Notifications
You must be signed in to change notification settings - Fork 38.5k
Add a heartbeat executor for SSE emitters #34878
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Réda Housni Alaoui <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting together a pull request. Some comments below.
I don't think there needs to be a public SseEmitterHeartbeatExecutor
contract and implementation. This could be simply a TaskScheduler
with the rest managed internally in a dedicated package private class (similar to ReactiveTypeHandler
).
Heartbeats should not be emitted if emitters are already busy sending. Perhaps each emitter can keep a timestamp of the last send, and expose a method to send a heartbeat that would send only if a certain amount of time has passed since the last send.
Controllers that use Flux
for SSE can easily incorporate heartbeats. Such Flux
return values are also turned into SseEmitter
s, and so adding heartbeats to every emitter you could end up with duplicate heartbeat streams for the same emitter. It makes me think that emitters should actually opt into heartbeats, and then they should be called to send a heartbeat only if they opted in.
If I understand correctly, you'd like each instance of |
Good question. We could make it opt-in by default so for On that note, I should also add that this would be expected to be configurable through the WebMvc config. I think |
Fix #33355