Add dispose() method to ChatClient for resource cleanup#477
Open
Add dispose() method to ChatClient for resource cleanup#477
Conversation
WalkthroughAdds an explicit async disposal API: Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Client as ChatClient
participant Rooms as Rooms
participant Room as Room
participant Features as RoomFeatures
participant Conn as DefaultConnection
App->>Client: dispose() async
activate Client
Client->>Rooms: dispose()
activate Rooms
Rooms->>Rooms: set isDisposed = true
Rooms->>Room: release() (concurrent) for each tracked room
activate Room
Room->>Features: dispose()
activate Features
Features->>Features: cancel timers / remove listeners / clear caches
deactivate Features
Room->>Room: releaseChannel()
deactivate Room
Rooms->>Rooms: clear roomStates
deactivate Rooms
Client->>Conn: dispose()
activate Conn
Conn->>Conn: cancel timers
Conn->>Conn: unregister eventListeners
deactivate Conn
Client->>Client: mark isDisposed
deactivate Client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/477/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/477/AblyChat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Sources/AblyChat/DefaultConnection.swift`:
- Around line 7-12: The DefaultConnection class mutates eventListeners and
isDisposed and accesses timerManager from an Ably callback that runs off the
main thread, causing a data race; mark the class DefaultConnection with
`@MainActor` to isolate its mutable state and, inside the Ably
realtime.connection.on callback, explicitly hop to the main actor before
referencing self (or its properties) so all accesses to eventListeners,
isDisposed, and timerManager occur on the main actor.
In `@Sources/AblyChat/TypingTimerManager.swift`:
- Around line 89-99: The dispose() method lacks the required spec annotation;
update the method comment above internal func dispose() to include the
appropriate `@spec` tag (e.g., // `@spec` CHA-RL3g) to reference the Chat SDK
features spec for disposal behavior, ensuring you add the annotation alongside
the existing descriptive comment and keep references to cancelHeartbeatTimer(),
whoIsTypingTimers, and timer.cancelTimer() to make the behavior traceable.
AndyTWF
force-pushed
the
dispose-method
branch
from
a6b725e to
61c1ec5
Compare
AndyTWF
added a commit
to ably/docs
that referenced
this pull request
Feb 2, 2026
Add new section documenting ChatClient.dispose() method for JavaScript, React, Swift, Kotlin, and Jetpack Compose based on: - ably/ably-chat-swift#477 - ably/ably-chat-kotlin#188
github-actions
bot
temporarily deployed
to
staging/pull/477/AblyChat
AndyTWF
force-pushed
the
dispose-method
branch
from
61c1ec5 to
e5ef282
Compare
github-actions
bot
temporarily deployed
to
staging/pull/477/AblyChat
AndyTWF
force-pushed
the
dispose-method
branch
from
e5ef282 to
e6f0db4
Compare
Contributor
Author
|
Addressed CodeRabbit's review comments:
|
github-actions
bot
temporarily deployed
to
staging/pull/477/AblyChat
Contributor
Author
|
(CI failing is unrelated to this change that we need to look into) |
Implements CHA-CL1 spec: adds dispose() method that releases all resources held by the ChatClient instance. - ChatClient.dispose() releases all rooms (CHA-CL1a) then disposes connection (CHA-CL1b) - DefaultRooms.dispose() releases all managed rooms concurrently - DefaultConnection.dispose() removes all event listeners - All dispose methods are idempotent - Added MockChatClient.dispose() for example app compatibility - Mark DefaultConnection as @mainactor and wrap Ably callback for thread safety
AndyTWF
force-pushed
the
dispose-method
branch
from
e6f0db4 to
a97b5aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Tests/AblyChatTests/DisposeTests.swift (1)
84-90: Strengthen this test to prove concurrent room release, not just eventual release.At Line 84 onward, signaling completion immediately can still pass if
dispose()releases rooms serially. Add an assertion that bothrelease()calls were started before completing either operation.Suggested test hardening
// When: dispose() is called async let disposeTask: Void = rooms.dispose() + await Task.yield() + `#expect`(room1.releaseCallCount == 1) + `#expect`(room2.releaseCallCount == 1) // Allow the release operations to complete room1ReleaseOperation.complete() room2ReleaseOperation.complete() await disposeTaskAlso applies to: 92-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyChatTests/DisposeTests.swift` around lines 84 - 90, The test currently completes room1ReleaseOperation and room2ReleaseOperation immediately which doesn't prove concurrent release; modify the mock release implementations used by rooms.dispose() (the mocked release() calls represented by room1ReleaseOperation and room2ReleaseOperation) to set a "started" flag or fulfill an expectation when each release() begins, then assert both "started" signals have occurred before calling complete() on either operation, and only then complete both operations and await disposeTask; apply the same change to the second occurrence (the block around lines 92-95) so both tests verify that both release() calls were initiated prior to either finishing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AblyChat/DefaultConnection.swift`:
- Around line 61-64: The branch that cancels the transient timer and calls
callback(statusChange) can still allow execution to continue and emit the same
change again; in the methods touching isTimerRunning/currentState (referencing
isTimerRunning, currentState, timerManager.cancelTimer(), and
callback(statusChange)) cancel the timer and then immediately return/exit the
function (or otherwise short-circuit further handling) so the status change is
emitted only once; apply the same pattern to the other symmetric block that
covers lines 76-82 to prevent duplicate emissions.
In `@Tests/AblyChatTests/DisposeTests.swift`:
- Around line 20-22: The predicate closure passed to stream.first inside the
releaseImplementation closure must be annotated `@Sendable` to satisfy `@MainActor`
operator closure rules; update the closure passed to AsyncSequence.first (the
call in releaseImplementation) so the predicate is declared as an `@Sendable`
closure (e.g., annotate the parameter/closure) to match the pattern used
elsewhere (see releaseImplementation and the stream.first call).
---
Nitpick comments:
In `@Tests/AblyChatTests/DisposeTests.swift`:
- Around line 84-90: The test currently completes room1ReleaseOperation and
room2ReleaseOperation immediately which doesn't prove concurrent release; modify
the mock release implementations used by rooms.dispose() (the mocked release()
calls represented by room1ReleaseOperation and room2ReleaseOperation) to set a
"started" flag or fulfill an expectation when each release() begins, then assert
both "started" signals have occurred before calling complete() on either
operation, and only then complete both operations and await disposeTask; apply
the same change to the second occurrence (the block around lines 92-95) so both
tests verify that both release() calls were initiated prior to either finishing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.gitignoreExample/AblyChatExample/Mocks/MockClients.swiftSources/AblyChat/ChatClient.swiftSources/AblyChat/DefaultConnection.swiftSources/AblyChat/DefaultMessageReactions.swiftSources/AblyChat/DefaultMessages.swiftSources/AblyChat/DefaultOccupancy.swiftSources/AblyChat/DefaultTyping.swiftSources/AblyChat/InternalError.swiftSources/AblyChat/Room.swiftSources/AblyChat/Rooms.swiftSources/AblyChat/TypingTimerManager.swiftTests/AblyChatTests/DisposeTests.swift
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/AblyChat/Room.swift
- Sources/AblyChat/Rooms.swift
- Sources/AblyChat/TypingTimerManager.swift
Comment on lines
+61
to
64
| if isTimerRunning, currentState == .connected || currentState == .suspended || currentState == .failed { | ||
| self.timerManager.cancelTimer() | ||
| callback(statusChange) | ||
| } |
There was a problem hiding this comment.
Prevent duplicate status-change emission while transient timer is active.
When the branch at Line 61 emits callback(statusChange), execution continues and can emit again at Line 81 because isTimerRunning was captured before cancellation (Line 52). This can produce duplicate events for a single state transition.
💡 Proposed fix
if isTimerRunning, currentState == .connected || currentState == .suspended || currentState == .failed {
self.timerManager.cancelTimer()
callback(statusChange)
+ return
}Also applies to: 76-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AblyChat/DefaultConnection.swift` around lines 61 - 64, The branch
that cancels the transient timer and calls callback(statusChange) can still
allow execution to continue and emit the same change again; in the methods
touching isTimerRunning/currentState (referencing isTimerRunning, currentState,
timerManager.cancelTimer(), and callback(statusChange)) cancel the timer and
then immediately return/exit the function (or otherwise short-circuit further
handling) so the status change is emitted only once; apply the same pattern to
the other symmetric block that covers lines 76-82 to prevent duplicate
emissions.
Comment on lines
+20
to
+22
| releaseImplementation = { @Sendable () async in | ||
| await (stream.first { _ in true }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Tests/AblyChatTests/DisposeTests.swift | head -30 | tail -15Repository: ably/ably-chat-swift
Length of output: 611
🏁 Script executed:
# Let's see a broader view around lines 20-22
sed -n '15,30p' Tests/AblyChatTests/DisposeTests.swiftRepository: ably/ably-chat-swift
Length of output: 507
🏁 Script executed:
# Let's also check the file structure to understand the `@MainActor` context
sed -n '1,50p' Tests/AblyChatTests/DisposeTests.swiftRepository: ably/ably-chat-swift
Length of output: 2011
🏁 Script executed:
# Verify the syntax is valid by checking if similar `@Sendable` annotations appear elsewhere in the codebase
rg -t swift '@Sendable.*in\s+' Tests/ --max-count=5Repository: ably/ably-chat-swift
Length of output: 3290
🏁 Script executed:
# Check if there are other uses of AsyncSequence.first in the codebase to see patterns
rg -t swift '\.first\s*{' --max-count=10 -A 1Repository: ably/ably-chat-swift
Length of output: 149
Add @Sendable annotation to the AsyncSequence.first predicate closure.
At line 21, the predicate closure passed to stream.first is missing the @Sendable annotation. This violates the coding guideline requiring operator closures in @MainActor contexts to be marked @Sendable to avoid data race warnings. The codebase consistently uses this pattern elsewhere (e.g., DefaultRoomsTests.swift, IntegrationTests.swift).
Suggested fix
releaseImplementation = { `@Sendable` () async in
- await (stream.first { _ in true })
+ await (stream.first { `@Sendable` _ in true })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| releaseImplementation = { @Sendable () async in | |
| await (stream.first { _ in true }) | |
| } | |
| releaseImplementation = { `@Sendable` () async in | |
| await (stream.first { `@Sendable` _ in true }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/AblyChatTests/DisposeTests.swift` around lines 20 - 22, The predicate
closure passed to stream.first inside the releaseImplementation closure must be
annotated `@Sendable` to satisfy `@MainActor` operator closure rules; update the
closure passed to AsyncSequence.first (the call in releaseImplementation) so the
predicate is declared as an `@Sendable` closure (e.g., annotate the
parameter/closure) to match the pattern used elsewhere (see
releaseImplementation and the stream.first call).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the
dispose()method onChatClientto allow explicit disposal of the chat client and release of all associated resources (CHA-CL1 spec item).PR to fix spec: ably/specification#422
Changes
dispose()toChatClientProtocolandChatClientimplementationdispose()toDefaultRoomsthat releases all rooms concurrentlydispose()to room features that have internal state:DefaultMessages- cleans up subscription point continuationsDefaultTyping- invalidates typing timers viaTypingTimerManagerDefaultOccupancy- cleans up internal stateDefaultConnection- removes event listenersDefaultMessageReactions- no-op (no internal state)clientDisposederror case with error code 40014 (resourceDisposed)dispose()methods during room releaseBehavior
dispose()method is idempotent - calling it multiple times has no additional effectTest Plan
Added
DisposeTests.swiftwith tests covering:Summary by CodeRabbit
New Features
Behavior Changes
Tests