Fix panic in pubsub by deleting subs in callback #1101
Conversation
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant PubSub
participant Subscription
PubSub->>Subscription: Unsubscribe request
alt Subscription exists and is empty
PubSub->>PubSub: Delete subscription from map
PubSub->>Subscription: Close subscription
else Subscription not empty or doesn't exist
PubSub->>PubSub: No action taken
end
The sequence diagram illustrates the refined unsubscription logic, showing how the method now checks the subscription's status before performing any deletion or closure actions. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)server/backend/sync/memory/pubsub.go (1)
Fix incorrect condition in subscription cleanup logic There's a critical logic error in the deletion condition. The current implementation will never delete empty subscriptions because:
This effectively prevents cleanup of empty subscription maps, potentially causing memory leaks. Apply this fix to correct the logic: m.subscriptionsMap.Delete(docKey, func(subs *Subscriptions, exists bool) bool {
- if !exists || 0 < subs.Len() {
if !exists || subs.Len() > 0 {
return false
}
subs.Close()
return true
})Let's verify the subscription cleanup behavior: Would you like me to help create test cases to verify:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1101 /- ##
==========================================
- Coverage 46.84% 46.82% -0.03%
==========================================
Files 84 84
Lines 12256 12257 1
==========================================
- Hits 5741 5739 -2
- Misses 5939 8631 2
- Partials 576 577 1 ☔ View full report in Codecov by Sentry. |
What this PR does / why we need it:
Fix panic in pubsub by deleting subs in callback
This PR addresses a panic error that occurred after the introduction of a concurrency map in the pubsub by modifying the deletion process of subscriptions. Now, instead of causing a panic,
subsare deleted within the callback to ensure thread safety and prevent race conditions.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Bug Fixes
Chores