[ECO-4963] Clean up when user finishes with subscription #207
Conversation
lawrence-forooghian
force-pushed
the
36-clean-up-subscriptions
branch
from
d54309a to
4fe6f4e
Compare
lawrence-forooghian
force-pushed
the
36-clean-up-subscriptions
branch
from
4fe6f4e to
87afc1c
Compare
I never really understood how the AsyncStream type, which this wraps, is a struct; it seems to behave like a reference type; for example: - if you start two iterations, one over the original value and one over a copy, then each element is received by precisely one of the iterations; - it seems to call its terminationHandler when the last reference to all of its copies goes away So, change these wrapper types to a reference type so that users can reason about them more easily, and to make it easy for me to add stored properties to them without having to worry about how to fit them into the reference-ish semantics (which I will do in an upcoming commit).
lawrence-forooghian
force-pushed
the
36-clean-up-subscriptions
branch
from
87afc1c to
2554e1a
Compare
WalkthroughThe pull request introduces a comprehensive refactoring of subscription management across the AblyChat library. The primary focus is on transitioning from mutable arrays of subscriptions to a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
lawrence-forooghian
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (18)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
6-6: Consider adding error handling for emit operationsWhile the transition to
SubscriptionStorageimproves thread safety, consider handling potential errors during emit operations, especially in an async context.private var discontinuitySubscriptions = SubscriptionStorage<DiscontinuityEvent>() internal func emitDiscontinuity(_ discontinuity: DiscontinuityEvent) { + do { discontinuitySubscriptions.emit(discontinuity) + } catch { + logger.error("Failed to emit discontinuity: \(error)") + } }Also applies to: 16-16, 20-20
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1)
Line range hint
110-117: Consider adding termination handler for consistencyWhile the implementation is correct, consider adding a termination handler to match the behavior in
DefaultRoomLifecycleContributorChannel.func subscribeToState() -> Subscription<ARTChannelStateChange> { let subscription = subscriptions.create(bufferingPolicy: .unbounded) + subscription.addTerminationHandler { [weak self] in + // Cleanup mock state if needed + } switch subscribeToStateBehavior { case .justAddSubscription: breakSources/AblyChat/RoomLifecycleManager.swift (1)
1235-1235: Consider adding error handling for status change wait event.While the emit call for status change wait event is correctly placed, consider adding error handling to gracefully handle potential failures in the event emission, especially since this occurs during presence operation validation.
- statusChangeWaitEventSubscriptions.emit(.init()) + do { + statusChangeWaitEventSubscriptions.emit(.init()) + } catch { + logger.log(message: "Failed to emit status change wait event: \(error)", level: .debug) + }Sources/AblyChat/Subscription.swift (1)
55-56: Consider using concurrency-safe constructs instead ofNSLockUsing
NSLockfor synchronizing access toterminationHandlersmay not align with Swift's concurrency model. Consider using anactorto manageterminationHandlersfor thread-safe access, which can provide cleaner code and integrate better with Swift's async features.Sources/AblyChat/SubscriptionStorage.swift (2)
6-8: Consider using an actor instead of manual synchronization.The comment explains using
@unchecked Sendablewith manual synchronization instead of actors. While the current implementation works, using an actor would provide safer concurrency guarantees and eliminate the need for manual locking.Consider refactoring to use an actor:
-internal class SubscriptionStorage<Element: Sendable>: @unchecked Sendable { +internal actor SubscriptionStorage<Element: Sendable> {This would:
- Remove the need for manual lock management
- Provide compile-time concurrency safety
- Better align with Swift's modern concurrency model
25-28: UsewithLockfor safer lock handling.The current lock/unlock pattern could lead to issues if an exception occurs between lock and unlock. Using
withLockis safer as it ensures the lock is always released.- lock.lock() - subscriptions[id] = .init(subscription: subscription) - lock.unlock() + lock.withLock { + subscriptions[id] = .init(subscription: subscription) + }Also applies to: 43-46
Tests/AblyChatTests/SubscriptionStorageTests.swift (2)
6-17: Add concurrent emission test.The current test verifies basic emission, but doesn't test concurrent emission scenarios which are important for a thread-safe storage implementation.
Consider adding a test like:
@Test func concurrentEmit() async throws { let storage = SubscriptionStorage<Int>() let subscription = storage.create(bufferingPolicy: .unbounded) // Emit concurrently from multiple tasks async let emissions = (0..<100).map { value in Task { storage.emit(value) } }.map { await $0.value } // Collect emitted values var received: Set<Int> = [] for await value in subscription.prefix(100) { received.insert(value) } // Verify all values were received exactly once #expect(received.count == 100) #expect(Set(0..<100) == received) }
19-39: Add timeout to termination test.The test waits indefinitely for termination. Consider adding a timeout to prevent test hangs if termination fails.
- await subscriptionTerminatedSignal.stream.first { _ in true } + let terminationReceived = await subscriptionTerminatedSignal.stream.first { _ in true } + #expect(terminationReceived != nil, "Termination signal not received within timeout")Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (2)
3-4: Remove comment about copying and make mock implementation independent.The comment indicates this is copied from the main implementation. Instead, the mock should be an independent implementation focusing on testing needs.
Consider:
- Removing the copy comment
- Simplifying the mock implementation to focus on testing requirements
- Adding mock-specific features like emission tracking
36-41: Add emission tracking to mock.The mock could be more useful for testing by tracking emissions.
+ private(set) var emittedElements: [Element] = [] + private let emittedElementsLock = NSLock() + func emit(_ element: Element) { + emittedElementsLock.withLock { + emittedElements.append(element) + } for subscription in subscriptions.values { subscription.subscription?.emit(element) } }Tests/AblyChatTests/SubscriptionTests.swift (1)
40-55: Add error handling test for iteration task.The test verifies cancellation but doesn't test error propagation through the iteration task.
Consider adding:
@Test func addTerminationHandler_terminationHandlerCalledWhenIterationTaskThrows() async throws { let onTerminationCalled = AsyncStream<Void>.makeStream() let subscription = Subscription<Void>(bufferingPolicy: .unbounded) subscription.addTerminationHandler { onTerminationCalled.continuation.yield() } let iterationTask = Task { for await _ in subscription { throw SomeError() } } await onTerminationCalled.stream.first { _ in true } #expect(try await iterationTask.value, throws: SomeError.self) }Sources/AblyChat/DefaultRoomReactions.swift (1)
85-87: Consider using consistent cleanup method across files.While the implementation is correct, it uses
unsubscribewhileDefaultOccupancyusesoff. Consider standardizing the cleanup method across the codebase for better maintainability.- self?.channel.unsubscribe(eventListener) + self?.channel.off(eventListener)Sources/AblyChat/Messages.swift (1)
195-199: LGTM! Consider documenting termination behavior.The change from struct to final class with Sendable conformance is appropriate for subscription lifecycle management.
Consider adding documentation about the termination behavior, particularly when the subscription will be terminated and what cleanup users can expect.
README.md (2)
221-221: Add comma for better readability and update variable name.The documentation effectively explains the discontinuity handling, but there are minor improvements needed:
- Add a comma after "Here" for better readability
- Consider renaming
errortodiscontinuityEventin the code example to match the subscription typeApply this diff:
-Here you can create a subscription that will emit a discontinuity event +Here, you can create a subscription that will emit a discontinuity event-for await error in subscription { - print("Recovering from the error: \(error.error)") +for await discontinuityEvent in subscription { + print("Recovering from the error: \(discontinuityEvent.error)")Also applies to: 228-229
🧰 Tools
🪛 LanguageTool
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...K provides anonDiscontinuitymethod. Here you can create a subscription that will...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
106-401: Excellent consistency in subscription pattern implementation!The documentation demonstrates a well-thought-out and consistent approach to subscription management across all features:
- Uniform use of AsyncSequence for all subscriptions
- Consistent pattern for subscription creation and iteration
- Clear lifecycle management for proper cleanup
This consistency will make the API more intuitive and easier to maintain.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~219-~219: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nection, continuity cannot be guaranteed and you'll need to take steps to recover me...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...K provides anonDiscontinuitymethod. Here you can create a subscription that will...(AI_HYDRA_LEO_MISSING_COMMA)
Example/AblyChatExample/Mocks/MockClients.swift (2)
198-198: Consider consolidating duplicate typing eventsThe typing implementation emits events in multiple places. Consider consolidating the event emission logic into a single helper method.
actor MockTyping: Typing { + private func emitTypingEvent(currentlyTyping: [String]) { + mockSubscriptions.emit(TypingEvent(currentlyTyping: currentlyTyping)) + } + func start() async throws { - mockSubscriptions.emit(TypingEvent(currentlyTyping: [clientID])) + emitTypingEvent(currentlyTyping: [clientID]) } func stop() async throws { - mockSubscriptions.emit(TypingEvent(currentlyTyping: [])) + emitTypingEvent(currentlyTyping: []) }Also applies to: 207-214, 224-224, 228-228
240-240: Consider consolidating presence event emission logicSimilar to the typing events, the presence implementation has duplicate event emission code. Consider extracting a helper method.
actor MockPresence: Presence { + private func emitPresenceEvent(action: PresenceAction, data: PresenceData?) { + mockSubscriptions.emit( + PresenceEvent( + action: action, + clientID: clientID, + timestamp: Date(), + data: data + ) + ) + } + private func enter(dataForEvent: PresenceData?) async throws { - mockSubscriptions.emit( - PresenceEvent( - action: .enter, - clientID: clientID, - timestamp: Date(), - data: dataForEvent - ) - ) + emitPresenceEvent(action: .enter, data: dataForEvent) }Also applies to: 295-302, 314-321, 333-340
Sources/AblyChat/DefaultPresence.swift (1)
Line range hint
221-239: Consider optimizing the array iterationWhile the implementation is correct, the array iteration could be simplified:
subscription.addTerminationHandler { [weak self] in - for eventListener in eventListeners { - if let eventListener { - self?.channel.presence.unsubscribe(eventListener) - } - } + eventListeners.compactMap { $0 }.forEach { listener in + self?.channel.presence.unsubscribe(listener) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Example/AblyChatExample/Mocks/MockClients.swift(17 hunks)Example/AblyChatExample/Mocks/MockSubscription.swift(2 hunks)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift(1 hunks)README.md(7 hunks)Sources/AblyChat/DefaultConnection.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultOccupancy.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(2 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(3 hunks)Sources/AblyChat/DefaultRoomReactions.swift(2 hunks)Sources/AblyChat/DefaultTyping.swift(2 hunks)Sources/AblyChat/Messages.swift(2 hunks)Sources/AblyChat/RoomLifecycleManager.swift(11 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Sources/AblyChat/Subscription.swift(3 hunks)Sources/AblyChat/SubscriptionStorage.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)Tests/AblyChatTests/SubscriptionStorageTests.swift(1 hunks)Tests/AblyChatTests/SubscriptionTests.swift(1 hunks)
🧰 Additional context used
📓 Learnings (2)
Example/AblyChatExample/Mocks/MockSubscription.swift (1)
Learnt from: lawrence-forooghian
PR: ably/ably-chat-swift#182
File: Example/AblyChatExample/Mocks/MockSubscription.swift:23-23
Timestamp: 2024-12-05T13:15:59.085Z
Learning: In Swift, the default buffering policy for `AsyncStream.makeStream(of:)` is `.unbounded`, so specifying it explicitly is unnecessary.
Example/AblyChatExample/Mocks/MockClients.swift (1)
Learnt from: lawrence-forooghian
PR: ably/ably-chat-swift#182
File: Example/AblyChatExample/Mocks/MockSubscription.swift:23-23
Timestamp: 2024-12-05T13:15:59.085Z
Learning: In Swift, the default buffering policy for `AsyncStream.makeStream(of:)` is `.unbounded`, so specifying it explicitly is unnecessary.
🪛 LanguageTool
README.md
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...K provides an onDiscontinuity method. Here you can create a subscription that will...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
🔇 Additional comments (29)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
55-58: Good implementation of subscription cleanup!The termination handler properly manages memory by:
- Using weak reference to avoid retain cycles
- Ensuring cleanup of Ably channel listeners
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
6-6: LGTM! Thread-safe implementation with actorThe implementation correctly:
- Uses actor for thread safety
- Maintains consistency with core implementation
- Properly manages subscriptions using SubscriptionStorage
Also applies to: 18-18, 22-22
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
11-11: LGTM! Consistent implementation across mocksThe implementation maintains consistency with other mocks and properly handles thread safety through actor usage.
Also applies to: 47-47, 51-51
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1)
11-11: LGTM! Proper state managementThe implementation correctly manages state and maintains thread safety through actor usage.
Also applies to: 123-123
Sources/AblyChat/Rooms.swift (1)
141-141: Great refactoring to useSubscriptionStorage!The change from array-based storage to
SubscriptionStorageimproves thread safety and encapsulation of subscription management. The use ofcreateandemitmethods provides a cleaner API compared to manual array operations.Also applies to: 145-145, 150-150
Sources/AblyChat/RoomLifecycleManager.swift (2)
90-90: Well-structured subscription management refactoring!The transition from array-based storage to
SubscriptionStoragefor room status changes improves thread safety and provides a more maintainable subscription lifecycle management.Also applies to: 332-332, 359-359
337-337: Consistent refactoring of debug subscriptions!The debug-only subscription management has been uniformly updated to use
SubscriptionStorage. This maintains consistency with the production code changes and provides the same benefits of improved thread safety and cleaner subscription management for testing scenarios.Also applies to: 346-346, 372-372, 385-385, 405-405, 413-413, 612-612, 616-616, 1262-1262, 1266-1266
Sources/AblyChat/DefaultConnection.swift (2)
Line range hint
33-97: Appropriate use of termination handler to manage event listenerThe implementation correctly adds a termination handler to remove the
eventListenerwhen thesubscriptionis terminated. This ensures that the listener is properly cleaned up, preventing potential memory leaks and unintended behavior.
98-100: Ensure weak capturing ofeventListenerto avoid retain cyclesIn the termination handler,
eventListeneris captured strongly. SinceeventListenerholds a closure that capturesselfweakly, there is a potential for a retain cycle if not managed carefully. Ensure thateventListenerdoes not prevent deallocation ofDefaultConnection.Sources/AblyChat/Subscription.swift (1)
14-14: ChangingSubscriptionfrom a struct to a class alters semanticsTransitioning
Subscriptionfrom astructto afinal classchanges its behavior from value semantics to reference semantics. This could affect how subscriptions are managed and copied throughout the codebase. Verify that all existing usages ofSubscriptionare compatible with this change and do not introduce unintended side effects.Example/AblyChatExample/Mocks/MockSubscription.swift (2)
5-5: Transition from struct to class changesMockSubscriptionbehaviorChanging
MockSubscriptionfrom astructto afinal classalters its behavior to reference semantics. Ensure that this change is intentional and that all test cases and mock scenarios correctly handleMockSubscriptionas a reference type.
31-35:setOnTerminationmethod enhances mock functionalityThe addition of
setOnTerminationallows for custom actions upon termination of theMockSubscription, improving the ability to simulate and test termination behavior in mocks.Sources/AblyChat/DefaultOccupancy.swift (1)
25-27: LGTM! Clean subscription management implementation.The changes properly handle subscription cleanup by storing the event listener and adding a termination handler with appropriate memory management using weak self.
Also applies to: 46-50
Sources/AblyChat/Messages.swift (1)
226-228: LGTM! Clean termination handler implementation.The termination handler implementation properly forwards to the underlying subscription.
Sources/AblyChat/DefaultTyping.swift (2)
Line range hint
28-75: LGTM! Well-implemented retry mechanism with exponential backoff.The implementation includes:
- Proper error handling and logging
- Exponential backoff with jitter for retries
- Maximum retry duration of 30 seconds
- Weak reference handling to prevent retain cycles
76-81: LGTM! Clean subscription termination handling.The termination handler properly cleans up resources by:
- Using weak references to prevent retain cycles
- Unsubscribing from presence events when the subscription is terminated
README.md (6)
106-106: LGTM! Clear documentation of the new subscription-based approach.The updated example clearly demonstrates how to use the new
AsyncSequence-based subscription pattern for connection status changes.
206-206: LGTM! Clear documentation of room status subscription.The example effectively shows how to use the new subscription model for room status updates.
273-274: LGTM! Clear documentation of message history retrieval.The documentation clearly explains the new method for retrieving message history for subscribed listeners.
336-336: LGTM! Clear documentation of presence subscription.The example effectively demonstrates how to use the new subscription model for presence updates.
386-386: LGTM! Clear documentation of typing subscription.The example clearly shows how to use the new subscription model for typing updates.
401-401: LGTM! Clear documentation of occupancy subscription.The example effectively demonstrates how to use the new subscription model for occupancy updates.
Example/AblyChatExample/Mocks/MockClients.swift (4)
67-67: LGTM! Clean transition to MockSubscriptionStorageThe change from array-based storage to
MockSubscriptionStorageimproves thread safety and subscription management.Also applies to: 78-81
93-93: LGTM! Well-structured message subscription implementationThe implementation correctly generates random messages with appropriate fields and intervals.
Also applies to: 102-113
137-137: LGTM! Simplified event emissionThe change from iterating over subscriptions to using the storage's emit method reduces complexity.
151-151: LGTM! Improved reaction event generationGood use of random intervals (0.1...0.5) for realistic reaction simulation.
Also applies to: 160-169
Sources/AblyChat/DefaultPresence.swift (1)
202-214: Well-implemented subscription cleanup!Good use of weak references to prevent retain cycles, and proper cleanup of the event listener when the subscription is terminated.
Sources/AblyChat/DefaultMessages.swift (2)
Line range hint
55-104: LGTM! Consistent with the subscription pattern.The introduction of the eventListener variable enables proper cleanup in the termination handler, maintaining consistency with the pattern used across the codebase.
106-117: Excellent implementation of thread-safe cleanup!The termination handler properly:
- Ensures thread safety using MainActor
- Cleans up both the event listener and internal tracking
- Prevents retain cycles with weak self
Hook in to AsyncStream’s termination notification mechanism, so that when the user discards a subscription or cancels the task in which they’re iterating over a subscription, we: - remove this subscription from our internal data structures - remove any corresponding ably-cocoa listeners that drive this subscription I’m sure that there will turn out to be a bunch of wrong stuff that I’ve done here, due to my still-shaky knowledge of concurrency stuff and AsyncSequence best practices, but it’s a start. Resolves #36.
lawrence-forooghian
force-pushed
the
36-clean-up-subscriptions
branch
from
2554e1a to
9bf7331
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/AblyChat/Subscription.swift (1)
93-98:⚠️ Potential issueFix potential race condition in termination handler execution.
Capturing
terminationHandlersintoconstantTerminationHandlersbefore settingonTerminationmay lead to missed handlers that are added after settingonTermination.Apply this fix to ensure all handlers are executed:
- let constantTerminationHandlers = terminationHandlers continuation.onTermination = { _ in + self.lock.lock() + let handlers = self.terminationHandlers + self.lock.unlock() for terminationHandler in handlers { terminationHandler() } }
🧹 Nitpick comments (5)
Sources/AblyChat/DefaultConnection.swift (1)
Line range hint
33-97: Consider refactoring the event listener for better maintainability.The event listener implementation correctly handles all states but could benefit from being broken down into smaller, focused methods to improve readability and testability.
Consider extracting the following into separate methods:
- Timer management logic
- State transition handling
- Status change emission
let eventListener = realtime.connection.on { [weak self] stateChange in guard let self else { return } + Task { await self.handleConnectionStateChange(stateChange, subscription) } } +private func handleConnectionStateChange( + _ stateChange: ARTConnectionStateChange, + _ subscription: Subscription<ConnectionStatusChange> +) async { + let statusChange = createStatusChange(from: stateChange) + + if await shouldHandleTransientDisconnect(statusChange) { + await handleTransientDisconnect(statusChange, subscription) + return + } + + await emitStatusChange(statusChange, subscription) +}README.md (1)
221-221: Add missing comma for better readability.-Each feature of the Chat SDK provides an `onDiscontinuity` method. Here you can create a subscription that will emit a discontinuity event on its `AsyncSequence` interface whenever a discontinuity in that feature has been observed. +Each feature of the Chat SDK provides an `onDiscontinuity` method. Here, you can create a subscription that will emit a discontinuity event on its `AsyncSequence` interface whenever a discontinuity in that feature has been observed.🧰 Tools
🪛 LanguageTool
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...K provides anonDiscontinuitymethod. Here you can create a subscription that will...(AI_HYDRA_LEO_MISSING_COMMA)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
55-58: LGTM! Good cleanup implementation with proper memory managementThe termination handler correctly cleans up the event listener and uses weak references to avoid retain cycles. Consider adding a debug log statement in the termination handler to aid in debugging subscription lifecycle issues.
subscription.addTerminationHandler { [weak underlyingChannel] in + logger.debug("Cleaning up state subscription for channel: \(String(describing: underlyingChannel))") underlyingChannel?.unsubscribe(eventListener) }Sources/AblyChat/DefaultMessages.swift (1)
Line range hint
55-104: Consider extracting message parsing logicThe message handling logic is quite complex with multiple guard statements and data validations. Consider extracting this into a separate method for better maintainability and testability.
let eventListener = channel.subscribe(RealtimeMessageName.chatMessage.rawValue) { message in Task { - // TODO: Revisit errors thrown as part of https://github.com/ably-labs/ably-chat-swift/issues/32 - guard let ablyCocoaData = message.data, - let data = JSONValue(ablyCocoaData: ablyCocoaData).objectValue, - let text = data["text"]?.stringValue - else { - throw ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without data or text") - } - // ... rest of the parsing logic ... + let message = try await parseIncomingMessage(message) messageSubscription.emit(message) } } +private func parseIncomingMessage(_ message: ARTMessage) async throws -> Message { + guard let ablyCocoaData = message.data, + let data = JSONValue(ablyCocoaData: ablyCocoaData).objectValue, + let text = data["text"]?.stringValue + else { + throw ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without data or text") + } + // ... rest of the parsing logic ... + return Message( + serial: serial, + action: action, + clientID: clientID, + roomID: self.roomID, + text: text, + createdAt: message.timestamp, + metadata: metadata ?? .init(), + headers: headers ?? .init() + ) +}Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1)
Line range hint
110-117: Consider Using a Bounded Buffering Policy to Prevent Memory GrowthThe use of
.unboundedbuffering policy insubscriptions.create(bufferingPolicy: .unbounded)could lead to uncontrolled memory consumption if events are produced faster than they are consumed. To mitigate potential memory issues, consider specifying a bounded buffering policy with an appropriate limit.Apply this change to specify a bounded buffering policy:
let subscription = subscriptions.create( - bufferingPolicy: .unbounded + bufferingPolicy: .bufferingOldest(limit: yourDesiredLimit) )Replace
yourDesiredLimitwith a suitable buffer size based on expected workload and resource constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Example/AblyChatExample/Mocks/MockClients.swift(17 hunks)Example/AblyChatExample/Mocks/MockSubscription.swift(2 hunks)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift(1 hunks)README.md(7 hunks)Sources/AblyChat/DefaultConnection.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultOccupancy.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(2 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(3 hunks)Sources/AblyChat/DefaultRoomReactions.swift(2 hunks)Sources/AblyChat/DefaultTyping.swift(2 hunks)Sources/AblyChat/Messages.swift(2 hunks)Sources/AblyChat/RoomLifecycleManager.swift(11 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Sources/AblyChat/Subscription.swift(3 hunks)Sources/AblyChat/SubscriptionStorage.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)Tests/AblyChatTests/SubscriptionStorageTests.swift(1 hunks)Tests/AblyChatTests/SubscriptionTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- Sources/AblyChat/DefaultTyping.swift
- Example/AblyChatExample/Mocks/MockSubscription.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Sources/AblyChat/DefaultPresence.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Sources/AblyChat/Rooms.swift
- Sources/AblyChat/SubscriptionStorage.swift
- Tests/AblyChatTests/SubscriptionStorageTests.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/RoomLifecycleManager.swift
🧰 Additional context used
📓 Learnings (1)
Example/AblyChatExample/Mocks/MockClients.swift (1)
Learnt from: lawrence-forooghian
PR: ably/ably-chat-swift#182
File: Example/AblyChatExample/Mocks/MockSubscription.swift:23-23
Timestamp: 2024-12-05T13:15:59.085Z
Learning: In Swift, the default buffering policy for `AsyncStream.makeStream(of:)` is `.unbounded`, so specifying it explicitly is unnecessary.
🪛 LanguageTool
README.md
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...K provides an onDiscontinuity method. Here you can create a subscription that will...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (14)
Sources/AblyChat/DefaultConnection.swift (2)
98-100: LGTM! Clean termination handler implementation.The termination handler correctly manages cleanup by removing the event listener and prevents memory leaks with weak self.
Line range hint
106-120: Well-designed actor for thread-safe state management.The
ConnectionStatusManageractor effectively ensures thread-safe access to connection state while maintaining a clean API.README.md (1)
Line range hint
106-401: Documentation updates are comprehensive and well-structured.The README changes effectively communicate the new subscription-based API with clear examples and consistent formatting throughout.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~219-~219: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nection, continuity cannot be guaranteed and you'll need to take steps to recover me...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...K provides anonDiscontinuitymethod. Here you can create a subscription that will...(AI_HYDRA_LEO_MISSING_COMMA)
Tests/AblyChatTests/SubscriptionTests.swift (2)
25-38: LGTM! Well-structured test for subscription cleanup.The test effectively verifies that termination handlers are called when a subscription is discarded by:
- Creating a subscription in a local scope
- Adding a termination handler
- Letting the subscription go out of scope
- Verifying the handler is called
40-55: LGTM! Comprehensive test for task cancellation.The test thoroughly verifies that termination handlers are called when an iteration task is cancelled by:
- Creating a subscription
- Adding a termination handler
- Creating and cancelling an iteration task
- Verifying the handler is called
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
11-11: LGTM! Improved subscription management.The transition from array-based storage to
SubscriptionStorageenhances:
- Thread safety through encapsulated subscription management
- Memory management by automating subscription cleanup
- Code maintainability by reducing boilerplate
Also applies to: 47-47, 51-51
Sources/AblyChat/Messages.swift (1)
196-199: LGTM! Enhanced MessageSubscription with termination support.The changes improve the subscription lifecycle management:
- Clear documentation about single iteration requirement
- Consistent with the broader refactoring of subscription handling
- Thread-safe termination handler support
Also applies to: 226-228
Example/AblyChatExample/Mocks/MockClients.swift (2)
67-67: LGTM! Consistent use of MockSubscriptionStorage across mocks.The changes improve the mock implementations by:
- Encapsulating subscription management
- Ensuring thread-safe event emission
- Reducing code duplication
- Maintaining consistent patterns across all mock types
Also applies to: 78-78, 93-93, 102-102, 151-151, 160-160, 198-198, 207-207, 240-240, 248-248, 361-361, 370-370
137-137: LGTM! Simplified event emission.The event emission has been streamlined using the
MockSubscriptionStorage.emitmethod, making the code more maintainable and less error-prone.Also applies to: 181-181, 224-224, 228-228, 295-302, 314-321, 333-340
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (2)
6-6: LGTM! Good transition to SubscriptionStorageThe change from a mutable array to
SubscriptionStorageimproves thread safety and subscription lifecycle management.
16-16: LGTM! Clean implementation using SubscriptionStorage APIThe changes simplify the code by leveraging
SubscriptionStorage's methods for emission and subscription creation, improving thread safety and maintainability.Also applies to: 20-20
Sources/AblyChat/DefaultMessages.swift (1)
106-116: LGTM! Well-implemented termination handler with proper actor isolationThe termination handler correctly:
- Runs on MainActor for thread safety
- Uses weak self to prevent retain cycles
- Cleans up both the event listener and subscription tracking
However, let's verify that all subscription points are properly cleaned up across the codebase.
✅ Verification successful
✅ Subscription points cleanup verified - no issues found
The verification confirms that all subscription points are properly cleaned up. The implementation is well-encapsulated within
DefaultMessages.swiftwith matching declaration and cleanup points.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential subscription point leaks by finding all subscriptionPoints declarations # and ensuring they have corresponding cleanup code echo "Searching for subscriptionPoints declarations..." rg "subscriptionPoints.*=.*\[.*\]" -A 5 echo "Verifying cleanup in termination handlers..." rg "subscriptionPoints\.removeValue" -B 5Length of output: 1359
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (2)
11-11: Adoption ofSubscriptionStorageImproves Subscription HandlingThe change from an array of subscriptions to
SubscriptionStorage<ARTChannelStateChange>enhances thread safety and automates the cleanup process of subscriptions. This refactoring aligns with the overall goal of streamlining subscription management.
123-123: Consistent Event Emission withsubscriptions.emitUpdating
emitStateChangeto usesubscriptions.emit(stateChange)ensures that all subscribers receive state change notifications consistently through theSubscriptionStorage. This modification maintains alignment with the new subscription management system.
Hook in to
AsyncStream’s termination notification mechanism, so that when the user discards a subscription or cancels the task in which they’re iterating over a subscription, we:subscription
I’m sure that there will turn out to be a bunch of wrong stuff that I’ve done here, due to my still-shaky knowledge of concurrency stuff and
AsyncSequencebest practices, but it’s a start.(I still need to update the documentation in https://github.com/ably/docs/ to reflect the removal of
unsubscribe.)Resolves #36.
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
SubscriptionStoragefor more efficient subscription tracking.Improvements
Technical Changes
Testing