spring-framework icon indicating copy to clipboard operation
spring-framework copied to clipboard

Add a heartbeat executor for SSE emitters

Open reda-alaoui opened this issue 7 months ago • 2 comments

Fix #33355

reda-alaoui avatar May 11 '25 14:05 reda-alaoui

It makes me thing that emitters should actually opt into heartbeats

If I understand correctly, you'd like each instance of SseEmitter to opt into heartbeats. That means each teammate instantiating an SseEmitter would need to support the burden of configuring it correctly. IMHO, on many applications, forgetting to enable the heartbeat could lead to severe failures (e.g. memory leaks). I think there should be at least a way to have implicit SSE heartbeat activation. Could you share your opinion on that?

reda-alaoui avatar Jun 14 '25 11:06 reda-alaoui

Good question. We could make it opt-in by default so for SseEmitters, but opt out for reactive types adapted to SseEmitter. That said even with reactive types, it would be useful to be able to rely on built-in support for heartbeats. In short, please ignore the last part of my review comment in respect to opting in and out. We'll start with centrally configurable heartbeat support, and see if we need to refine that further.

On that note, I should also add that this would be expected to be configurable through the WebMvc config. I think WebMvcConfigurer#configureAsyncSupport is a good place for an SSE heartbeat duration.

rstoyanchev avatar Jun 16 '25 09:06 rstoyanchev

@rstoyanchev could you take a look at the new changes?

Here are some things I had doubts about while writing the implementation.

Now that SseEmitterHeartbeatExecutor instance is not provided by the outside world (e.g. Spring Boot):

  • I didn't know if there was a way to still register it as a SmartLifecycle instance. Therefore, now the heartbeat task is lazily scheduled on first emitter registration and never canceled (before it was canceled on shutdown).
  • I am not sure how I should require a TaskScheduler implementation. So I chose to fallback to SimpleAsyncTaskScheduler.
  • I thought I should still try to keep the behaviour globally disabled by default to avoid the breaking change labellization. This is why the heartbeat period duration is nullable.

I think there should be more tests to write but I prefer to do that once you tell me I am heading the right way.

reda-alaoui avatar Jun 17 '25 13:06 reda-alaoui

Gentle ping :innocent:

reda-alaoui avatar Jul 07 '25 08:07 reda-alaoui