bigcyy commented on PR #4116:
URL: https://github.com/apache/hertzbeat/pull/4116#issuecomment-4237942505
Thanks for the contribution.
I don't think this PR is needed for the current log SSE scenario.
The frontend already uses `EventSource.onopen` / `onerror` to maintain the
connection status, so sending an extra
`CONNECTED` event from the backend does not add much value here. Also, the
frontend currently only listens for
`LOG_EVENT`; this new `CONNECTED` event is not consumed by the log stream
page.
There is also a lifecycle concern: this `emitter.send(CONNECTED...)` is
called inside `createEmitter()`, before the
controller returns the `SseEmitter` to Spring MVC. At that point Spring
may not have initialized the underlying
response handler yet, so the event can be queued as an early send attempt
rather than actually written immediately. If
the initial event fails later during emitter initialization, the `catch`
block in `createEmitter()` may not catch it,
and cleanup may not happen as expected.
For these reasons, I suggest not adding this initial `CONNECTED` event. If
we need a stronger business-level
subscription acknowledgement in the future, we should design it together
with frontend consumption and proper emitter
lifecycle/error handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
