Skip to content

[ECO-5245] Convert public API and internals to @MainActor #261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 8, 2025
Merged

[ECO-5245] Convert public API and internals to @MainActor #261

merged 4 commits into from
Apr 8, 2025

Conversation

@lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Apr 3, 2025 •
edited by coderabbitai bot
Loading

In 7d6acde we made the decision to use actor types to manage the mutable state of the SDK. Whilst we've been able to build the SDK using this approach, it has proved cumbersome.

The public API is littered with async methods and getters for things that really should be synchronous, such as adding a subscription or fetching a room's state. This deviates from the JavaScript API and makes call sites look complicated.

The use of async for fetching room state also means that it's impossible for a user to, for example, check a room's state and then reason about what operations they should be able to attempt to perform on the room (i.e. without the SDK immediately rejecting the attempt), because the room state can change under their feet. This is one of a class of threading-related problems that I describe in #260.

Internally, we find ourselves having to write await to do basic things like fetching the room lifecycle manager's state or to find out the arguments with which a mock method was called. And similarly, one internal component can't rely on the state that's isolated to a different internal isolation domain in order to make a decision, because this state might change under its feet.

To address this, here I have decided to isolate all of the SDK's mutable state to the main actor. This simplifies the experience of developing the SDK, and it simplifies the experience of interacting with it from the main thread, which is where most of our users interact with it from.

(Note that this does not prevent us from using background tasks internally for moving heavy processing off the main thread; I'm specifically talking about state.)

This also moves us closer to being able to address #49 (keeping our internal state in sync with that of the underlying ably-cocoa objects).

Have removed:

  • all actor types (both in SDK and mocks)
  • nearly all locking (there are a couple of non-MainActor types for which we still have it)

There might be some simplifications that can be made for the lifecycle manager tests now, but I'm not going to look at them now because that class will change a lot for the single-channel work in #242.

I'll update the ably.com documentation in #254.

Even after this change, Messages.subscribe is still async because its implementation requires it to be. I believe that this is a bug; have created #257 for it.

Resolves #232.

Summary by CodeRabbit

  • Documentation
    • Revised contribution guidelines and concurrency discussions to support a more predictable development workflow.
  • Refactor
    • Streamlined chat operations by shifting from asynchronous to synchronous patterns and enforcing main-thread execution, enhancing stability and responsiveness.
  • Tests
    • Updated test suites to run on the main actor, improving reliability and consistency in concurrent scenarios.
  • Chores
    • Removed redundant locking mechanisms and cleaned up state management for improved internal efficiency.

@coderabbitai
Copy link

coderabbitai bot commented Apr 3, 2025 •
edited
Loading

Walkthrough

The changes refactor the SDK’s concurrency model by adding guidelines that enforce isolation of mutable state to the main actor. Many components, including public APIs, example code, core protocols, and tests, have been updated to use the @MainActor attribute and remove unnecessary asynchronous constructs. Method signatures have been adjusted to be synchronous where appropriate, actors have been converted to classes in mocks and core components, and documentation has been enhanced with new guidelines on Swift concurrency rough edges.

Changes

File(s) Change Summary
CONTRIBUTING.md, README.md Added new guidelines on isolating mutable state to the main actor; introduced a section on Swift concurrency rough edges; updated initialization style for status subscriptions.
Example/AblyChatExample/ContentView.swift Updated method signatures (e.g., createChatClient marked with @MainActor and room-related methods now taking a Room parameter); replaced custom task handling with standard .task using a do-catch block.
Example/AblyChatExample/Mocks/*.swift
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift		 Converted various actor declarations to class; removed async from subscription/termination methods; adjusted property initializations and added/remodeled @MainActor usage.
Sources/AblyChat/*.swift Applied @MainActor to public protocols and classes (e.g., ChatAPI, Messages, Connection, Room, etc.); refactored asynchronous methods to synchronous ones; updated property access using nonisolated where needed.
Tests/AblyChatTests/*.swift Test files updated with @MainActor; removed unnecessary await keywords; included @Sendable attributes in closure parameters; adjusted instantiation and mock usage to align with synchronous execution.

Sequence Diagram(s)

sequenceDiagram
    participant UI as User Interface (MainActor)
    participant API as ChatAPI (MainActor)
    participant Client as ChatClient (MainActor)
    participant Room as Room (Synchronous)
    participant Sub as Subscription

    UI->>API: Initialize ChatAPI
    API->>Client: createChatClient() (MainActor)
    Client->>Room: Create/attach room synchronously
    Room->>Sub: Subscribe to messages/status updates
    Sub-->>UI: Deliver updates on main thread
Loading

Assessment against linked issues

Objective Addressed Explanation
Switch public API to be @MainActor [#232, ECO-5245] ✅

Possibly related issues

Possibly related PRs

Poem

In the code garden, I, a busy little bunny,
Hop along the main thread, keeping state so funny.
No more async delays or locking run around,
With @MainActor guiding, thread safety we have found.
Hop on, dear developers, let clear code abound!
🐇✨

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot temporarily deployed to staging/pull/261/AblyChat April 3, 2025 17:16 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/261/AblyChat April 3, 2025 17:24 Inactive
coderabbitai[bot]
coderabbitai bot reviewed Apr 3, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🔭 Outside diff range comments (2)
Example/AblyChatExample/Mocks/MockClients.swift (1)

119-123: 💡 Verification agent

🧩 Analysis chain

Method signature synchronized but inconsistent with implementation

The subscribe method is now synchronous, which aligns with the PR's goal. However, there appears to be an inconsistency: according to the PR summary, Messages.subscribe should remain async, but this mock implementation is synchronous. This might be intentional for the mock, but it's worth verifying.

🏁 Script executed:

#!/bin/bash
# Check the Messages protocol to see if subscribe is still expected to be async
rg -A 3 "func subscribe.*BufferingPolicy" --type swift | grep -e "async"

Length of output: 517

Action Required: Align Mock Signature with Asynchronous Interface

The production implementation of subscribe(bufferingPolicy:) is defined as an asynchronous function (in Sources/AblyChat/Messages.swift and related files), whereas the mock implementation in Example/AblyChatExample/Mocks/MockClients.swift remains synchronous. This inconsistency can lead to confusion or integration issues.

  • Location: Example/AblyChatExample/Mocks/MockClients.swift, lines 119–123
  • Issue: The method signature in the mock does not reflect the async throws nature of the production interface.
  • Recommendation: Update the mock implementation to be asynchronous (e.g., add async throws to its signature) or document the reasoning for intentionally deviating from the production signature.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

96-104: 🛠️ Refactor suggestion

Methods that modify state might need thread safety consideration

These methods modify mutable state and were previously protected by actor isolation. Now they could potentially be called concurrently from different threads, leading to race conditions.

Consider one of these approaches:

  1. Add @MainActor annotation to these methods
  2. Ensure the mock is only accessed from a single thread in tests
  3. Add explicit synchronization (like a lock) if concurrent access is expected
+@MainActor
 func attach() async throws(InternalError) {
     attachCallCount += 1
     // ...
 }

+@MainActor
 func publish(_ name: String?, data: JSONValue?, extras: [String: JSONValue]?) {
     lastMessagePublishedName = name
     lastMessagePublishedExtras = extras
     lastMessagePublishedData = data
 }

Also applies to: 110-118, 179-183, 193-204, 206-208

🧹 Nitpick comments (7)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)

19-19: Validate append-only usage of emitDiscontinuityArguments.
Appending to emitDiscontinuityArguments is straightforward for single-thread usage. If you plan to run these tests concurrently, confirm that this array isn’t mutated from multiple threads at the same time without synchronization.

Tests/AblyChatTests/DefaultMessagesTests.swift (3)

68-68: Cross-check for removed concurrency dependencies.
Confirm that dependencies used by defaultMessages initialization no longer require await.

151-151: Confirm that the @Sendable closure captures only thread-safe resources.
Similar to line 115, ensure the closure’s captured data is safely shareable across concurrency boundaries.

167-168: Check ordering for onDiscontinuity subscription vs. event emission.
These two lines are tightly coupled: the subscription is created, then the event is immediately emitted. Ensure the test verifies real-world timing (sometimes discontinuities arrive before subscription).

Sources/AblyChat/RoomReactions.swift (1)

34-35: Documentation needs updating to match synchronous behavior.

The method documentation still mentions an "AsyncSequence" which suggests asynchronous behavior, but the method is now synchronous. The documentation should be updated to accurately reflect the current behavior.

Sources/AblyChat/DefaultTyping.swift (1)

251-251: Consider concurrency safety for EventTracker struct.
Since EventTracker is accessing mutable state across Task boundaries, consider making it an actor or confining its mutation strictly to the main actor context.

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

11-11: Nonisolated annotations are now unnecessary but harmless

Now that the class is no longer an actor, the nonisolated keywords on these methods don't serve a functional purpose since classes don't have isolation boundaries by default.

You could remove these nonisolated annotations for cleaner code, but keeping them doesn't cause any harm and might document that these methods were intentionally designed to be callable from any context.

Also applies to: 59-61, 142-154, 156-177

🛑 Comments failed to post (1)
Sources/AblyChat/DefaultTyping.swift (1)

65-65: ⚠️ Potential issue

Potential concurrency concern with eventTracker mutation in Task.
Although this class is marked @MainActor, using Task { ... } by default may run on a background thread, creating a data race on eventTracker. To keep mutations on the main actor, wrap this operation in Task { @MainActor in ... } or use MainActor.run:

 Task {
-    let currentEventID = eventTracker.updateEventID()
+    await MainActor.run {
+        let currentEventID = eventTracker.updateEventID()
+        // proceed with logic here
+    }
 }
📝 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.

Task {
    await MainActor.run {
        let currentEventID = eventTracker.updateEventID()
        // proceed with logic here
    }
}

@github-actions github-actions bot temporarily deployed to staging/pull/261/AblyChat April 3, 2025 17:42 Inactive
coderabbitai[bot]
coderabbitai bot reviewed Apr 3, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
Tests/AblyChatTests/Mocks/MockChannels.swift (1)

4-26: Consider explicitly marking the class with @mainactor

While the removal of synchronization mechanisms is consistent with the PR's approach, it would be clearer to explicitly mark this class with @MainActor to ensure all future usage remains on the main actor. This would make the concurrency model more explicit and prevent potential misuse.

-final class MockChannels: InternalRealtimeChannelsProtocol {
+@MainActor
+final class MockChannels: InternalRealtimeChannelsProtocol {
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (4)

11-11: Nonisolated keyword is redundant when not in actor context

The nonisolated keyword is specific to actor isolation and doesn't have the same effect in a class. Since this has been converted from an actor to a class, this keyword may be unnecessary.

-    nonisolated var properties: ARTChannelProperties { .init(attachSerial: attachSerial, channelSerial: channelSerial) }
+    var properties: ARTChannelProperties { .init(attachSerial: attachSerial, channelSerial: channelSerial) }

59-61: Remove nonisolated keyword from all methods

Similar to the properties getter, the nonisolated keyword is redundant in a class context.

-    nonisolated var underlying: any RealtimeChannelProtocol {
+    var underlying: any RealtimeChannelProtocol {
         fatalError("Not implemented")
     }

142-154: Remove nonisolated from subscribe method

The nonisolated keyword should be removed from this method as it's no longer in an actor context.

-    nonisolated func subscribe(_: String, callback: @escaping ARTMessageCallback) -> ARTEventListener? {
+    func subscribe(_: String, callback: @escaping ARTMessageCallback) -> ARTEventListener? {

156-158: Clean up remaining nonisolated keywords

Remove the nonisolated keyword from all remaining methods as they're redundant in a class context.

This applies to the following methods:

  • unsubscribe(_:) on line 156
  • Both on(_:callback:) methods on lines 160 and 164
  • off(_:) on line 168
  • name property getter on line 172

Also applies to: 160-166, 168-170, 172-177

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc30e24 and 6cf993e.

📒 Files selected for processing (53)
  • CONTRIBUTING.md (2 hunks)
  • Example/AblyChatExample/ContentView.swift (9 hunks)
  • Example/AblyChatExample/Mocks/MockClients.swift (11 hunks)
  • Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1 hunks)
  • README.md (1 hunks)
  • Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (5 hunks)
  • Sources/AblyChat/ChatAPI.swift (1 hunks)
  • Sources/AblyChat/ChatClient.swift (4 hunks)
  • Sources/AblyChat/Connection.swift (1 hunks)
  • Sources/AblyChat/DefaultConnection.swift (3 hunks)
  • Sources/AblyChat/DefaultMessages.swift (4 hunks)
  • Sources/AblyChat/DefaultOccupancy.swift (4 hunks)
  • Sources/AblyChat/DefaultPresence.swift (5 hunks)
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (3 hunks)
  • Sources/AblyChat/DefaultTyping.swift (10 hunks)
  • Sources/AblyChat/EmitsDiscontinuities.swift (2 hunks)
  • Sources/AblyChat/Messages.swift (3 hunks)
  • Sources/AblyChat/Occupancy.swift (3 hunks)
  • Sources/AblyChat/Presence.swift (4 hunks)
  • Sources/AblyChat/Room.swift (14 hunks)
  • Sources/AblyChat/RoomFeature.swift (2 hunks)
  • Sources/AblyChat/RoomLifecycleManager.swift (15 hunks)
  • Sources/AblyChat/RoomReactions.swift (3 hunks)
  • Sources/AblyChat/Rooms.swift (6 hunks)
  • Sources/AblyChat/SimpleClock.swift (1 hunks)
  • Sources/AblyChat/Subscription.swift (2 hunks)
  • Sources/AblyChat/SubscriptionStorage.swift (1 hunks)
  • Sources/AblyChat/TimerManager.swift (1 hunks)
  • Sources/AblyChat/Typing.swift (3 hunks)
  • Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultChatClientTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultMessagesTests.swift (7 hunks)
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (100 hunks)
  • Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (3 hunks)
  • Tests/AblyChatTests/DefaultRoomReactionsTests.swift (5 hunks)
  • Tests/AblyChatTests/DefaultRoomTests.swift (15 hunks)
  • Tests/AblyChatTests/DefaultRoomsTests.swift (12 hunks)
  • Tests/AblyChatTests/IntegrationTests.swift (14 hunks)
  • Tests/AblyChatTests/Mocks/MockChannels.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockConnection.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockInternalRealtimeClientFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtime.swift (3 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoom.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockSimpleClock.swift (1 hunks)
  • Tests/AblyChatTests/SubscriptionStorageTests.swift (3 hunks)
  • Tests/AblyChatTests/SubscriptionTests.swift (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
  • Tests/AblyChatTests/ChatAPITests.swift
  • README.md
  • Tests/AblyChatTests/DefaultChatClientTests.swift
  • Tests/AblyChatTests/Mocks/MockConnection.swift
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift
  • Tests/AblyChatTests/SubscriptionStorageTests.swift
  • Tests/AblyChatTests/DefaultMessagesTests.swift
  • Sources/AblyChat/TimerManager.swift
  • Tests/AblyChatTests/IntegrationTests.swift
  • CONTRIBUTING.md
  • Sources/AblyChat/ChatAPI.swift
  • Tests/AblyChatTests/Mocks/MockInternalRealtimeClientFactory.swift
  • Tests/AblyChatTests/DefaultRoomTests.swift
  • Tests/AblyChatTests/Mocks/MockSimpleClock.swift
  • Sources/AblyChat/Connection.swift
  • Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift
  • Sources/AblyChat/Messages.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
  • Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
  • Sources/AblyChat/Typing.swift
  • Sources/AblyChat/Occupancy.swift
  • Tests/AblyChatTests/SubscriptionTests.swift
  • Sources/AblyChat/DefaultPresence.swift
  • Sources/AblyChat/RoomReactions.swift
  • Sources/AblyChat/RoomFeature.swift
  • Sources/AblyChat/SimpleClock.swift
  • Sources/AblyChat/Subscription.swift
  • Sources/AblyChat/EmitsDiscontinuities.swift
  • Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
  • Sources/AblyChat/ChatClient.swift
  • Sources/AblyChat/DefaultRoomReactions.swift
  • Sources/AblyChat/DefaultOccupancy.swift
  • Sources/AblyChat/DefaultTyping.swift
  • Example/AblyChatExample/ContentView.swift
🧰 Additional context used
🧬 Code Definitions (12)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (7)
Sources/AblyChat/RoomFeature.swift (1)
  • onDiscontinuity (69-71)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • onDiscontinuity (19-21)
Sources/AblyChat/DefaultMessages.swift (2)
  • onDiscontinuity (48-50)
  • onDiscontinuity (205-207)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
  • onDiscontinuity (156-173)
Sources/AblyChat/DefaultOccupancy.swift (2)
  • onDiscontinuity (24-26)
  • onDiscontinuity (91-93)
Sources/AblyChat/DefaultRoomReactions.swift (2)
  • onDiscontinuity (30-32)
  • onDiscontinuity (126-128)
Sources/AblyChat/DefaultTyping.swift (2)
  • onDiscontinuity (32-34)
  • onDiscontinuity (211-213)
Tests/AblyChatTests/DefaultRoomsTests.swift (6)
Sources/AblyChat/Rooms.swift (3)
  • testsOnly_hasRoomMapEntryWithID (303-313)
  • release (316-354)
  • testsOnly_subscribeToOperationWaitEvents (145-147)
Sources/AblyChat/Room.swift (1)
  • release (418-425)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
  • release (268-290)
Tests/AblyChatTests/Mocks/MockRoom.swift (1)
  • release (55-62)
Sources/AblyChat/RoomLifecycleManager.swift (1)
  • testsOnly_subscribeToOperationWaitEvents (581-583)
Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1)
  • setRoom (12-14)
Tests/AblyChatTests/Mocks/MockChannels.swift (6)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
  • get (82-89)
Example/AblyChatExample/Mocks/MockRealtime.swift (3)
  • get (86-88)
  • get (256-258)
  • get (260-262)
Sources/AblyChat/DefaultMessages.swift (2)
  • get (32-34)
  • get (172-178)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (3)
  • get (144-147)
  • get (268-284)
  • get (286-302)
Sources/AblyChat/DefaultOccupancy.swift (2)
  • get (20-22)
  • get (81-88)
Sources/AblyChat/DefaultPresence.swift (2)
  • get (16-18)
  • get (20-22)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
  • onRoomStatusChange (297-299)
Sources/AblyChat/DefaultConnection.swift (4)
Sources/AblyChat/TimerManager.swift (3)
  • hasRunningTask (24-26)
  • cancelTimer (19-22)
  • setTimer (7-17)
Sources/AblyChat/SubscriptionStorage.swift (1)
  • emit (43-47)
Sources/AblyChat/Subscription.swift (1)
  • emit (72-79)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
  • off (247-249)
  • off (400-402)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (8)
Sources/AblyChat/DefaultRoomReactions.swift (4)
  • subscribe (26-28)
  • subscribe (69-123)
  • onDiscontinuity (30-32)
  • onDiscontinuity (126-128)
Sources/AblyChat/RoomReactions.swift (1)
  • subscribe (45-47)
Sources/AblyChat/DefaultMessages.swift (4)
  • subscribe (28-30)
  • subscribe (76-169)
  • onDiscontinuity (48-50)
  • onDiscontinuity (205-207)
Sources/AblyChat/RoomFeature.swift (1)
  • onDiscontinuity (69-71)
Sources/AblyChat/EmitsDiscontinuities.swift (1)
  • onDiscontinuity (34-36)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (15-17)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
  • emitDiscontinuity (21-23)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (19-21)
Sources/AblyChat/Rooms.swift (2)
Sources/AblyChat/Room.swift (1)
  • createRoom (150-159)
Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1)
  • createRoom (16-25)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (6)
Sources/AblyChat/RoomLifecycleManager.swift (6)
  • testsOnly_subscribeToHandledContributorStateChanges (350-352)
  • testsOnly_subscribeToHandledTransientDisconnectTimeouts (378-380)
  • createManager (41-50)
  • performAttachOperation (712-714)
  • performAttachOperation (716-718)
  • onRoomStatusChange (297-299)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1)
  • createManager (11-14)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
  • performAttachOperation (19-29)
  • onRoomStatusChange (54-56)
  • waitToBeAbleToPerformPresenceOperations (62-64)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
  • emitStateChange (206-208)
Sources/AblyChat/RoomFeature.swift (1)
  • waitToBeAbleToPerformPresenceOperations (73-75)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
  • waitToBeAbleToPerformPresenceOperations (25-35)
Sources/AblyChat/DefaultMessages.swift (7)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
  • onDiscontinuity (156-173)
Sources/AblyChat/DefaultRoomReactions.swift (2)
  • onDiscontinuity (30-32)
  • onDiscontinuity (126-128)
Sources/AblyChat/RoomFeature.swift (1)
  • onDiscontinuity (69-71)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • onDiscontinuity (19-21)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
  • onDiscontinuity (17-19)
Sources/AblyChat/DefaultOccupancy.swift (2)
  • onDiscontinuity (24-26)
  • onDiscontinuity (91-93)
Sources/AblyChat/DefaultTyping.swift (2)
  • onDiscontinuity (32-34)
  • onDiscontinuity (211-213)
Sources/AblyChat/Presence.swift (2)
Sources/AblyChat/DefaultPresence.swift (4)
  • subscribe (52-54)
  • subscribe (56-58)
  • subscribe (261-280)
  • subscribe (282-307)
Sources/AblyChat/DefaultRoomReactions.swift (2)
  • subscribe (26-28)
  • subscribe (69-123)
Example/AblyChatExample/Mocks/MockClients.swift (6)
Sources/AblyChat/Room.swift (2)
  • onStatusChange (110-112)
  • onStatusChange (429-431)
Sources/AblyChat/Connection.swift (1)
  • onStatusChange (36-38)
Tests/AblyChatTests/Mocks/MockRoom.swift (1)
  • onStatusChange (43-45)
Sources/AblyChat/Presence.swift (2)
  • subscribe (130-132)
  • subscribe (134-136)
Sources/AblyChat/DefaultRoomReactions.swift (2)
  • subscribe (26-28)
  • subscribe (69-123)
Sources/AblyChat/DefaultTyping.swift (2)
  • subscribe (16-18)
  • subscribe (55-115)
Sources/AblyChat/RoomLifecycleManager.swift (8)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (19-21)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (15-17)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2)
  • emitDiscontinuity (21-23)
  • waitToBeAbleToPerformPresenceOperations (25-35)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
  • onRoomStatusChange (54-56)
  • waitToBeAbleToPerformPresenceOperations (62-64)
Sources/AblyChat/RoomFeature.swift (1)
  • waitToBeAbleToPerformPresenceOperations (73-75)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1)
  • createManager (11-14)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
  • subscribeToState (193-204)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
  • subscribeToState (251-258)
🔇 Additional comments (119)
Sources/AblyChat/SubscriptionStorage.swift (6)

6-7: Good application of @MainActor to ensure thread safety.

Converting this class from @unchecked Sendable to @MainActor is a solid improvement. This change aligns perfectly with the PR's goal of isolating mutable state to the main actor, eliminating the need for manual synchronization mechanisms. The @MainActor annotation ensures that all operations on subscriptions are performed on the main thread, making the code safer and more maintainable.

21-21: Removal of locks simplifies the code.

The direct assignment to the subscriptions dictionary without explicit locking is now safe due to the @MainActor annotation on the class. This change simplifies the code while maintaining thread safety guarantees.

24-26: Proper use of @MainActor in the termination handler.

Using Task { @MainActor in ... } ensures that the subscription termination handling occurs on the main thread, which is necessary since it modifies the subscriptions dictionary. The use of a task allows the termination handler to be non-blocking while still enforcing the actor isolation.

34-35: Simplified property access with actor isolation.

Direct access to subscriptions.count without locking is now safe due to the @MainActor annotation. This simplification makes the code cleaner and easier to maintain.

39-39: Safe dictionary modification without explicit locking.

Directly modifying the subscriptions dictionary in subscriptionDidTerminate without explicit locking is safe due to the @MainActor annotation. This change simplifies the code and maintains thread safety.

44-46: Simplified iteration over subscriptions.

Direct iteration over subscriptions without locking in the emit method is now safe due to the @MainActor annotation. This change removes the need for complex synchronization code while maintaining thread safety.

Sources/AblyChat/DefaultConnection.swift (8)

8-10: Great job using internal private(set) for encapsulation

These property modifications align well with the PR objective of isolating mutable state to the main actor. By using private(set), you're ensuring the properties can only be modified within the class while still allowing them to be read externally.

15-16: Good synchronous initialization

Nice work simplifying the initialization by directly setting the properties instead of using asynchronous calls. This makes the code cleaner and more efficient.

42-42: Proper removal of await from timer check

Correctly removed the await keyword from timerManager.hasRunningTask() to match the method's now synchronous implementation.

52-52: Synchronous timer cancellation

Good conversion of the timer cancellation to a synchronous call, which matches the updated TimerManager implementation.

55-56: Consistent state updates

These direct state updates ensure that local state matches the emitted status change. Well-structured approach to maintaining consistency.

61-69: Improved timer handler implementation

The refactored timer handling code correctly captures the weak reference to timerManager and directly updates the properties on self. This is a good implementation of the transient disconnect timer logic.

80-82: Essential state updates after emitting status change

Important addition of these state updates after emitting the status change. This ensures the internal state is always consistent with what's been communicated to subscribers.

86-88: Good use of @MainActor for connection cleanup

Excellent use of Task { @MainActor in ... } to ensure the event listener is unregistered on the main actor. This aligns with the PR's objective of isolating mutable state changes to the main actor.

Tests/AblyChatTests/Mocks/MockChannels.swift (4)

4-4: Removed @unchecked Sendable conformance aligns with MainActor isolation strategy

The removal of @unchecked Sendable is consistent with the PR's approach of isolating mutable state to the main actor instead of relying on explicit thread safety mechanisms. This simplifies the class's concurrency model.

6-7: Property renaming improves clarity and simplifies access

Renaming the properties from _getArguments and _releaseArguments to getArguments and releaseArguments removes the underscore prefix, which is a good practice for properties that are meant to be accessed via their property wrapper (private(set)). The inline initialization also simplifies the code.

14-14: Removed mutex synchronization for property access

The code now directly appends to the getArguments array without using a mutex lock. This is safe if all access to this class is confined to the main actor, which aligns with the PR's strategy of main-actor isolation.

24-24: Removed mutex synchronization for property access

Similar to the getArguments property, the code now directly appends to the releaseArguments array without using a mutex lock. This is consistent with the PR's approach to simplify concurrency by isolating mutable state to the main actor.

Tests/AblyChatTests/Mocks/MockRoom.swift (3)

4-4: Transformation from actor to class aligns with main actor pattern.

The change from actor MockRoom to class MockRoom is consistent with the PR's objective to isolate mutable state to the main actor rather than using individual actors. This simplifies the concurrency model across the SDK.

43-43: API simplification by removing async keyword.

Removing the async keyword from onStatusChange simplifies the API and improves the user experience by making the method synchronous. This is consistent with the PR's goal to reduce unnecessary asynchronous operations.

4-70:

❓ Verification inconclusive

Verify thread safety in the mock implementation.

With the switch from actor to class, automatic protection against concurrent access to mutable state is removed. Since the class has mutable state (releaseCallCount), ensure any test using this mock handles concurrency appropriately or only accesses it from the main thread.

This change appears to be part of the broader strategy to centralize concurrency control at the @MainActor level rather than using individual actors.

🏁 Script executed:

#!/bin/bash
# Check how this mock is used in tests to ensure thread safety

# Look for places where MockRoom is instantiated
echo "Searching for MockRoom instantiations:"
rg "MockRoom\(" Tests/ -A 2

# Look for places where releaseCallCount is accessed
echo "\nSearching for releaseCallCount access:"
rg "releaseCallCount" Tests/ -A 2

Length of output: 5500

Thread Safety in the MockRoom Implementation

The switch from an actor to a class means that the built‐in concurrency protection is no longer present. Our search shows that:

  • In tests (e.g., in Tests/AblyChatTests/DefaultRoomsTests.swift), MockRoom is instantiated and its release() method is called in a sequential, single-threaded manner.
  • Checks on releaseCallCount (e.g., assertions like #expect(roomToReturn.releaseCallCount == 1)) suggest that the current usage assumes controlled, non-concurrent access.

Action Items:

  • Confirm that all invocations of release() are executed on the main thread or within a context that serializes access.
  • If there's any possibility of concurrent calls, introduce appropriate synchronization or limit access to ensure thread safety.
  • Remember that this change aligns with the project’s strategy of centralizing concurrency control at the @MainActor level.
Tests/AblyChatTests/Mocks/MockRealtime.swift (4)

6-6: Implementation successfully matches the new concurrency model

The class definition has correctly been kept as a final class while implementing the InternalRealtimeClientProtocol. This change aligns with the PR objective of refactoring the SDK's concurrency model by simplifying property access and removing specialized actor types.

12-13: Synchronization mechanism successfully removed

The refactoring correctly removes the previous synchronization mechanism, replacing the mutex-guarded private properties with directly accessible private(set) properties. This aligns with moving state management to the main actor as described in the PR objectives.

32-32: Simplified direct property access

The direct append to requestArguments without mutex locking represents a clean implementation of the new concurrency model, where synchronization is handled by the main actor rather than explicit locks.

48-48: Direct property assignment without synchronization

The direct assignment to createWrapperSDKProxyOptionsArgument without synchronization is consistent with the new concurrency approach where thread safety is managed through the main actor.

Example/AblyChatExample/Mocks/MockClients.swift (11)

4-4: Actor successfully converted to class

The MockChatClient has been successfully converted from an actor to a class, aligning with the PR objective of simplifying the concurrency model by isolating mutable state to the main actor instead of using actor types.

22-22: Actor successfully converted to class

The MockRooms actor has been correctly converted to a class as part of the broader effort to refactor the SDK's concurrency model and simplify state management.

44-53: Room implementation properly refactored

The MockRoom class has been successfully converted from an actor, with properties properly marked as nonisolated where appropriate, which allows them to be accessed from any context safely in the new concurrency model.

58-62: Property initialization refactored correctly

The direct initialization of properties in the constructor replaces previous lazy initialization patterns, which is appropriate for the new synchronous access pattern and main actor isolation model.

83-83: Synchronous method signature is appropriate

The onStatusChange method now correctly uses a synchronous signature, consistent with the relevant code snippets from Room.swift where the method returns a subscription without requiring asynchronous execution.

88-88: Messages implementation properly converted and synchronized

The MockMessages class and its subscribe method have been successfully converted from asynchronous actor patterns to synchronous class methods, matching the simplified concurrency model.

Also applies to: 119-119

188-188: MockRoomReactions properly implements synchronous subscription

The MockRoomReactions class and its subscribe method successfully follow the new pattern of synchronous operation, consistent with the changes in DefaultRoomReactions.swift.

Also applies to: 226-226

235-235: MockTyping properly converted to synchronous class implementation

The MockTyping class and its subscribe method correctly implement the synchronous pattern, aligned with the changes in DefaultTyping.swift.

Also applies to: 257-257

278-278: MockPresence successfully converted with correct method signatures

The MockPresence class has been properly converted from an actor, with the subscribe methods correctly implemented as synchronous methods that match the signatures from the Presence.swift interface.

Also applies to: 385-391

398-398: MockOccupancy properly implements the new concurrency model

The MockOccupancy class and its subscribe method correctly follow the simplified synchronous pattern, which is consistent with the PR's objective of moving state management to the main actor.

Also applies to: 418-418

431-446: MockConnection implementation correctly refactored

The MockConnection class properly implements the Connection protocol with the appropriate synchronous method signatures. The onStatusChange method is correctly marked as nonisolated to indicate thread safety.

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (4)

4-4: Impact of switching from actor to class on concurrency safety

Changing from final actor to final class removes the automatic isolation guarantees that Swift actors provide. Since this is part of the broader initiative to consolidate mutable state to the main actor, ensure proper thread safety is maintained for this mock class.

Consider if any explicit @MainActor annotations are needed on the class or specific methods to maintain thread safety, especially for methods that modify state like publish(_:data:extras:) on line 179 and emitStateChange(_:) on line 206.

20-29: Review continued need for @unchecked Sendable with actor removal

Now that the class is no longer an actor, the @unchecked Sendable annotation might be less critical, though still important if this struct is passed across thread boundaries. The TODO comment mentions revisiting this in issue #195.

Consider whether @unchecked Sendable is still needed after the concurrency model change. If the struct is only used within the main actor context, it might be unnecessary.

179-183: Ensure thread safety for state-modifying methods

The publish method modifies the state of the mock object by updating lastMessagePublishedName, lastMessagePublishedExtras, and lastMessagePublishedData. With the removal of actor isolation, ensure this method is called in a thread-safe manner.

Consider whether this method should be explicitly marked with @MainActor or if other synchronization mechanisms are needed to prevent race conditions.

96-104:

❓ Verification inconclusive

Verify thread safety for async operations

With the removal of actor isolation, the attach() and detach() methods might need explicit thread safety measures. If these are intended to be called from arbitrary threads, consider whether they need to be explicitly marked with @MainActor or if synchronization is needed.

Also applies to: 110-118

🏁 Script executed:

#!/bin/bash
# Check for other @MainActor annotations in similar mock classes
rg "@MainActor" --type swift | grep -i "Mock.*\.swift"

Length of output: 218

Verify Thread Safety in Async Methods for Mock Realtime Channel

In Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift, both the attach() method (lines 96–104) and its counterpart (lines 110–118) need a careful review regarding thread safety. With the removal of actor isolation, please ensure that these async operations are safe for concurrent access. In particular:

  • Assess whether these methods may be called from arbitrary threads.
    If yes, consider explicitly annotating them with @MainActor or introducing synchronization to guard mutations (e.g., to attachCallCount).

  • Reference from similar mocks:
    Other mocks (for example, in Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift) already demonstrate the use of @MainActor, which might indicate the intended design approach.

Please re-evaluate the thread-confinement requirements for these methods to prevent potential race conditions.

Sources/AblyChat/DefaultMessages.swift (4)

13-16: Constructor made synchronous as part of MainActor conversion

The constructor has been updated to be synchronous (removing async), and the initialization of implementation no longer uses await. This is part of the PR's effort to isolate mutable state to the main actor, simplifying the API and improving the developer experience.

48-50: Simplified onDiscontinuity method from async to sync

The onDiscontinuity method has been converted from asynchronous to synchronous by removing the async keyword, aligning with the other contributor implementations. This change makes the method signature more consistent with similar methods in DefaultOccupancy, DefaultPresence, and other related classes.

72-72: Made handleChannelEvents call synchronous

The call to handleChannelEvents is now synchronous, removing the need for await. This is consistent with the overall approach of simplifying concurrency by moving mutable state to the main actor.

205-207: Made onDiscontinuity implementation synchronous

The onDiscontinuity implementation in the Implementation class is now synchronous, directly delegating to the featureChannel.onDiscontinuity method. This matches the pattern in other feature components and simplifies the concurrency model.

Tests/AblyChatTests/Mocks/MockRoomFactory.swift (2)

3-3: Converted actor to class as part of MainActor migration

The MockRoomFactory has been changed from an actor to a class to align with the PR's goal of isolating all mutable state to the main actor instead of using actor-based isolation. This is part of the broader concurrency model simplification.

16-16: Simplified createRoom method to be synchronous

The createRoom method signature has been updated to remove the async keyword, making it synchronous. This matches the updated protocol definition and simplifies the testing approach by reducing the need for await when calling this method in tests.

Tests/AblyChatTests/DefaultRoomsTests.swift (6)

5-6: Added @mainactor to test struct

Adding the @MainActor attribute to the test struct ensures that all methods within this struct execute on the main actor, which is crucial for testing components that have been migrated to use the main actor isolation pattern. This aligns with the PR's objective of isolating mutable state to the main actor.

56-58: Removed await for synchronous methods

The calls to rooms.testsOnly_hasRoomMapEntryWithID and accessing roomFactory.createRoomArguments no longer use await since these methods are now synchronous after the main actor migration. This simplifies the test code by eliminating unnecessary async/await patterns.

109-110: Added @sendable to closures in async contexts

The closure passed to first has been marked with @Sendable to ensure it can be safely used across concurrency domains. This is an important safety measure when working with Swift's concurrency model, as it ensures the closure doesn't capture mutable state in an unsafe way.

115-117: Added @sendable to operationWaitEvent closure

The closure processing operationWaitEvent has been marked with @Sendable to ensure thread safety when working with closures across concurrency domains. This improves the robustness of the test code in the Swift concurrency environment.

123-125: Added @sendable to another operationWaitEvent closure

Consistently applied the @Sendable attribute to all closures used in asynchronous contexts, ensuring thread safety throughout the test code. This systematic approach to marking closures in async contexts demonstrates good practices for working with Swift's concurrency model.

186-186: Added @sendable to closure parameter for async sequence

The closure for the roomReleaseCalls.first call is now marked with @Sendable for thread safety, maintaining consistency with other similar closures throughout the test file. This pattern is repeated in multiple places in the test file (lines 235, 295, 402) for all asynchronous sequences.

Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)

4-4: Converted actor to class for MainActor migration

The MockRoomLifecycleManager has been changed from an actor to a regular class as part of the broader migration to use @MainActor for state isolation. This allows the mock to be more easily used in test contexts that are themselves marked with @MainActor.

54-54: Simplified onRoomStatusChange method to be synchronous

The onRoomStatusChange method no longer has the async keyword, making it synchronous to match the updated protocol definition. This aligns with the updated implementation in RoomLifecycleManager and other related components, creating a more consistent API.

Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2)

4-4: Class declaration changed from actor to class

The change from final actor to final class aligns with the PR's objective of isolating mutable state to the main actor rather than using individual actors. This removes the automatic thread safety that actors provide, which is now expected to be handled through @MainActor at the call sites.

17-17: Method made synchronous by removing async keyword

The onDiscontinuity method has been made synchronous by removing the async keyword, which aligns with the PR's goal of simplifying the API. Since the implementation simply creates a subscription (a synchronous operation), this change makes sense and matches similar changes in other components like DefaultMessages, DefaultOccupancy, etc.

Tests/AblyChatTests/DefaultRoomReactionsTests.swift (6)

5-5: Added @mainactor attribute to test struct

The addition of @MainActor ensures all test methods run on the main actor, which aligns with the PR's strategy of isolating mutable state to the main actor. This change enables the tests to directly interact with the now-synchronous methods without needing explicit await keywords.

16-16: Removed await when instantiating DefaultRoomReactions

The await keyword has been removed when instantiating DefaultRoomReactions, indicating that the constructor is now synchronous. This change is consistent with the PR's goal of simplifying the API by making operations synchronous when possible, while still maintaining proper concurrency control through @MainActor.

Also applies to: 42-42, 58-58

28-30: Removed await when accessing channel properties

The await keyword has been removed when accessing channel properties, indicating that these properties can now be accessed synchronously. This simplifies the testing code and aligns with the PR's objective of making the SDK's state management more straightforward.

45-45: Removed await when calling subscribe()

The await keyword has been removed when calling defaultRoomReactions.subscribe(), indicating that this method is now synchronous. This change matches the updated method signature in DefaultRoomReactions, making the API simpler for users.

62-63: Removed await when working with discontinuity events

The await keywords have been removed when calling roomReactions.onDiscontinuity() and featureChannel.emitDiscontinuity(), making these operations synchronous. This is part of the overall strategy to simplify the API by removing unnecessary asynchronous operations.

66-66: Added @sendable attribute to closure

The @Sendable attribute has been added to the closure in first { @Sendable _ in true }. This is necessary because the closure is used in an asynchronous context (the await messagesDiscontinuitySubscription.first call), ensuring it can be safely used across actor boundaries.

Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (8)

5-5: Good use of @MainActor to enforce the new concurrency model

This annotation ensures all methods within the struct execute on the main actor, aligning with the PR's goal of isolating mutable state to the main actor.

62-63: Good removal of unnecessary async modifier

Removing the async modifier from the createManager function makes the API simpler and more straightforward, in line with the PR's objective to eliminate unnecessary asynchronous operations.

95-95: Properly switched to synchronous access for subscription

The removal of await aligns with the change to make testsOnly_subscribeToHandledContributorStateChanges() a synchronous method on the main actor.

103-103: Properly switched to synchronous access for subscription

Similar to the previous change, removing await reflects the method's transition to synchronous execution on the main actor.

395-396: Good addition of @Sendable annotations

Adding @Sendable annotations to closures passed to async methods is good practice, ensuring proper type-checking for concurrent contexts.

Also applies to: 508-509, 1333-1334, 1643-1645, 2060-2061, 2075-2077, 2106-2106

187-187: Successfully migrated all subscription methods to synchronous access

All subscription method calls have been properly converted from async to synchronous access, maintaining consistent behavior while simplifying the API.

Also applies to: 195-195, 223-223, 246-246, 352-352, 355-355, 438-438, 489-489, 597-597, 620-620, 657-657, 703-703, 747-747, 776-776, 784-784, 822-822, 857-857, 925-925, 1062-1062, 1150-1150, 1221-1221, 1267-1267, 1315-1315, 1318-1318, 1641-1641, 1715-1715, 1811-1811, 1923-1923, 2016-2016, 2019-2019, 2100-2100, 2109-2109

117-117: Successfully migrated all property and method accesses to synchronous

All property accesses and method calls have been properly converted from async to synchronous access, in line with the migration to @MainActor.

Also applies to: 133-133, 232-232, 254-254, 258-258, 283-283, 289-289, 307-307, 367-367, 376-376, 402-408, 450-450, 459-459, 512-514, 530-530, 605-606, 628-628, 632-632, 673-673, 710-710, 713-713, 731-731, 755-756, 805-805, 830-831, 869-869, 872-872, 876-876, 905-905, 908-908, 936-936, 939-939, 943-943, 982-984, 1035-1038, 1071-1073, 1075-1075, 1084-1084, 1164-1164, 1172-1172, 1233-1233, 1275-1275, 1279-1279, 1337-1339, 1341-1343, 1346-1346, 1384-1386, 1414-1414, 1454-1455, 1479-1480, 1521-1521, 1567-1567, 1609-1609, 1661-1661, 1692-1692, 1736-1736, 1739-1739, 1742-1742, 1755-1755, 1770-1770, 1807-1807, 1823-1823, 1859-1859, 1861-1861, 1890-1890, 1905-1906, 1939-1939, 1971-1971, 2041-2043, 2067-2073

1-2211: Overall excellent conversion to @MainActor model

The changes in this file successfully implement the PR's goal of converting the SDK to use @MainActor for state management. The modifications:

  1. Make the test struct @MainActor-annotated
  2. Remove all unnecessary async modifiers from methods
  3. Convert all awaited property accesses and method calls to direct synchronous ones
  4. Add appropriate @Sendable annotations for closures

These changes simplify the code, make it more straightforward, and maintain the same behavior while properly isolating mutable state to the main actor.

Sources/AblyChat/Rooms.swift (6)

6-6: Adopting @MainActor on Rooms
Adding @MainActor ensures all calls occur on the main actor, simplifying concurrency management for UI-related code.

51-51: nonisolated property for clientOptions
Marking clientOptions as nonisolated makes it safely accessible from any thread, aligning with your concurrency approach if it's a read-only value.

54-54: Transition from actor to class
Switching to a class while adopting @MainActor for concurrency can simplify design while maintaining isolation, provided all mutable state is still accessed on the main actor.

238-238: Removed await inside get(roomID:options:)
Switching to a synchronous call confirms there is no longer a need for async. Verify that this does not introduce blocking behavior on the main actor.

295-295: createRoom no longer async
Removing async clarifies that this operation is purely synchronous. Ensure the room creation process doesn’t inadvertently block the main actor for long-running tasks.

347-347: Reworded comment referencing MainActor
The updated comment clarifies that this code runs under @MainActor despite not being an actor. This is consistent with the current concurrency model.

Sources/AblyChat/Presence.swift (7)

11-11: Added @MainActor to Presence
Ensures all presence operations execute on the main actor, aligning with the SDK’s overarching concurrency strategy.

81-81: Dropped async from subscribe(event:bufferingPolicy:)
Returning a Subscription<PresenceEvent> directly simplifies usage. Confirm no async initialization is required behind the scenes.

92-92: Removed async from subscribe(events:bufferingPolicy:)
Similarly, returning the subscription without async aligns with the new synchronous approach. Verify that background setup isn’t needed here.

121-121: Synchronous overload of subscribe(event:)
Consistent with other subscription methods. Makes usage straightforward in synchronous contexts.

126-126: Synchronous overload of subscribe(events:)
Again, removing async provides consistency across the presence API.

130-131: Default single-event subscription
Uses .unbounded as the default buffering policy. This is a logical fallback and mirrors the newly synchronous pattern.

134-135: Default multi-event subscription
Provides .unbounded buffering by default, matching the single-event subscription approach for consistency.

Sources/AblyChat/RoomLifecycleManager.swift (16)

7-14: MainActor isolation added to RoomLifecycleContributor protocol with synchronous emitDiscontinuity method.

The protocol is now properly isolated to the MainActor and the emitDiscontinuity method has been changed from async to synchronous, which aligns with the PR objective of isolating mutable state to the MainActor.

17-23: MainActor isolation added to RoomLifecycleManager with synchronous room status access.

The protocol now properly isolates its functionality to the MainActor and provides synchronous access to the room status and status change subscription. This change aligns with the PR objective of simplifying the API by removing unnecessary async operations for state access.

27-35: MainActor isolation added to RoomLifecycleManagerFactory with synchronous factory method.

The factory protocol is now properly isolated to the MainActor and the createManager method has been made synchronous, allowing for simpler object creation without awaiting. This is consistent with the overall approach of isolating mutable state to the MainActor.

53-53: Converted DefaultRoomLifecycleManager from actor to MainActor-isolated class.

The class has been changed from an actor to a regular class, which is consistent with the isolation approach using @MainActor. This change simplifies the concurrency model by centralizing isolation to the MainActor instead of using separate actor instances.

71-84: Synchronous convenience initializer for DefaultRoomLifecycleManager.

The initializer has been made synchronous by removing the async keyword, which aligns with the MainActor isolation pattern. Since all initialization work is now performed on the MainActor, asynchronous initialization is no longer needed.

87-103: Synchronous test-specific convenience initializer for DefaultRoomLifecycleManager.

Similar to the regular convenience initializer, the test-specific initializer has been made synchronous by removing the async keyword, maintaining consistency with the MainActor isolation pattern.

124-124: Synchronous subscription to channel state.

The channel state subscription is now performed synchronously, which is possible because the code is executing on the MainActor. This simplifies the initialization code by removing the need to await subscriptions.

410-410: Synchronous discontinuity event emission.

The emitDiscontinuity method call is now synchronous, aligning with the updated protocol declaration. This simplifies the code by removing unnecessary asynchronous operations for event emission.

425-426: Using async sequence filtering directly in conditional check.

This code uses contributors.async.map with an allSatisfy operation directly in the conditional check. This is a more elegant way to handle async iteration while still using await appropriately for the asynchronous operation.

463-464: Clarified MainActor execution order in comments.

The comment explains how tasks created within MainActor-isolated code will execute in a predictable order, providing important context for why the .suspendedAwaitingStartOfRetryOperation and .suspended statuses will occur in the expected sequence.

773-775: Clarified MainActor execution order in comments for retry operation.

Similar to the previous comment, this explains the expected execution order for task creation within MainActor-isolated code, providing important context about the status transition sequence.

787-788: Clarified MainActor execution order in comments for rundown operation.

Another explanatory comment about task execution order within MainActor-isolated code, providing consistent documentation about status transition sequences throughout the codebase.

812-816: Converted emitPendingDiscontinuityEvents to synchronous method.

The method has been made synchronous, which is appropriate since it now runs on the MainActor and calls the synchronous emitDiscontinuity method. This is consistent with the overall approach of making state operations synchronous when isolated to the MainActor.

822-822: Synchronous discontinuity event emission in loop.

Similar to line 410, this call to emitDiscontinuity is now synchronous, aligning with the updated protocol declaration and simplifying the code.

1203-1203: Simplified async sequence consumption.

The code now uses a more concise pattern for awaiting the first element of an async sequence, using the first method with a closure that always returns true. This improves readability while maintaining the same functionality.

626-627: Clarified understanding of MainActor isolation in comments.

The comment explains the author's understanding of how MainActor isolation affects method suspension and continuation handling, which provides valuable context for understanding the code's concurrency behavior. This kind of documentation is essential when working with Swift's concurrency model.

Sources/AblyChat/Room.swift (20)

6-13: MainActor isolation added to Room protocol with nonisolated roomID property.

The Room protocol is now properly isolated to the MainActor, and the roomID property is marked as nonisolated to allow access from any context without requiring MainActor isolation. This is appropriate since the room ID is immutable and can be safely accessed from any thread.

20-20: Marked messages property as nonisolated.

The messages property is now marked as nonisolated, allowing it to be accessed from any context without MainActor isolation. This is appropriate since the property returns an instance conforming to the Messages protocol, which should handle its own thread-safety concerns.

29-29: Marked presence property as nonisolated.

The presence property is now marked as nonisolated, allowing it to be accessed from any context without MainActor isolation. This improves API usability while delegating thread-safety concerns to the Presence protocol implementation.

38-38: Marked reactions property as nonisolated.

The reactions property is now marked as nonisolated, allowing it to be accessed from any context. This is consistent with the treatment of other feature-specific properties in the protocol.

47-47: Marked typing property as nonisolated.

The typing property is now marked as nonisolated, allowing it to be accessed from any context. This provides a more convenient API for clients while pushing thread-safety concerns to the implementation.

56-56: Marked occupancy property as nonisolated.

The occupancy property is now marked as nonisolated, consistent with the other feature-specific properties in the protocol. This approach simplifies API usage by allowing direct access without MainActor switching.

63-63: Converted status property to synchronous access.

The status property now provides synchronous access to the room status without requiring await. This aligns with the PR objective of simplifying the API and improving usability while maintaining thread safety through MainActor isolation.

73-73: Converted onStatusChange method to synchronous.

The onStatusChange method now returns a subscription synchronously without requiring await. This provides a more straightforward API for subscribing to status changes while maintaining thread safety through MainActor isolation.

78-78: Converted default onStatusChange implementation to synchronous.

The default implementation of onStatusChange() is now synchronous, consistent with the protocol declaration. This ensures that the default implementation matches the updated protocol signature.

106-106: Marked options property as nonisolated.

The options property is now marked as nonisolated, allowing it to be accessed from any context. This is appropriate since room options are immutable after room creation and can be safely accessed from any thread.

110-112: Updated default onStatusChange implementation.

The default implementation has been updated to be synchronous, directly calling the synchronous onStatusChange(bufferingPolicy:) method without awaiting. This maintains consistency with the updated protocol declarations.

140-144: MainActor isolation added to RoomFactory with synchronous createRoom method.

The RoomFactory protocol is now properly isolated to the MainActor and the createRoom method has been made synchronous. This simplifies room creation while maintaining thread safety through MainActor isolation.

150-159: Converted createRoom implementation to synchronous.

The implementation of createRoom in DefaultRoomFactory is now synchronous, directly calling the synchronous DefaultRoom initializer without awaiting. This is consistent with the updated protocol declaration and improves code simplicity.

162-162: Converted DefaultRoom from actor to MainActor-isolated class.

The DefaultRoom class has been changed from an actor to a regular class. Since the class is used in a context that's isolated to the MainActor, this change simplifies the concurrency model while maintaining thread safety.

226-226: Converted DefaultRoom initializer to synchronous.

The initializer has been made synchronous by removing the async keyword. This is appropriate since all initialization work is now performed on the MainActor, eliminating the need for asynchronous initialization.

243-246: Synchronous lifecycle manager creation.

The code now creates the lifecycle manager synchronously, which is possible because the factory method is now synchronous when isolated to the MainActor. This improves code readability by removing unnecessary awaiting.

250-256: Synchronous DefaultMessages initialization.

The DefaultMessages instance is now created synchronously, which is appropriate since its initializer no longer needs to be asynchronous when isolated to the MainActor. This is consistent with the overall approach of simplifying the API through MainActor isolation.

259-264: Synchronous DefaultRoomReactions initialization.

Similar to DefaultMessages, the DefaultRoomReactions instance is now created synchronously. This maintains consistency across all feature implementations.

429-430: Converted onStatusChange implementation to synchronous.

The implementation of onStatusChange in DefaultRoom is now synchronous, directly calling the synchronous onRoomStatusChange method on the lifecycle manager without awaiting. This is consistent with the updated protocol declaration.

433-435: Converted status property implementation to synchronous.

The status property implementation in DefaultRoom is now synchronous, directly accessing the synchronous roomStatus property of the lifecycle manager without awaiting. This improves API usability while maintaining thread safety through MainActor isolation.

@lawrence-forooghian lawrence-forooghian mentioned this pull request Apr 3, 2025
Merged
maratal
maratal approved these changes Apr 4, 2025
View reviewed changes
Copy link
Collaborator

@maratal maratal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@lawrence-forooghian
Remove unnecessary @mainactor
6f4cd72 
SwiftUI views are automatically @mainactor.
@lawrence-forooghian
Remove unnecessary variable in example app
7444c39
@lawrence-forooghian
Get rid of tryTask modifier in example app
3be63df 
It doesn't copy the @_inheritActorContext attribute that the underlying
SwiftUI `task` modifier uses, meaning that when we pass it a closure
whose isolation matters (which I will do in #232 when we make the SDK
@mainactor) it doesn't behave as we'd wish.
@lawrence-forooghian
Convert public API and internals to @mainactor
fc83fc1 
In 7d6acde we made the decision to use `actor` types to manage the
mutable state of the SDK. Whilst we've been able to build the SDK using
this approach, it has proved cumbersome.

The public API is littered with `async` methods and getters for things
that really should be synchronous, such as adding a subscription or
fetching a room's state. This deviates from the JavaScript API and makes
call sites look complicated.

The use of `async` for fetching room state also means that it's
impossible for a user to, for example, check a room's state and then
reason about what operations they should be able to attempt to perform
on the room (i.e. without the SDK immediately rejecting the attempt),
because the room state can change under their feet. This is one of a
class of threading-related problems that I describe in #260.

Internally, we find ourselves having to write `await` to do basic things
like fetching the room lifecycle manager's state or to find out the
arguments with which a mock method was called. And similarly, one
internal component can't rely on the state that's isolated to a
different internal isolation domain in order to make a decision, because
this state might change under its feet.

To address this, here I have decided to isolate all of the SDK's mutable
state to the main actor. This simplifies the experience of developing
the SDK, and it simplifies the experience of interacting with it from
the main thread, which is where most of our users interact with it from.

(Note that this does not prevent us from using background tasks
internally for moving heavy processing off the main thread; I'm
specifically talking about _state_.)

This also moves us closer to being able to address #49 (keeping our
internal state in sync with that of the underlying ably-cocoa objects).

Note that this change does decrease the parallelisability of our test
suite. But it still runs very fast.

Have removed:

- all `actor` types (both in SDK and mocks)
- nearly all locking (there are a couple of non-MainActor types for
  which we still have it)

There might be some simplifications that can be made for the lifecycle
manager tests now, but I'm not going to look at them now because that
class will change a lot for the single-channel work in #242.

I'll update the ably.com documentation in #254.

Even after this change, Messages.subscribe is still `async` because its
implementation requires it to be. I believe that this is a bug; have
created #257 for it.

Resolves #232.
coderabbitai[bot]
coderabbitai bot reviewed Apr 7, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
Sources/AblyChat/DefaultConnection.swift (1)

84-84: 🛠️ Refactor suggestion

Consider making the termination handler itself @mainactor.

Instead of wrapping the code in a Task with the @mainactor attribute, consider making the entire closure @mainactor:

-subscription.addTerminationHandler { [weak self] in
-    Task { @MainActor in
-        self?.realtime.connection.off(eventListener)
-    }
+subscription.addTerminationHandler { @MainActor [weak self] in
+    self?.realtime.connection.off(eventListener)
}

This would be more consistent with the approach of isolating operations to the main actor and eliminate the need for the Task wrapper.

🧹 Nitpick comments (1)
Sources/AblyChat/SubscriptionStorage.swift (1)

44-45: Consider pruning nil references while emitting.
Currently, weak subscription references that have become nil remain in the dictionary. You might optionally remove them to keep it uncluttered.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 6cf993e and fc83fc1.

📒 Files selected for processing (54)
  • CONTRIBUTING.md (2 hunks)
  • Example/AblyChatExample/ContentView.swift (9 hunks)
  • Example/AblyChatExample/Mocks/MockClients.swift (11 hunks)
  • Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1 hunks)
  • README.md (1 hunks)
  • Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (6 hunks)
  • Sources/AblyChat/ChatAPI.swift (1 hunks)
  • Sources/AblyChat/ChatClient.swift (4 hunks)
  • Sources/AblyChat/Connection.swift (1 hunks)
  • Sources/AblyChat/DefaultConnection.swift (2 hunks)
  • Sources/AblyChat/DefaultMessages.swift (6 hunks)
  • Sources/AblyChat/DefaultOccupancy.swift (4 hunks)
  • Sources/AblyChat/DefaultPresence.swift (3 hunks)
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (2 hunks)
  • Sources/AblyChat/DefaultTyping.swift (10 hunks)
  • Sources/AblyChat/EmitsDiscontinuities.swift (2 hunks)
  • Sources/AblyChat/Messages.swift (3 hunks)
  • Sources/AblyChat/Occupancy.swift (3 hunks)
  • Sources/AblyChat/Presence.swift (4 hunks)
  • Sources/AblyChat/Room.swift (14 hunks)
  • Sources/AblyChat/RoomFeature.swift (2 hunks)
  • Sources/AblyChat/RoomLifecycleManager.swift (15 hunks)
  • Sources/AblyChat/RoomReactions.swift (3 hunks)
  • Sources/AblyChat/Rooms.swift (6 hunks)
  • Sources/AblyChat/SimpleClock.swift (1 hunks)
  • Sources/AblyChat/Subscription.swift (2 hunks)
  • Sources/AblyChat/SubscriptionStorage.swift (1 hunks)
  • Sources/AblyChat/TimerManager.swift (1 hunks)
  • Sources/AblyChat/Typing.swift (3 hunks)
  • Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultChatClientTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultMessagesTests.swift (7 hunks)
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (100 hunks)
  • Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (3 hunks)
  • Tests/AblyChatTests/DefaultRoomReactionsTests.swift (5 hunks)
  • Tests/AblyChatTests/DefaultRoomTests.swift (15 hunks)
  • Tests/AblyChatTests/DefaultRoomsTests.swift (12 hunks)
  • Tests/AblyChatTests/IntegrationTests.swift (14 hunks)
  • Tests/AblyChatTests/Mocks/MockChannels.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockConnection.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockInternalRealtimeClientFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtime.swift (3 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoom.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockSimpleClock.swift (1 hunks)
  • Tests/AblyChatTests/SubscriptionStorageTests.swift (3 hunks)
  • Tests/AblyChatTests/SubscriptionTests.swift (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
  • Tests/AblyChatTests/DefaultChatClientTests.swift
  • Tests/AblyChatTests/DefaultMessagesTests.swift
  • Tests/AblyChatTests/ChatAPITests.swift
  • Tests/AblyChatTests/SubscriptionStorageTests.swift
  • Tests/AblyChatTests/DefaultRoomTests.swift
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift
  • README.md
  • Sources/AblyChat/SimpleClock.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
  • Tests/AblyChatTests/IntegrationTests.swift
  • Tests/AblyChatTests/Mocks/MockInternalRealtimeClientFactory.swift
  • Sources/AblyChat/TimerManager.swift
  • Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift
  • Sources/AblyChat/ChatAPI.swift
  • Sources/AblyChat/Connection.swift
  • Sources/AblyChat/EmitsDiscontinuities.swift
  • Sources/AblyChat/Subscription.swift
  • Tests/AblyChatTests/SubscriptionTests.swift
  • Sources/AblyChat/Occupancy.swift
  • Sources/AblyChat/Typing.swift
  • Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
  • Sources/AblyChat/RoomReactions.swift
  • Sources/AblyChat/DefaultRoomReactions.swift
  • Tests/AblyChatTests/Mocks/MockSimpleClock.swift
  • Sources/AblyChat/DefaultTyping.swift
  • Sources/AblyChat/RoomFeature.swift
  • Sources/AblyChat/DefaultPresence.swift
  • Sources/AblyChat/ChatClient.swift
  • Sources/AblyChat/DefaultMessages.swift
  • Sources/AblyChat/Rooms.swift
  • Sources/AblyChat/DefaultOccupancy.swift
  • Example/AblyChatExample/Mocks/MockClients.swift
  • Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
  • Example/AblyChatExample/ContentView.swift
  • Sources/AblyChat/Room.swift
🧰 Additional context used
🧬 Code Definitions (9)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (6)
Sources/AblyChat/DefaultMessages.swift (4)
  • subscribe (28-30)
  • subscribe (76-171)
  • onDiscontinuity (48-50)
  • onDiscontinuity (207-209)
Sources/AblyChat/DefaultRoomReactions.swift (4)
  • subscribe (26-28)
  • subscribe (69-121)
  • onDiscontinuity (30-32)
  • onDiscontinuity (124-126)
Sources/AblyChat/RoomReactions.swift (1)
  • subscribe (45-47)
Sources/AblyChat/EmitsDiscontinuities.swift (1)
  • onDiscontinuity (34-36)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (15-17)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
  • emitDiscontinuity (21-23)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
  • onRoomStatusChange (297-299)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (7)
Sources/AblyChat/DefaultOccupancy.swift (2)
  • onDiscontinuity (24-26)
  • onDiscontinuity (91-93)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • onDiscontinuity (19-21)
Sources/AblyChat/DefaultMessages.swift (2)
  • onDiscontinuity (48-50)
  • onDiscontinuity (207-209)
Sources/AblyChat/DefaultRoomReactions.swift (2)
  • onDiscontinuity (30-32)
  • onDiscontinuity (124-126)
Sources/AblyChat/DefaultTyping.swift (2)
  • onDiscontinuity (32-34)
  • onDiscontinuity (211-213)
Sources/AblyChat/RoomFeature.swift (1)
  • onDiscontinuity (69-71)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
  • onDiscontinuity (156-173)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
  • createManager (41-50)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
  • createManager (56-71)
Tests/AblyChatTests/DefaultRoomsTests.swift (6)
Sources/AblyChat/Rooms.swift (3)
  • testsOnly_hasRoomMapEntryWithID (303-313)
  • release (316-354)
  • testsOnly_subscribeToOperationWaitEvents (145-147)
Sources/AblyChat/Room.swift (1)
  • release (418-425)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
  • release (268-290)
Tests/AblyChatTests/Mocks/MockRoom.swift (1)
  • release (55-62)
Sources/AblyChat/RoomLifecycleManager.swift (1)
  • testsOnly_subscribeToOperationWaitEvents (581-583)
Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1)
  • setRoom (12-14)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (4)
  • testsOnly_subscribeToHandledContributorStateChanges (350-352)
  • testsOnly_subscribeToHandledTransientDisconnectTimeouts (378-380)
  • onRoomStatusChange (297-299)
  • testsOnly_subscribeToOperationWaitEvents (581-583)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
  • onRoomStatusChange (54-56)
Sources/AblyChat/Presence.swift (1)
Sources/AblyChat/DefaultPresence.swift (4)
  • subscribe (52-54)
  • subscribe (56-58)
  • subscribe (261-282)
  • subscribe (284-311)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (10)
  • subscribe (243-245)
  • subscribe (352-354)
  • subscribe (356-358)
  • unsubscribe (235-237)
  • unsubscribe (360-362)
  • on (227-229)
  • on (231-233)
  • on (396-398)
  • off (247-249)
  • off (400-402)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (2)
  • subscribe (5-7)
  • unsubscribe (13-15)
Tests/AblyChatTests/Mocks/MockConnection.swift (2)
  • on (14-16)
  • off (18-20)
Sources/AblyChat/RoomLifecycleManager.swift (7)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (15-17)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)
  • emitDiscontinuity (19-21)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
  • onRoomStatusChange (54-56)
  • waitToBeAbleToPerformPresenceOperations (62-64)
Sources/AblyChat/RoomFeature.swift (1)
  • waitToBeAbleToPerformPresenceOperations (73-75)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1)
  • createManager (11-14)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
  • status (294-312)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
  • subscribeToState (193-204)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Example app, tvOS (Xcode 16.2)
  • GitHub Check: Generate code coverage (Xcode 16.2)
  • GitHub Check: Xcode, tvOS (Xcode 16.2)
  • GitHub Check: Xcode, iOS (Xcode 16.2)
  • GitHub Check: Xcode, macOS (Xcode 16.2)
🔇 Additional comments (69)
Tests/AblyChatTests/Mocks/MockRoomFactory.swift (2)

3-3: Correct actor to class conversion.

The change from actor to class aligns with the PR's objective of moving mutable state management to the main actor instead of using distributed actors. This simplifies the concurrency model as part of the broader refactoring strategy.

16-16: Appropriate removal of async keyword.

Removing the async keyword from the method signature matches the changes in the RoomFactory protocol and reflects the project's goal to simplify the API by making operations synchronous where possible. This change also maintains consistency with the refactored concurrency model across the codebase.

Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (3)

4-4: Conversion from actor to class aligns with PR objectives

This change is part of the broader refactoring to isolate mutable state to the main actor rather than using multiple actors. Since this is a mock class used in tests, ensure all test code that uses this mock has been properly updated to work with the new concurrency model.

17-17: Method signature updated to remove async keyword

This change aligns with the PR objective of simplifying the API by removing unnecessary asynchronous patterns. The method now returns a Subscription<DiscontinuityEvent> synchronously, which is consistent with similar changes in other components like DefaultMessages, DefaultOccupancy, etc.

4-36: Verify thread safety with actor-to-class conversion

With the removal of the actor-based protection, ensure that this mock class is only accessed from the main thread in tests or is otherwise protected from concurrent access if used from multiple threads. Consider adding a comment documenting this requirement for future maintainers.

Tests/AblyChatTests/Mocks/MockChannels.swift (4)

4-4: Approved: Removal of @unchecked Sendable aligns with the PR objectives.

This change correctly removes the @unchecked Sendable marker from the class declaration, which aligns with the PR's goal of moving away from actor types and isolated state management toward the @MainActor pattern. This simplifies the concurrency model by relying on the main actor for thread safety instead of manual sendability checks.

6-7: Simplified property access by removing underscore prefix and mutex.

The properties have been renamed from _getArguments and _releaseArguments to getArguments and releaseArguments, removing the underscore prefix while maintaining the private(set) access level. This change, coupled with the removal of a mutex lock (not visible in the diff but mentioned in the summary), indicates that these properties will now be safely accessed through the main actor, eliminating the need for explicit synchronization.

14-14: Removed mutex-based synchronization for direct property access.

The method now directly appends to the getArguments array without mutex protection. This is safe because the class will be accessed through the @MainActor, ensuring all interactions happen on the main thread, thus eliminating the need for explicit synchronization.

24-24: Removed mutex-based synchronization for direct property access.

Similar to the get method, this implementation now directly appends to the releaseArguments array without mutex protection. This change simplifies the code and is safe within the context of the @MainActor pattern being implemented across the SDK.

Sources/AblyChat/DefaultConnection.swift (5)

8-10: Good conversion to synchronous properties.

The change from an actor-based approach to direct properties with internal private(set) properly isolates the mutable state to the main thread while maintaining encapsulation.

15-16: Clean initialization of connection properties.

The direct initialization of status and error properties simplifies the code by removing the need for asynchronous calls during initialization.

41-41: Successful conversion to synchronous timer check.

The removal of await from timerManager.hasRunningTask() aligns with the goal of making state operations synchronous.

45-70: Well-implemented transient disconnect handling.

The transient disconnect timer logic has been properly updated to use synchronous property updates. The conditions and state transitions follow the requirements properly.

79-80: Effective direct state updates.

Direct updates to the status and error properties remove the dependency on an actor and properly handle state transitions.

CONTRIBUTING.md (1)

42-42: Highlighting main actor isolation is a great clarity boost.
This new guideline explicitly stating that the SDK’s mutable state should be isolated to the main actor aligns well with Swift’s concurrency philosophy, helping contributors write safer, more consistent code.

Sources/AblyChat/SubscriptionStorage.swift (5)

6-7: Adopting @MainActor is a solid concurrency safety measure.
Marking the class with @MainActor removes the manual locking overhead, ensuring all mutable state is consistently updated on the main actor.

21-21: Storing subscriptions in the dictionary on the main actor.
Directly adding the subscription to the dictionary is safe under main actor isolation.

24-26: Terminating subscription with Task { @MainActor in ... } ensures concurrency correctness.
Scheduling the termination handler on the main actor is consistent with the class’s concurrency model.

34-34: Returning subscriptions.count under @mainactor is correct.
This approach uses main actor isolation instead of locks, which is succinct and safe.

39-39: Removing subscription from the dictionary under main actor is clean.
Eliminating locks and relying on the main actor is straightforward and avoids race conditions.

Sources/AblyChat/Presence.swift (6)

11-11: Marking the protocol with @MainActor boosts safety.
This is consistent with the SDK’s practice of isolating mutable state on the main actor.

130-131: Default implementation referencing the new synchronous subscription.
Straightforward call to the unbounded version; no concerns.

134-135: Consistent synchronous approach for multiple-event subscription.
Similarly references the unbounded overload; looks good.

126-126:

❓ Verification inconclusive

Confirm multiple-event subscription no longer requiring await.
Likewise, verify no references are expecting async usage here.

🏁 Script executed:

#!/bin/bash
rg -A3 "await\s+.*subscribe\(events:\)"

Length of output: 41

Action: Verify that all subscription calls are fully synchronous and no callers use await on subscribe(events:).

The refactored method in Sources/AblyChat/Presence.swift (line 126) now returns a synchronous Subscription<PresenceEvent>, and our initial search for await subscribe(events:) produced no output. This suggests that no call sites are mistakenly expecting an async call. However, please manually review any code that calls this API (and associated tests) to confirm that there are no lingering assumptions requiring asynchronous usage.

121-121:

✅ Verification successful

Transition from async -> Subscription to synchronous.
Make sure no external code was relying on an await call for single-event subscription.

🏁 Script executed:

#!/bin/bash
rg -A3 "await\s+.*subscribe\(event:\)"

Length of output: 40

I'm not fully confident with the initial search result. To be sure no external code is mistakenly relying on asynchronous usage of the subscribe call, please run an additional, slightly modified verification:

🏁 Script executed:

#!/bin/bash
# Additional check: Searching for any asynchronous usage of subscribe calls with "await".
rg -A3 "await\s+subscribe\s*\("

Length of output: 227

Review Confirmation: Transition Verified

After verifying the asynchronous usage of the subscribe method, we found no instances of await subscribe(event:) anywhere in the repository. Although an asynchronous call (try await subscribe(bufferingPolicy: .unbounded)) exists in Sources/AblyChat/Messages.swift, it pertains to a different subscription method. Therefore, the synchronous change for Presence.subscribe(event:) is isolated and safe.

81-81:

✅ Verification successful

Confirm no external calls rely on the old async signature.
Changing this to a synchronous method might break code expecting await.

Run this search to verify there are no references depending on async calls to subscribe(event:bufferingPolicy:):

🏁 Script executed:

#!/bin/bash
rg -A3 "await\s+.*subscribe\(event:"

Length of output: 38

I'll run an additional couple of searches to confirm there are no external calls using an await with this method. In particular, I want to check both for any await subscribe(event: usage and list all occurrences of subscribe(event: to inspect their context.

Please run these additional searches and review the results. If they confirm that no external call is using await subscribe(event:, you can safely proceed with converting the method to synchronous.

🏁 Script executed:

#!/bin/bash
# 1. Search for any asynchronous calls (using 'await') on subscribe(event:)
rg -n -A3 "await\s+subscribe\(event:" --color always

# 2. List all occurrences of subscribe(event:) to verify how it's used
rg -n -A3 "subscribe\(event:" --color always

Length of output: 3558

Synchronous Signature Change Confirmed
The additional search results show no usage of an await on calls to subscribe(event:. All detected usages are synchronous, so converting the method signature from an async form to a synchronous one should not impact any external callers.

Sources/AblyChat/Messages.swift (3)

9-9: Properly isolates the Messages protocol to the main actor.

This change ensures that all implementations of the Messages protocol are executed on the main actor, aligning with the SDK's new guideline of isolating all mutable state to the main actor.

86-86: Correctly marks channel property as nonisolated.

The nonisolated keyword allows this property to be accessed from any actor without requiring the caller to be on the main actor. This is appropriate for a property that provides access to the underlying channel, which may need to be accessed from various concurrency contexts.

308-309: Ensures termination handlers are executed on the main actor.

Adding the @MainActor attribute to the addTerminationHandler method ensures that termination handlers are properly executed on the main actor, which is critical for UI updates and state management.

Sources/AblyChat/RoomLifecycleManager.swift (9)

7-8: Properly isolates the RoomLifecycleContributor protocol to the main actor.

The @MainActor annotation ensures that all implementations will be executed on the main actor, and making emitDiscontinuity synchronous simplifies the interaction model. This aligns with the relevant implementations in DefaultRoomLifecycleContributor.swift.

Also applies to: 14-14

17-18: Isolates RoomLifecycleManager protocol and removes unnecessary async.

Making the protocol @MainActor annotated and removing async from the roomStatus property and onRoomStatusChange method simplifies the API while maintaining thread safety through main actor isolation.

Also applies to: 22-23

27-28: Isolates the RoomLifecycleManagerFactory protocol to the main actor.

Making the protocol @MainActor annotated and removing async from the createManager method return type ensures that factory operations happen on the main actor, maintaining a consistent concurrency model.

Also applies to: 35-35

53-53: Converts DefaultRoomLifecycleManager from an actor to a class.

This change is significant as it shifts from Swift's actor isolation model to explicit main actor isolation. This aligns with the PR's goal to isolate all mutable state to the main actor rather than using distributed actor types.

124-124: Removes await for channel subscriptions under main actor.

Now that the class is main actor isolated, the channel subscriptions can be done synchronously, simplifying the code while maintaining thread safety guarantees.

626-627: Updates comment to accurately reflect main actor concurrency model.

The comment has been updated to explain the behavior of MainActor-isolated calls within the same actor, which is important for understanding the execution flow in this new concurrency model.

816-816: Makes emitPendingDiscontinuityEvents synchronous.

With the shift to main actor isolation, this method no longer needs to be asynchronous, simplifying the control flow while maintaining thread safety.

822-822: Makes emitDiscontinuity call synchronous.

Now that the emitDiscontinuity method on the contributor is synchronous and main actor isolated, the call no longer needs to use await.

1127-1127: Simplifies channel state subscription iteration.

Removes the await keyword since the context is now main actor isolated, simplifying the iteration over state changes.

Tests/AblyChatTests/Mocks/MockConnection.swift (2)

4-4: Removes unnecessary NSObject inheritance.

Simplifies the mock class by removing inheritance from NSObject while maintaining the essential InternalConnectionProtocol conformance.

14-14: Updates callback to be executed on the main actor.

Adding @MainActor to the closure parameter ensures that any state changes from connection events are processed on the main actor, maintaining thread safety.

Tests/AblyChatTests/DefaultRoomsTests.swift (5)

5-5: Properly isolates test struct to the main actor.

Adding @MainActor to the test struct ensures that all test methods run on the main actor, which is necessary for interacting with the now main actor-isolated SDK components.

56-58: Simplifies property and method access with main actor isolation.

With the test struct now marked as @MainActor, these calls no longer need to use await since they're executed on the same actor as the subject under test.

109-110: Correctly marks closures as @Sendable in async contexts.

Adding the @Sendable attribute to closures used in asynchronous contexts ensures they can be safely used across different execution contexts, which is required by Swift's concurrency model.

Also applies to: 115-117, 123-125, 186-187, 192-194, 235-236, 242-244, 295-296, 302-304, 338-339, 344-346, 402-403

111-111: Makes subscription calls synchronous with main actor isolation.

These calls to obtain subscriptions can now be made synchronously since they're executed on the same actor as the subject under test.

Also applies to: 188-188, 238-238, 298-298, 339-339

313-313: Simplifies test assertions with main actor isolation.

With the test struct now marked as @MainActor, these assertions no longer need to use await since they're executed on the same actor as the subject under test.

Also applies to: 369-369, 405-405

Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (2)

3-3: Clean conversion from actor to class

This change aligns with the PR objective of removing actors and simplifying the concurrency model. The factory no longer needs to be an actor since its state can be safely managed on the main thread.

11-11: Correct removal of async modifier

The createManager method signature has been properly updated to remove the async keyword, making it synchronous. This matches the implementation in the real RoomLifecycleManagerFactory and simplifies the API by reducing unnecessary asynchronous calls.

Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (2)

5-5: Improved thread safety with @mainactor

The callback type has been updated to explicitly use @MainActor, ensuring that presence subscription callbacks will be executed on the main thread. This is a positive change that improves thread safety when handling presence events.

9-9: Improved thread safety with @mainactor

Similarly, this action-specific subscribe method now properly ensures that callbacks run on the main thread with the @MainActor attribute, maintaining consistency with the other subscribe method.

Tests/AblyChatTests/Mocks/MockRoom.swift (2)

4-4: Clean conversion from actor to class

This change aligns with the PR objective of simplifying the concurrency model by isolating mutable state to the main actor instead of using actor-based isolation.

43-43: Appropriate removal of async modifier

The onStatusChange method has been correctly updated to be synchronous by removing the async keyword. This simplifies the API while still returning a subscription that can be used to observe status changes asynchronously.

Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)

4-4: Clean conversion from actor to class

This change aligns with the PR objective of removing actors and simplifying the concurrency model by isolating mutable state to the main actor.

54-54: Appropriate removal of async modifier

The onRoomStatusChange method has been correctly updated to be synchronous by removing the async keyword. This matches the implementation in the real RoomLifecycleManager and simplifies the subscription API while maintaining appropriate asynchronous operations for the actual attachment, detachment, and release methods.

Tests/AblyChatTests/DefaultRoomReactionsTests.swift (2)

5-5: Appropriate use of @mainactor for test class

Adding @mainactor to the test struct ensures all test methods run on the main actor, which aligns with the PR's objective of isolating mutable state to the main actor. This change is consistent with the broader refactoring approach.

15-16: Correctly removed await keywords from synchronous operations

Good job removing await keywords from method calls like initialization, property accesses, and method calls that are now synchronous due to the @mainactor isolation. This simplifies the test code and makes it more readable while maintaining the same functionality.

Also applies to: 28-30, 45-45, 62-63, 66-66

Tests/AblyChatTests/Mocks/MockRealtime.swift (4)

6-6: Appropriate conversion from actor to class

Converting MockRealtime from an actor to a regular class aligns with the PR's goal of isolating mutable state to the main actor rather than using actor-based concurrency.

12-13: Simplified property access without mutex protection

Good refactoring of the properties to use direct access rather than mutex-protected private backing properties. This simplifies the code while maintaining thread safety through the main actor.

31-33: Removed explicit synchronization in request method

Appropriately removed mutex locking when appending to the requestArguments array, relying instead on the main actor for thread safety. This reduces code complexity while maintaining correctness.

47-49: Removed explicit synchronization in createWrapperSDKProxy method

Direct assignment to createWrapperSDKProxyOptionsArgument without mutex protection simplifies the code, relying on the main actor for thread safety instead.

Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (5)

5-5: Appropriate use of @mainactor for test class

Adding @mainactor to the test struct ensures all test methods run on the main actor, which aligns with the PR's objective of isolating mutable state to the main actor. This is consistent with the broader refactoring approach.

62-63: Simplified method signature by removing async

Good job refactoring the createManager method to be synchronous. Since it's now running on the main actor and doesn't contain any asynchronous work, this simplifies the API.

95-96: Correctly removed await from synchronous subscriptions

Appropriately removed await keywords from subscription methods that are now synchronous due to the @mainactor isolation. This simplifies the test code while maintaining functionality.

Also applies to: 103-104

117-117: Correctly removed await from property access

Good job removing await keywords from property accesses throughout the file. This simplifies the code and makes it more readable while maintaining the same functionality. Properties accessed on the main actor no longer require await.

Also applies to: 134-134, 232-232, 258-258, 307-307, 367-367, 402-408, 450-450, 512-514, 530-530, 605-606, 632-632, 755-756, 831-831, 875-876, 943-943, 1075-1076, 1165-1165, 1279-1280, 1346-1346

395-396: Added @sendable to closures in async contexts

Correctly added @sendable to closures used in async contexts, ensuring proper concurrency safety. This is important for maintaining correctness when working with async sequences.

Also applies to: 410-412, 507-508, 643-645, 1333-1334, 2060-2062

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (5)

4-4: Appropriate conversion from actor to class

Converting MockRealtimeChannel from an actor to a regular class aligns with the PR's goal of isolating mutable state to the main actor rather than using actor-based concurrency. This change is consistent with the broader refactoring approach.

142-142: Correctly updated subscribe method to use @mainactor for callback

Good job updating the subscribe method to specify that the callback will be executed on the main actor. This ensures thread safety for UI-related code while maintaining compatibility with the Ably SDK's API.

156-157: Removed nonisolated from unsubscribe method

Appropriately removed the nonisolated keyword from the unsubscribe method, as it's no longer needed with the class-based approach. The main actor ensures thread safety.

160-162: Correctly updated on methods to use @mainactor for callback

Good job updating both on method overloads to specify that callbacks will be executed on the main actor. This ensures thread safety for UI-related code and state updates.

Also applies to: 164-166

168-170: Removed nonisolated from off method

Appropriately removed the nonisolated keyword from the off method, as it's no longer needed with the class-based approach. The main actor ensures thread safety.

@lawrence-forooghian lawrence-forooghian merged commit 2d75985 into main Apr 8, 2025
19 checks passed
@lawrence-forooghian lawrence-forooghian deleted the 232-switch-to-MainActor branch April 8, 2025 12:18
@lawrence-forooghian lawrence-forooghian mentioned this pull request Apr 8, 2025
Merged
@coderabbitai coderabbitai bot mentioned this pull request Apr 14, 2025
Merged
This was referenced May 18, 2025
Merged
Merged
@coderabbitai coderabbitai bot mentioned this pull request Jul 6, 2025
Merged
@coderabbitai coderabbitai bot mentioned this pull request Jul 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@maratal maratal maratal approved these changes

@umair-ably umair-ably Awaiting requested review from umair-ably

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consider switching the public API of the SDK to be @MainActor

3 participants

@lawrence-forooghian @maratal