fix(channels): make subscriptions lifecycle-safe - #4969
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4969 +/- ##
==========================================
+ Coverage 67.32% 67.45% +0.12%
==========================================
Files 293 293
Lines 15246 15328 +82
Branches 1728 1742 +14
==========================================
+ Hits 10265 10339 +74
- Misses 4834 4839 +5
- Partials 147 150 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Vitaly312 <vn264537@gmail.com> Co-authored-by: Artur Shiriev <lesnik512@gmail.com>
515b05b to
c8411c9
Compare
| except BaseException: | ||
| cleanup_task = create_task(self.unsubscribe(subscriber, channels)) | ||
| try: | ||
| await asyncio.gather(asyncio.shield(cleanup_task), return_exceptions=True) | ||
| except CancelledError: | ||
| await asyncio.gather(cleanup_task, return_exceptions=True) | ||
| raise |
There was a problem hiding this comment.
If the backend is unreachable (e.g. network failure):
subscribefor the first subscriber fails, its except block triggersunsubscribe, then_remove_subscriber_remove_subscriberfails,self._channels.setdefault(channel, set()).add(subscriber)in its except block returns the subscriber into the local state- So this call of
subscribewill end by an exception but a subscriber stays in the local state, and next subscriptions will not trigger backend subscription. So, all subsequent events will not be published since the plugin is unsubscribed
I think it's better to just move backend calls before state mutating instead of compensating state mutation on exceptions, it will simplify code.
There was a problem hiding this comment.
@Vitaly312 I agree. I'll make the necessary adjustments.
## Description Calls backend.subscribe/unsubscribe before mutating local state to fix incorrect compensating state mutation, as I described in litestar-org#4969 (comment).
| .. changelog:: 3.0.0 | ||
| :date: 2364-01-27 | ||
|
|
||
| .. change:: Make channel subscriptions lifecycle-safe |
There was a problem hiding this comment.
Can we get a more detailed title and description here? It says what was done mechanically, but doesn't really explain the bug that was fixed
| if history: | ||
| await self.put_subscriber_history(subscriber=subscriber, limit=history, channels=channels) |
There was a problem hiding this comment.
History should be fetched after the state mutation. Otherwise, if the event was published after history fetching but before adding a corresponding subscriber in the self._channels (e.g. when backend.subscribe suspends), this event will be lost since it isn't in the history and _sub_worker will ignore it because the subscriber isn't in the self._channels. I think history should be fetched in the end like it was before (missed that in my PR), but it should roll back if failed:
...
if history:
try:
await self.put_subscriber_history(subscriber=subscriber, limit=history, channels=channels)
except:
await self.unsubscribe(subscriber=subscriber, channels=channels)
raise
return subscriberBut this approach requires some deduplication. I mean, if some event was published after the plugin puts a subscriber in the state, and before history fetching, this event will be consumed twice. I guess this requires changing abstract ChannelsBackend.stream_events signature to provide some timestamp/ID for each event to distinguish between identical events and duplicates (or just change subscribe signature to not handle history fetching at all/require that consumers must be idempotent).
Description
ChannelsPluginupdates subscriber state and backend subscriptions without shared lifecycle synchronization. Concurrent transitions can unsubscribe active subscribers, while failures can leak subscribers and dynamic channel entries.This change:
This consolidates the applicable fixes from #4895, #4872, and #4868 while crediting their authors.
Closes #4894
Closes #4871
Closes #4867
The Redis stream cursor behavior noted in #4867 remains unchanged.
📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/4969