feat(sse): add ping_interval for keepalive pings#4623
feat(sse): add ping_interval for keepalive pings#4623TanayK07 wants to merge 12 commits intolitestar-org:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4623 +/- ##
==========================================
+ Coverage 67.33% 67.36% +0.02%
==========================================
Files 292 292
Lines 14941 14990 +49
Branches 1676 1684 +8
==========================================
+ Hits 10061 10098 +37
- Misses 4743 4756 +13
+ Partials 137 136 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey @provinzkraut @euri10 — friendly ping! This PR has been open for ~10 days. CI is green on all SSE tests (5527 passed) — the only failures are 3 Postgres channel pub/sub timeouts which also fail on main branch (unrelated flaky tests). This incorporates all the review feedback from the stalled #4098:
Would love a review when you get a chance. Happy to make any changes needed. Thanks! |
|
Just pushed a few improvements based on self-review:
CI is green across 3.9–3.13 — the one red check is a Postgres channel timeout that also happens on main (not related to SSE). @provinzkraut @sobolevn would really appreciate a look when you have a moment — happy to adjust anything. |
|
Update : Codecov/patch is now green (was the last remaining check). All 27 CI checks pass. This PR is ready for |
Add optional `ping_interval` parameter to `ServerSentEvent` that sends SSE comment keepalive pings (`: ping`) at the specified interval to prevent connection timeouts from reverse proxies or clients. Closes litestar-org#4082
- Fix RST heading level for "Keepalive Pings" section (~~~ → ^^^) - Remove dead else branch in _send() to achieve 100% patch coverage
Remove the "Keepalive Pings" subsection heading that caused a "Title level inconsistent" RST error. The content is integrated directly into the "Server Sent Event Responses" section instead.
- Remove dead callable iterator check in to_asgi_response (SSE iterator is always _ServerSentEventIterator, never a callable) - Add test for str chunk handling in ASGIStreamingSSEResponse to cover both branches of the bytes/str encoding path
- Use proper ASGI Message/HTTPDisconnectEvent types for mock functions - Add isinstance narrowing for message body to satisfy mypy - Add from __future__ import annotations and TYPE_CHECKING imports
With from __future__ import annotations, string-quoting type hints is redundant. Ruff format removes them automatically.
…or ping_interval - Validate ping_interval > 0 (reject zero/negative to prevent tight loops) - Delegate to parent to_asgi_response when ping_interval is None (cleaner backward compat) - Add __all__ export to sse.py for consistency with streaming.py - Add tests: validation errors, empty generator shutdown, large interval no-ping - Docs: clarify units (seconds), typical values, sleep-first behavior
Fixes ruff TC001 lint error — ASGIResponse is only used in a type annotation, not at runtime.
f8ac564 to
7c1ec86
Compare
cofin
left a comment
There was a problem hiding this comment.
Thanks for the PR. I added a couple of minor comments, but it otherwise looks good.
- Replace assert guards with conditional raises in _send and _ping - Reorder docstring args to list request first - Add :pr: 4623 to changelog entry
Thanks, I have addressed the comments, made the updates and added 3 small tests for codecov coverage. Whom else can I tag to review such that we can merge this? Appreciate it @cofin |
Description
Add optional
ping_intervalparameter toServerSentEventthat sends SSE comment keepalive pings at the specified interval to prevent connection timeouts from reverse proxies or clients.Closes #4082
Approach
This PR incorporates all reviewer feedback from the stalled #4098:
SSEStreamclass — overridesto_asgi_response()directly onServerSentEventASGIStreamingSSEResponselives insse.py, notstreaming.py— keeps SSE concerns out of generic streaming: ping\r\n\r\n), notevent: ping— comments are invisible to JSEventSourceclientsanyio.Lockonsend()— ping and stream tasks run concurrently, lock prevents response corruptionanyio.Eventto signal shutdown — clean task cancellation when stream endsUsage
This is fully opt-in —
ping_intervaldefaults toNone, so existing behavior is unchanged.Changes
litestar/response/sse.pyASGIStreamingSSEResponse, addping_intervaltoServerSentEvent, overrideto_asgi_response()tests/unit/test_response/test_sse.pydocs/usage/responses.rstdocs/release-notes/changelog.rstTest Plan
ping_intervalworks unchanged (regression test)ping_intervalsends keepalive comments during idle periods: ping), not events📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/4623