[ECO-5225] Subscribe without attach fix - #258
Conversation
…e underlying channel was not already attached
WalkthroughThis change refactors the subscription management in the default messages module. It replaces a dictionary holding Changes
Sequence Diagram(s)sequenceDiagram
participant Impl as Implementation
participant Store as SubscriptionStore
Impl->>Store: addSubscription(uuid, subscription)
Impl->>Store: resolveSubscriptionStart(for: uuid)
Store->>Store: setSerial(uuid, newSerial)
Note over Impl,Store: Subscription updates occur asynchronously.
Impl->>Store: removeSubscription(uuid) [on termination/channel re-attach]
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/AblyChat/DefaultMessages.swift (1)
83-84: Ensure robust error handling during subscription start
By adding the subscription first (addSubscription) and then attempting to start it (resolveSubscriptionStart), the workflow is logical. However, ifresolveSubscriptionStartfails, consider removing or cleaning up the partially added subscription to avoid inconsistent states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/AblyChat/DefaultMessages.swift(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Generate code coverage (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.2)
🔇 Additional comments (8)
Sources/AblyChat/DefaultMessages.swift (8)
57-57: Good introduction of a shared subscription store
Storing a sharedSubscriptionStoreat the implementation level is a clear, modular approach to centralizing subscription logic. This actor-based design also helps ensure thread safety.
156-158: Proper resource cleanup on termination
Placing the subscription removal in aTaskblock ensures it runs asynchronously without blocking the termination handler. This is a good pattern. Consider logging or handling errors if future store operations become fallible.
207-207: Graceful handling of nonexistent subscriptions
Theguard letstatement ensures that attempting to fetch history for an unregistered subscription results in an error. This is a clear and defensive approach that prevents further complications.
260-262: Bulk reset on channel attach
Looping through all subscriptions to reset their serials is correct for handling non-resumed attaches. The actor-based store is consistently accessed viaawait, ensuring safe concurrency.
270-270: Expanded method signature clarifies usage
Renaming the method toresolveSubscriptionStart(for:)is more descriptive and clearly indicates it operates on a specific subscription.
276-277: Immediate return after setting serial
Once the serial is successfully set, returning early prevents unnecessary code execution. This clean control flow is a good practice for improved readability.
368-372: Well-defined wrapper for subscription and serial
Encapsulating both theMessageSubscriptionand its potentially nilserialin one struct nicely centralizes state. This keeps the code flexible for handling subscriptions that do not yet have an attach serial.
374-395: Actor-based subscription storage
Introducing an actor for subscription storage is a robust solution. It ensures all modifications to thesubscriptionsdictionary are thread-safe. Additional logs when adding/removing subscriptions could provide useful debugging context but are optional.
| try await subscriptionStore.setSerial(uuid: uuid, serial: serialOnChannelAttach()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors from serialOnChannelAttach()
Spawning a separate Task to set the serial is fine, but the thrown errors inside this Task block are not caught. Without additional error handling or logging, failures may be silently ignored and hamper troubleshooting.
Task {
- try await subscriptionStore.setSerial(uuid: uuid, serial: serialOnChannelAttach())
+ do {
+ let attachSerial = try await serialOnChannelAttach()
+ await subscriptionStore.setSerial(uuid: uuid, serial: attachSerial)
+ } catch {
+ logger.log(message: "Failed to set serial: \(error)", level: .error)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
maratal
left a comment
maratal
left a comment
There was a problem hiding this comment.
Those two still fail because getPreviousMessages doesn't pickup attachSerial since it doesn't wait until handleChannelEvents resolves it.
|
@umair-ably ^^ |
|
See #249 (comment) |
|
@umair-ably couldn't you resolve conflicts? |
sure thing, i'll just wrap up the docs needed for the single channel release |
|
@maratal if you're happy with this please can you resolve conflicts and merge? |
…ttachfix # Conflicts: # Sources/AblyChat/DefaultMessages.swift
|
Check / SPM (Xcode 16.1) (pull_request) hanged, so I think it should be merged after #295 resolved @lawrence-forooghian |
|
Fixed in #318 |
4 participants
Fixes an issue in which a Message subscription would not return if the underlying channel was not already attached.
@maratal - this is the fix that seemed to have helped with failing/flaky tests
Summary by CodeRabbit
New Features
Refactor