Add a heartbeat executor for SSE emitters

[reda-alaoui]

Fix #33355

[rstoyanchev]

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 SseEmitters, 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.

[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?

[rstoyanchev]

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.

[reda-alaoui]

@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]

Gentle ping 😇