[ECO-5377] Finalise and document threading approach #85
Conversation
WalkthroughSwitches internal concurrency to a DispatchQueue-based model: adds DispatchQueueMutex and ably_syncNoDeadlock, threads an internalQueue through core/plugin/objects and tests, introduces many nosync_ and testsOnly_ fast-paths/accessors, and pins two dependencies to exact git revisions. CONTRIBUTING.md threading guidance was inserted twice. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Public as ARTRealtimeChannel+Objects
participant SDK as PluginAPI
participant Q as internalQueue
participant Plugin as DefaultInternalPlugin
participant Core as InternalDefaultRealtimeObjects
participant Pool as ObjectsPool
Note over App,Public: Acquire objects handle
App->>Public: request objects
Public->>SDK: internalQueue(for: client)
SDK-->>Public: internalQueue
Public->>Q: sync { Plugin.nosync_realtimeObjects(channel, pluginAPI) }
Q-->>Public: InternalDefaultRealtimeObjects
Public-->>App: Objects handle
Note over App,Core: Mutate (e.g., increment)
App->>Core: increment()
Core->>Q: dispatchPrecondition(.onQueue) / ensure on internalQueue
Core->>Pool: nosync_buildMessage / nosync_apply(...)
Pool->>SDK: nosync_sendObject(message, completion)
SDK-->>Q: completion (runs on internalQueue)
alt success
Q-->>Core: success callback
else failure
Q-->>Core: error callback
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.swift📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
Files:
Tests/**/*.swift📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Files:
🧬 Code graph analysis (1)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (6)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lawrence-forooghian
changed the title
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
17fbbf3 to
b125c6e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
b125c6e to
55b834b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
275-376: Critical: Set subtraction uses unsupported + between Set and Array.Line 362
subtracting(receivedObjectIds + [Self.rootKey])likely doesn’t compile unless you have a custom overload. Use union.Apply:
- let objectIdsToRemove = Set(entries.keys).subtracting(receivedObjectIds + [Self.rootKey]) + let objectIdsToRemove = Set(entries.keys).subtracting(receivedObjectIds.union([Self.rootKey]))Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (2)
16-22: Data race on_publishHandlerread inpublish— guard with the lock.Copy the handler under the lock, then call it outside.
- func publish(objectMessages: [OutboundObjectMessage]) async throws(InternalError) { - if let handler = _publishHandler { - try await handler(objectMessages) - } else { - protocolRequirementNotImplemented() - } - } + func publish(objectMessages: [OutboundObjectMessage]) async throws(InternalError) { + let handler = mutex.withLock { _publishHandler } + guard let handler else { + protocolRequirementNotImplemented() + return + } + try await handler(objectMessages) + }
24-26: Keep backwards-compatibility: implementtestsOnly_overridePublishviasetPublishHandler.Prevents unexpected
protocolRequirementNotImplemented()in tests still using this API.- func testsOnly_overridePublish(with _: @escaping ([OutboundObjectMessage]) async throws(InternalError) -> Void) { - protocolRequirementNotImplemented() - } + func testsOnly_overridePublish(with handler: @escaping ([OutboundObjectMessage]) async throws(InternalError) -> Void) { + setPublishHandler(handler) + }
🧹 Nitpick comments (19)
CONTRIBUTING.md (1)
73-82: Tiny grammar fix in Threading section.Change “invoked on same queue as used by” → “invoked on the same queue used by”.
-2. Callbacks passed to the public API are invoked on same queue as used by the `ARTRealtime` instance (the `dispatchQueue` client option) +2. Callbacks passed to the public API are invoked on the same queue used by the `ARTRealtime` instance (the `dispatchQueue` client option)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
99-112: Helpful diagnostics on invalid state.Consider logging operationDescription and current state before throwing to aid field debugging.
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
25-37: Optional: rethrows(E) to improve call ergonomics.If you want non-throwing closures not to force try, switch to
rethrows(E).Apply:
- internal func withLock<R, E>(_ body: (inout T) throws(E) -> R) throws(E) -> R { + internal func withLock<R, E>(_ body: (inout T) throws(E) -> R) rethrows(E) -> R {Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
82-107: Remove unused userCallbackQueue parameter from Entry.nosync_replaceData.It’s unused in both branches; keeping it adds noise and risks drift.
Apply:
- fileprivate func nosync_replaceData( + fileprivate func nosync_replaceData( using state: ObjectState, objectMessageSerialTimestamp: Date?, objectsPool: inout ObjectsPool, - userCallbackQueue: DispatchQueue, ) -> DeferredUpdate { @@ - let deferredUpdate = existingEntry.nosync_replaceData( + let deferredUpdate = existingEntry.nosync_replaceData( using: syncObjectsPoolEntry.state, objectMessageSerialTimestamp: syncObjectsPoolEntry.objectMessageSerialTimestamp, objectsPool: &self, - userCallbackQueue: userCallbackQueue, )Also applies to: 299-305
128-146: Remove comments on testsOnly_ accessors per repo guidelines.Guideline: don’t add generic explanatory comments for testsOnly_ properties.
Apply:
- /// Test-only accessor for isTombstone that handles locking internally. internal var testsOnly_isTombstone: Bool { @@ - /// Test-only accessor for tombstonedAt that handles locking internally. internal var testsOnly_tombstonedAt: Date? {Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
114-119: Remove explanatory comments from testsOnly_ accessors (per repo guideline).Guideline: “For testsOnly_ property declarations, do not add generic explanatory comments.” Strip the doc comments from these accessors.
- /// Test-only accessor for objectID that handles locking internally. internal var testsOnly_objectID: String { mutableStateMutex.withLock { mutableState in mutableState.liveObjectMutableState.objectID } }Apply similarly to
testsOnly_isTombstoneandtestsOnly_tombstonedAt.Also applies to: 406-411, 420-425
895-921: Minor: prefer lazy filter/map to avoid intermediate array growth in entries().Not critical, but you could tighten allocation a bit.
- var result: [(key: String, value: InternalLiveMapValue)] = [] - - for (key, entry) in data where !Self.nosync_isEntryTombstoned(entry, delegate: delegate) { - // Convert entry to LiveMapValue using the same logic as get(key:) - if let value = nosync_convertEntryToLiveMapValue(entry, delegate: delegate) { - result.append((key: key, value: value)) - } - } - - return result + return data.lazy + .filter { !Self.nosync_isEntryTombstoned($0.value, delegate: delegate) } + .compactMap { key, entry in + nosync_convertEntryToLiveMapValue(entry, delegate: delegate).map { (key: key, value: $0) } + }Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (1)
123-129: Add explicit ACLs inside the private nested type (explicit_acl).Even within a
privatetype, declare member ACLs explicitly to satisfy the guideline.- private struct LifecycleEventResponse: OnLiveObjectLifecycleEventResponse { - let subscription: any SubscribeResponse + private struct LifecycleEventResponse: OnLiveObjectLifecycleEventResponse { + private let subscription: any SubscribeResponse - func off() { + internal func off() { subscription.unsubscribe() } }Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
37-53: Prepare path: consider asserting queue expectationsThe method is prefixed
nosync_yet it capturesinternalQueueand writes vianosync_setPluginDataValue. If the contract requires running on the internalQueue, add a debug precondition; otherwise, a short comment clarifying that nosync here means “no internal locking required” would help future readers.
149-156: Minor: simplify continuation with throwing variantYou can avoid
Resultplumbing by usingwithCheckedThrowingContinuationand throwing directly from the completion.Proposed refactor:
- try await withCheckedContinuation { (continuation: CheckedContinuation<Result<Void, InternalError>, _>) in + try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, Error>) in let internalQueue = pluginAPI.internalQueue(for: client) internalQueue.async { pluginAPI.nosync_sendObject( withObjectMessages: objectMessageBoxes, channel: channel, ) { error in // We don't currently rely on this documented behaviour of `nosync_sendObject` but we may do later, so assert it to be sure it's happening. dispatchPrecondition(condition: .onQueue(internalQueue)) - if let error { - continuation.resume(returning: .failure(error.toInternalError())) - } else { - continuation.resume(returning: .success(())) - } + if let error { continuation.resume(throwing: error.toInternalError()) } + else { continuation.resume() } } } - }.get() + }Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
12-19: Document access constraints to prevent misuse.Clarify that
objectsmust not be accessed oninternalQueue; use nosync APIs instead.- var objects: [String: ObjectsPool.Entry] { + /// Access only off `internalQueue`. When already on `internalQueue`, use `nosync_` accessors. + var objects: [String: ObjectsPool.Entry] {Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift (2)
33-54: Wrap nosync operation on internalQueue; consider a small helper to reduce repetition.Pattern is correct; a tiny helper would keep tests DRY and avoid accidental re-entrancy.
Example helper (in TestFactories):
+extension DispatchQueue { + @inlinable + func syncReturn<T>(_ body: () -> T) -> T { sync { body() } } +}Usage:
- let creationOperation = internalQueue.sync { + let creationOperation = internalQueue.syncReturn { ObjectCreationHelpers.nosync_creationOperationForLiveMap(… }
7-16: Duplicate @SPEC tag(s).Avoid repeating the same
@spec. Remove the duplicateRTO11f4c1a.- // @spec RTO11f4c1a - // @spec RTO11f4c1a + // @spec RTO11f4c1aTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
352-361: Correct spec comment (will be discarded, not applied) and typoThe comment contradicts the test intent. With
ts1 < ts2,canApplyOperationshould return nil and the op is discarded. Also fix “RTOL4a” → “RTLO4a”.- // Apply operation with serial "ts1" which is lexicographically less than existing "ts2" and thus will be applied per RTLO4a (this is a non-pathological case of RTOL4a, that spec point being fully tested elsewhere) + // Apply operation with serial "ts1" which is lexicographically less than existing "ts2" and thus will be discarded per RTLO4a (this is a non-pathological case of RTLO4a, that spec point being fully tested elsewhere)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (2)
451-455: Nit: spec tag typoComment says
RTL9e; should beRTLC9e.- // RTL9e + // RTLC9e
351-353: Track TODO with an issueUnresolved spec detail in RTLC6d. Please link an issue and reference it here to avoid TODO drift.
I can open a tracking issue and propose wording based on the current spec discussion if helpful.
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (2)
351-355: Fix tombstone comments to match setupThe comments contradict the data (Date() means tombstoned).
Apply:
- // tombstonedAt is false, so not considered tombstoned - "tombstoned": TestFactories.internalMapEntry(tombstonedAt: Date(), data: ObjectData(string: "tombstoned")), + // tombstonedAt is set, so considered tombstoned + "tombstoned": TestFactories.internalMapEntry(tombstonedAt: Date(), data: ObjectData(string: "tombstoned")), - "tombstoned2": TestFactories.internalMapEntry(tombstonedAt: Date(), data: ObjectData(string: "tombstoned2")), + "tombstoned2": TestFactories.internalMapEntry(tombstonedAt: Date(), data: ObjectData(string: "tombstoned2")),
1122-1124: Correct spec tag typos (RTLO4a/RTOL4a → RTO4a)Keeps spec references consistent.
Apply:
- // Apply operation with serial "ts1" which is lexicographically less than existing "ts2" and thus will be applied per RTLO4a (this is a non-pathological case of RTOL4a, that spec point being fully tested elsewhere) + // Apply operation with serial "ts1" which is lexicographically less than existing "ts2" and thus will be applied per RTO4a (this is a non-pathological case of RTO4a, that spec point being fully tested elsewhere)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (1)
1-3: Import Ably in tests per project guidelinesKeeps test files consistent with the stated testing imports policy.
Apply:
import _AblyPluginSupportPrivate +import Ably @testable import AblyLiveObjects import Testing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)CONTRIBUTING.md(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(3 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(4 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(9 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(32 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(22 hunks)Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift(2 hunks)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(22 hunks)Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift(1 hunks)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(22 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(51 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(53 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(3 hunks)Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift(8 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(17 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)
Prefer implicit .init(...) when the type can be inferred in initializer expressions
Prefer enum case shorthand (.caseName) when the type can be inferred
For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Prefer Swift raw string literals for JSON strings instead of escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swiftPackage.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/LiveObjectMutableState.swiftSources/AblyLiveObjects/Internal/InternalLiveMapValue.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftTests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftTests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest, in test files
Do not usefatalErrorfor expectation failures; prefer Swift Testing’s#require
Only add labels to test cases or suites when the label differs from the suite struct or test method name
Tag tests per CONTRIBUTING.md’s "Attributing tests to a spec point" with exact comment format; distinguish@specvs@specPartial; do not repeat@specfor the same spec point
Add comments in tests to clarify when certain test data is irrelevant to the scenario
In tests, import Ably usingimport Ably
In tests, import AblyLiveObjects using@testable import AblyLiveObjects
In tests, import_AblyPluginSupportPrivateusingimport _AblyPluginSupportPrivate(do not useinternal import)
When passing a logger to internal components in tests, useTestLogger()
When unwrapping optionals in tests, prefer#requireoverguard let
Files:
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftTests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swiftTests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
Sources/AblyLiveObjects/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
In AblyLiveObjects library (non-test) code, import modules as: Ably with
import Ably, and _AblyPluginSupportPrivate withinternal import _AblyPluginSupportPrivate
Files:
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/LiveObjectMutableState.swiftSources/AblyLiveObjects/Internal/InternalLiveMapValue.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)
Files:
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/LiveObjectMutableState.swiftSources/AblyLiveObjects/Internal/InternalLiveMapValue.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
🧠 Learnings (5)
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : Only add labels to test cases or suites when the label differs from the suite struct or test method name
Applied to files:
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift
📚 Learning: 2025-08-26T11:32:26.789Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-08-26T11:32:26.789Z
Learning: Applies to Sources/AblyLiveObjects/**/*.swift : In AblyLiveObjects library (non-test) code, import modules as: Ably with `import Ably`, and _AblyPluginSupportPrivate with `internal import _AblyPluginSupportPrivate`
Applied to files:
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolvedSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : In tests, import AblyLiveObjects using `testable import AblyLiveObjects`
Applied to files:
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : In tests, import `_AblyPluginSupportPrivate` using `import _AblyPluginSupportPrivate` (do not use `internal import`)
Applied to files:
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
📚 Learning: 2025-08-22T20:27:49.043Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#79
File: Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift:246-258
Timestamp: 2025-08-22T20:27:49.043Z
Learning: In Swift, `mutex.withLock { ... }` closures with single expressions automatically return the result of that expression, so explicit return statements are not needed when the closure contains only a call like `mutableState.on(...)` that returns the expected type.
Applied to files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
🧬 Code graph analysis (17)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
value(100-104)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
get(124-128)
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (1)
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
nosync_realtimeObjects(24-32)nosync_realtimeObjects(56-58)
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (1)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (3)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (6)
createInternalQueue(688-691)counterObjectState(229-244)objectOperation(336-356)counterCreateOperation(374-383)counterOp(386-388)mapCreateOperation(359-371)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
createZeroValued(66-81)nosync_replaceData(210-223)value(100-104)nosync_mergeInitialValue(226-230)nosync_apply(247-266)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_replaceData(82-107)nosync_apply(33-58)
Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (2)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withLock(25-37)withoutLock(45-49)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
nosync_getObjectFromPool(144-148)
Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
entries(136-140)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (5)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (4)
createZeroValued(66-81)nosync_replaceData(210-223)nosync_mergeInitialValue(226-230)nosync_apply(247-266)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (6)
createZeroValued(87-104)get(124-128)nosync_replaceData(276-292)nosync_mergeInitialValue(295-306)nosync_apply(323-343)set(152-182)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_replaceData(82-107)nosync_apply(33-58)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
setPublishHandler(33-37)
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (6)
nosync_onChannelAttached(293-301)nosync_onChannelAttached(450-476)nosync_handleObjectProtocolMessage(308-319)nosync_handleObjectProtocolMessage(582-612)nosync_handleObjectSyncProtocolMessage(326-338)nosync_handleObjectSyncProtocolMessage(479-579)
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (5)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (4)
nosync_subscribe(84-100)emit(138-147)on(102-120)unsubscribeAll(130-132)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
createListener(36-58)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
subscribe(150-164)on(172-185)unsubscribeAll(166-170)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withLock(25-37)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
nosync_apply(247-266)nosync_emit(198-202)nosync_replaceData(210-223)createZeroValued(66-81)nosync_mergeInitialValue(226-230)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (8)
nosync_apply(323-343)nosync_emit(263-267)nosync_replaceData(276-292)entries(136-140)createZeroValued(87-104)nosync_mergeInitialValue(295-306)nosync_resetData(384-388)nosync_releaseTombstonedEntries(391-395)
Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift (4)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
createZeroValued(66-81)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
createZeroValued(87-104)entries(136-140)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
nosync_creationOperationForLiveMap(103-160)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withLock(25-37)withoutLock(45-49)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (4)
nosync_subscribe(84-100)unsubscribeAll(130-132)on(102-120)offAll(134-136)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (8)
unsubscribeAll(231-235)on(237-250)offAll(252-256)nosync_emit(263-267)nosync_replaceData(276-292)nosync_mergeInitialValue(295-306)mergeInitialValue(517-575)nosync_apply(323-343)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
nosync_emit(66-73)nosync_replaceData(82-107)nosync_apply(33-58)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withoutLock(45-49)
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (5)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (3)
createInternalQueue(688-691)objectState(189-205)counterObjectState(229-244)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
createZeroValued(66-81)value(100-104)subscribe(150-164)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
createZeroValued(87-104)entries(136-140)subscribe(215-229)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
createZeroValueObject(222-272)nosync_applySyncObjectsPool(275-376)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
createListener(36-58)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (3)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (3)
nosync_handleObjectSyncProtocolMessage(123-134)nosync_handleObjectProtocolMessage(111-121)nosync_onChannelAttached(107-109)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (7)
nosync_handleObjectSyncProtocolMessage(326-338)nosync_handleObjectSyncProtocolMessage(479-579)nosync_handleObjectProtocolMessage(308-319)nosync_handleObjectProtocolMessage(582-612)testsOnly_createZeroValueLiveObject(343-353)nosync_onChannelAttached(293-301)nosync_onChannelAttached(450-476)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (5)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)
nosync_getObjectFromPool(144-148)on(264-277)on(669-687)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withLock(25-37)withoutLock(45-49)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (2)
nosync_subscribe(84-100)on(102-120)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
createZeroValueObject(222-272)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (5)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withLock(25-37)withoutLock(45-49)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
nosync_creationOperationForLiveMap(103-160)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (7)
nosync_getOrCreateMap(434-471)nosync_getOrCreateCounter(387-423)createZeroValueObject(222-272)nosync_performGarbageCollection(486-518)nosync_reset(474-483)nosync_applySyncObjectsPool(275-376)nosync_apply(33-58)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (3)
nosync_onChannelAttached(107-109)nosync_handleObjectProtocolMessage(111-121)nosync_handleObjectSyncProtocolMessage(123-134)
🪛 LanguageTool
CONTRIBUTING.md
[grammar] ~78-~78: There might be a mistake here.
Context: ...ks passed to the public API are invoked on same queue as used by the ARTRealtime...
(QB_NEW_EN)
🔇 Additional comments (55)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
2-18: Lockfile aligns with pinned revisions.Matches Package.swift; nothing else to do. Keep this in sync when unpinning.
Use the script in Package.swift comment to re-check after any dependency updates.
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
683-691: Internal queue helper looks good.Serial by default, unique label, and .userInitiated QoS are sensible for tests.
Package.resolved (1)
2-18: Root lockfile matches the workspace and manifest pins.LGTM; consistent with the temporary revision pinning approach.
Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
84-86: LGTM: direct passthrough to pluginAPI.Implementation aligns with nosync contract.
17-18:channelStaterename validated: no remaining call sites detected.Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
45-49: LGTM: queue-correctness precondition is appropriate.The on-queue guard keeps withoutLock safe.
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (9)
33-58: LGTM: nosync_apply forwards correctly to object-specific handlers.
66-73: LGTM: deferred updates now emit via nosync paths.
109-126: LGTM: tombstone accessors expose nosync_isTombstone/tombstonedAt for GC.
162-174: LGTM: internalQueue now threaded through pool initialization and root creation.Also applies to: 185-193
222-228: LGTM: zero-value object creation propagates internalQueue to maps/counters.Also applies to: 246-254, 256-264
383-423: LGTM: nosync_getOrCreateCounter adds internalQueue and merges initial value on creation.
430-471: LGTM: nosync_getOrCreateMap mirrors counter flow and passes semantics.
473-483: LGTM: nosync_reset preserves root and clears via nosync_resetData.
486-519: LGTM: GC flow uses nosync_releaseTombstonedEntries and nosync_tombstonedAt; yields once upon completion.Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (7)
73-80: Good: state isolation via DispatchQueueMutex initialized with internalQueue.This establishes a clear single-threaded mutation domain matching ably-cocoa’s model. The explicit
internalQueuewiring into the mutex is correct.
263-267: Good: nosync_emit uses withoutLock on the internal queue and delivers on userCallbackQueue.Separation between internal mutation and user callbacks is clear and avoids callback-induced re-entrancy.
276-306: Correct threading of internalQueue into replace/merge paths.Passing
internalQueue: mutableStateMutex.dispatchQueuethrough ensures downstream mutations occur on the same queue and aligns with the nosync discipline.
323-343: Apply path aligns with RTLM15 and emits on userCallbackQueue; queue propagation looks consistent.No issues spotted; keep as-is.
710-716: Zero-value object creation is correctly routed with internalQueue/userCallbackQueue/clock.This prevents cross-queue pool mutations. LGTM.
877-893: Channel-state validation moved to nosync_ API is appropriate inside the lock.Given the CoreSDK’s nosync contract, validating while on the internal queue is consistent.
217-229: Ignore nested withLock suggestion:SubscriptionStorage.subscribeonly stores theupdateSelfLaterclosure and does not invoke it during subscription, so there’s no runtime nestedwithLockto trigger a dispatch precondition.Likely an incorrect or invalid review comment.
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (1)
85-100: nosync_subscribe rename and nosync_validateChannelState usage look correct.Matches the project-wide nosync discipline and keeps validation on the internal queue.
Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift (1)
50-73: Rename to nosync_toObjectData and use of nosync_objectID are consistent.The mapping remains 1:1 with previous behavior; switching to nosync identifiers aligns with new isolation semantics.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (3)
1285-1285: Good: switched to test-only accessor for object IDUsing
testsOnly_objectIDavoids relying on non-test internals. LGTM.
3141-3141: Good: test-only accessor for map object ID
testsOnly_objectIDusage is appropriate here too. LGTM.
3787-3791: Good: test-only tombstone flags
testsOnly_isTombstoneandtestsOnly_tombstonedAtkeep tests decoupled from production internals. LGTM.Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
103-110: nosync variant consistency looks correctRenaming to
nosync_creationOperationForLiveMapand usingnosync_toObjectDataaligns with the internal-queue contract. Please ensure all call sites execute this on the plugin’s internal queue.Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
24-32: Static nosync accessor matches new threading modelFetching with
nosync_pluginDataValueand failing fast if missing is correct for internal use. LGTM.
107-134: Event dispatch to nosync handlers looks correctForwarding to
nosync_onChannelAttached/nosync_handleObject{,Sync}ProtocolMessagealigns with the internal locking scheme. LGTM.Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (2)
21-23: nosync accessor matches DispatchQueueMutex contract.Precondition is enforced via
withoutLock’s.onQueuecheck. LGTM.
6-10: Approve changes Good move to internalQueue-backed mutex; aligns with the nosync pattern and DispatchQueueMutex invariants, and no violations of withoutLock/withLock preconditions were detected.Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift (1)
28-31: Correctly threading internalQueue through zero-valued creators.Matches the new initializer signatures.
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (2)
12-14: Init wiring looks consistent with DispatchQueueMutex usage.No issues spotted.
28-30: Annotate nosync_channelState with queue requirement- var nosync_channelState: _AblyPluginSupportPrivate.RealtimeChannelState { + /// Must be called on `internalQueue`; uses `DispatchQueueMutex.withoutLock`. + var nosync_channelState: _AblyPluginSupportPrivate.RealtimeChannelState { channelStateMutex.withoutLock { $0 } }Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (1)
231-235: LGTM: shared internalQueue is plumbed consistently through store/coreSDKThe tests correctly create and pass a single
internalQueueinto the store andMockCoreSDK, matching the new threading model.Also applies to: 306-309
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (2)
14-17: LGTM: tests correctly drive nosync_ via internalQueue and validate channel state/publish paths*The internalQueue-driven harness, channel-state error expectations, and publish assertions look correct and consistent.
Also applies to: 33-36, 52-54, 70-73, 103-106, 119-122, 138-141, 161-164, 188-191, 236-239, 263-266, 307-310, 334-337, 377-380, 417-420, 461-464, 492-495, 516-518, 536-539, 566-568, 590-593
38-41: Ensure a throwingDispatchQueue.syncextension is availableTests invoke
try internalQueue.sync { … }, but a code search found norethrowsextension onDispatchQueue.sync. Add or import a throwingsyncextension in the test target to satisfy these calls.Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
106-143: LGTM: increment constructs message under lock and publishes after releasing itChannel-state validation, finiteness check, and building
OutboundObjectMessageunder the mutex with publish outside is the right split.
210-223: LGTM: nosync_ mutation paths correctly use withoutLock and dispatch to userCallbackQueue*The counter’s replace/merge/apply paths align with the internal-queue discipline and avoid holding the lock during emission.
Also applies to: 226-230, 246-266
198-202: Ensure allnosync_*accessors are only called on the internal queue
Manual verification: confirmnosync_emit,nosync_isTombstone, andnosync_tombstonedAtinvocations occur exclusively within yourinternalQueuecallbacks to maintain thread safety.Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (2)
200-204: Queue-isolated nosync usage looks solidWrapping nosync_* calls with internalQueue.sync keeps mutations serialized and matches the new threading model.
Also applies to: 221-224, 231-234, 253-256
1450-1461: Rename test methods to start withtestfor XCTest discovery
XCTest only runs methods prefixed withtest; rename the following:
appliesOperationWhenCanBeApplied→testAppliesOperationWhenCanBeAppliedcreatesNewEntryWhenNoExistingEntry→testCreatesNewEntryWhenNoExistingEntrymapOperationApplicability→testMapOperationApplicabilitypublishesCorrectObjectMessageForDifferentValueTypes→testPublishesCorrectObjectMessageForDifferentValueTypespublishesCorrectObjectMessage→testPublishesCorrectObjectMessageLikely an incorrect or invalid review comment.
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
31-47: LGTM: deterministic internalQueue threading across testsConsistent creation/propagation of internalQueue and nosync_* usage aligns with the documented model.
Also applies to: 75-81, 108-114
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (1)
109-112: LGTM: applySyncObjectsPool and other nosync ops correctly wrapped on internalQueueMatches the serialized mutation contract; subscriber assertions validate emissions.
Also applies to: 147-150, 178-181, 209-212, 239-242, 268-269, 289-291, 354-357
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (10)
145-148: Good implementation of nosync patternThe method correctly uses
withoutLockwith the expected dispatch queue precondition, aligning with the threading model documented inDispatchQueueMutex.
91-115: Good parameter addition and mutex initializationThe addition of
internalQueueparameter and its use in initializingDispatchQueueMutexaligns well with the PR's threading approach objectives.
293-301: Correct use of withoutLock for nosync methodThe
nosync_onChannelAttachedmethod correctly useswithoutLock, indicating it's expected to be called when already on the internal queue. This aligns with the nosync naming convention and the broader threading model.
308-319: Good propagation of internalQueue through nosync methodsThe method correctly passes
mutableStateMutex.dispatchQueueas theinternalQueueparameter to the nested nosync method call.
326-338: Consistent nosync pattern implementationThe
nosync_handleObjectSyncProtocolMessagefollows the same pattern asnosync_handleObjectProtocolMessage, correctly usingwithoutLockand passing the internal queue.
667-687: Clever use of closure for deferred updatesThe
UpdateMutableStatetype alias and its usage in theonmethod elegantly handle the need to update mutable state after releasing the lock. This pattern ensures thread safety while avoiding potential deadlocks.
563-570: Consistent nosync method namingThe change from
applyObjectProtocolMessageObjectMessagetonosync_applyObjectProtocolMessageObjectMessageproperly indicates that this method should be called when already on the internal queue.
468-468: Remove nosync_reset existence check
Thenosync_reset()method is implemented inSources/AblyLiveObjects/Internal/ObjectsPool.swift(around line 474), so the call is valid.
615-621: Ensure callers invoke on the internal queue
- Call at Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift:563
- Call at Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift:603
Confirm each invocation runs oninternalQueue(e.g. withininternalQueue.asyncor already on that queue).
152-172: Allnosync_validateChannelStatecalls are wrapped inmutableStateMutex.withLock, ensuring they execute on the internal queue.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
Outdated
Show resolved
Hide resolved
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
Outdated
Show resolved
Hide resolved
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
55b834b to
be8e9bf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
Mistake in d9c327e.
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
be8e9bf to
e708951
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
e708951 to
5540135
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (1)
13-16: Good switch to ably_syncNoDeadlock to avoid self-queue deadlocksExecuting nosync_ retrieval under
internalQueue.ably_syncNoDeadlock { ... }is correct and prevents accidental re-entry hangs. Matches the PR’s threading model.
🧹 Nitpick comments (14)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
17-17: Same note for ably-cocoa-plugin-support: revision pin is fine; verify Package.swift and track unpin.Covered by the script above; please ensure the manifest also pins this to the same commit style and add a reminder to revert to tags pre-release.
CONTRIBUTING.md (1)
73-84: Threading section: add the explicit “different queues” invariant and tighten one sentence
- Please state explicitly that the user callback queue and the internal queue are always different; this prevents confusion and future regressions around nested locking.
- Minor style: drop “in order to”.
### Threading Since this is an extension of ably-cocoa, we follow the same threading approach: 1. The public API can be interacted with from any thread, including synchronous methods such as getters 2. Callbacks passed to the public API are invoked on the same queue as used by the `ARTRealtime` instance (the `dispatchQueue` client option) 3. Synchronisation of mutable state is performed using the same internal serial dispatch queue as is used by the `ARTRealtime` instance (the `internalDispatchQueue` client option) We follow the same naming convention as in ably-cocoa whereby if a method's name contains `nosync` then it must be called on the internal dispatch queue. This allows us to avoid deadlocks that would result from attempting to call `DispatchQueue.sync { … }` when already on the internal queue. -We have an extension on `DispatchQueue`, `ably_syncNoDeadlock(execute:)`, which behaves the same as `sync(execute:)` but with a runtime precondition that we are not already on the queue; favour our extension in order to avoid deadlock. +Note: the user callback queue and the internal queue are intentionally different. This avoids deadlocks when callbacks update internal state under `withLock`. + +We have an extension on `DispatchQueue`, `ably_syncNoDeadlock(execute:)`, which behaves the same as `sync(execute:)` but with a runtime precondition that we are not already on the queue; favour our extension to avoid deadlock.Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (2)
199-206: Nested withLock inside updateSelfLater: safe, but add a clarifying commentGiven updateSelfLater is stored and invoked later (e.g., during unsubscribe) outside the current locking context, reacquiring the mutex here won’t violate
.notOnQueue. Add a brief comment to prevent future “false positive” refactors.- try stored.nosync_subscribe(listener: listener, coreSDK: coreSDK, updateSelfLater: { [weak self] action in + try stored.nosync_subscribe(listener: listener, coreSDK: coreSDK, updateSelfLater: { [weak self] action in guard let self else { return } - mutex.withLock { stored in + // Safe: invoked later (e.g., during unsubscribe), outside the outer withLock’s context. + mutex.withLock { stored in action(&stored) } })
247-247: Typo: recieve → receiveMinor nit in the comment.
- // @specOneOf(2/3) RTLO4b5b - Check we can unsubscribe using the `response` that's passed to the listener, and that when two updates are emitted back-to-back, the unsubscribe in the first listener causes us to not recieve the second update + // @specOneOf(2/3) RTLO4b5b - Check we can unsubscribe using the `response` that's passed to the listener, and that when two updates are emitted back-to-back, the unsubscribe in the first listener causes us to not receive the second updateSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
37-53: Prepare: thread internalQueue/callbackQueue and store objects—consider asserting queues differ in debugThe wiring looks correct. As a defensive check, consider asserting that internalQueue and callbackQueue aren’t the same in debug builds to catch misconfiguration early.
internal func nosync_prepare(_ channel: _AblyPluginSupportPrivate.RealtimeChannel, client: _AblyPluginSupportPrivate.RealtimeClient) { let pluginLogger = pluginAPI.logger(for: channel) let internalQueue = pluginAPI.internalQueue(for: client) let callbackQueue = pluginAPI.callbackQueue(for: client) + #if DEBUG + if internalQueue === callbackQueue { + assertionFailure("internalQueue and callbackQueue must be different") + } + #endif
138-165: sendObject: simplify continuation and align type naming
- Consider withCheckedThrowingContinuation to avoid the intermediate Result.
- Minor consistency: elsewhere types are fully qualified; use the same for PluginAPIProtocol here.
- internal static func sendObject( + internal static func sendObject( objectMessages: [OutboundObjectMessage], channel: _AblyPluginSupportPrivate.RealtimeChannel, client: _AblyPluginSupportPrivate.RealtimeClient, - pluginAPI: PluginAPIProtocol, + pluginAPI: _AblyPluginSupportPrivate.PluginAPIProtocol, ) async throws(InternalError) { let objectMessageBoxes: [ObjectMessageBox<OutboundObjectMessage>] = objectMessages.map { .init(objectMessage: $0) } - try await withCheckedContinuation { (continuation: CheckedContinuation<Result<Void, InternalError>, _>) in + try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, any Error>) in let internalQueue = pluginAPI.internalQueue(for: client) internalQueue.async { pluginAPI.nosync_sendObject( withObjectMessages: objectMessageBoxes, channel: channel, ) { error in // We don't currently rely on this documented behaviour of `nosync_sendObject` but we may do later, so assert it to be sure it's happening. dispatchPrecondition(condition: .onQueue(internalQueue)) if let error { - continuation.resume(returning: .failure(error.toInternalError())) + continuation.resume(throwing: error.toInternalError()) } else { - continuation.resume(returning: .success(())) + continuation.resume() } } } - }.get() + } }Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (2)
24-24: Nit: use the local pluginAPI for consistencyYou already bound
let pluginAPI = Plugin.defaultPluginAPI; pass that instead of re-reading the static to avoid drift and improve clarity.- pluginAPI: Plugin.defaultPluginAPI, + pluginAPI: pluginAPI,
37-40: Nit: drop generic doc comment on testsOnly_ APIPer repo guidelines, testsOnly_ declarations don’t need explanatory comments. Consider removing the docstring here.
- /// For tests to access the non-public API of `PublicDefaultRealtimeObjects`. internal var testsOnly_nonTypeErasedObjects: PublicDefaultRealtimeObjects { nonTypeErasedObjects }Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
217-222: Nonce generation TODO — offer secure, spec-aligned helperHappy to replace the ad-hoc charset generator with a CSPRNG-based Base64URL string to satisfy the TODO.
- private static func generateNonce() -> String { - // TODO: confirm if there's any specific rules here: https://github.com/ably/specification/pull/353/files#r2228252389 - let letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" - return String((0 ..< 16).map { _ in letters.randomElement()! }) - } + private static func generateNonce() -> String { + // 16 random bytes, Base64URL-encoded (no padding) + var bytes = [UInt8](repeating: 0, count: 16) + _ = bytes.withUnsafeMutableBytes { ptr in + SecRandomCopyBytes(kSecRandomDefault, ptr.count, ptr.baseAddress!) + } + let b64 = Data(bytes).base64EncodedString() + .replacingOccurrences(of: "+", with: "-") + .replacingOccurrences(of: "/", with: "_") + .replacingOccurrences(of: "=", with: "") + return b64 + }Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
25-35: Simplify withLock by leveraging rethrows; Result wrapper unnecessaryYou can propagate the typed error directly and let
ably_syncNoDeadlockrethrow, reducing nesting.- internal func withLock<R, E>(_ body: (inout T) throws(E) -> R) throws(E) -> R { - let result: Result<R, E> = dispatchQueue.ably_syncNoDeadlock { - do throws(E) { - return try .success(body(&value)) - } catch { - return .failure(error) - } - } - - return try result.get() - } + internal func withLock<R, E>(_ body: (inout T) throws(E) -> R) throws(E) -> R { + try dispatchQueue.ably_syncNoDeadlock { + try body(&value) + } + }Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
247-256: Optionally run mutating helpers inside the internal queue for consistency.testsOnly_* helpers likely lock internally, but executing them within internalQueue.ably_syncNoDeadlock keeps call sites uniform with the rest of the suite.
Also applies to: 275-283, 318-325
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
108-120: Nested withLock inside subscribe/on is acceptable given queue separation.Given internalQueue != userCallbackQueue by design (per ably-cocoa conventions), invoking withLock from updateSelfLater is safe and non-reentrant. Consider adding a short comment to codify this invariant.
- try mutableState.liveObjectMutableState.nosync_subscribe(listener: listener, coreSDK: coreSDK, updateSelfLater: { [weak self] action in + // Safe: updateSelfLater runs on userCallbackQueue, which is distinct from internalQueue by design. + try mutableState.liveObjectMutableState.nosync_subscribe(listener: listener, coreSDK: coreSDK, updateSelfLater: { [weak self] action inAlso applies to: 215-229, 231-257
906-922: Small iteration cleanup in nosync_entries.Use compactMap to reduce temporary storage and branching.
- var result: [(key: String, value: InternalLiveMapValue)] = [] - - for (key, entry) in data where !Self.nosync_isEntryTombstoned(entry, delegate: delegate) { - // Convert entry to LiveMapValue using the same logic as get(key:) - if let value = nosync_convertEntryToLiveMapValue(entry, delegate: delegate) { - result.append((key: key, value: value)) - } - } - - return result + return data.compactMap { key, entry in + guard !Self.nosync_isEntryTombstoned(entry, delegate: delegate), + let value = nosync_convertEntryToLiveMapValue(entry, delegate: delegate) + else { return nil } + return (key: key, value: value) + }
320-321: Consider aligning testsOnly_ helper locks with nosync variants.*testsOnly_* helpers use withLock (fine for tests). If you later expose them for non-test diagnostics, add nosync_* counterparts to avoid surprise locking.
Also applies to: 355-367, 372-382
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)CONTRIBUTING.md(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(3 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(4 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(9 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(32 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(22 hunks)Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift(2 hunks)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(22 hunks)Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift(1 hunks)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift(1 hunks)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(22 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(51 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(53 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(3 hunks)Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift(8 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift
- Sources/AblyLiveObjects/Internal/CoreSDK.swift
- Package.resolved
- Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
- Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
- Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift
- Package.swift
- Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift
- Sources/AblyLiveObjects/Internal/ObjectsPool.swift
- Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift
- Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift
- Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)
Prefer implicit .init(...) when the type can be inferred in initializer expressions
Prefer enum case shorthand (.caseName) when the type can be inferred
For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Prefer Swift raw string literals for JSON strings instead of escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
Sources/AblyLiveObjects/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
In AblyLiveObjects library (non-test) code, import modules as: Ably with
import Ably, and _AblyPluginSupportPrivate withinternal import _AblyPluginSupportPrivate
Files:
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)
Files:
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swiftSources/AblyLiveObjects/Internal/ObjectCreationHelpers.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest, in test files
Do not usefatalErrorfor expectation failures; prefer Swift Testing’s#require
Only add labels to test cases or suites when the label differs from the suite struct or test method name
Tag tests per CONTRIBUTING.md’s "Attributing tests to a spec point" with exact comment format; distinguish@specvs@specPartial; do not repeat@specfor the same spec point
Add comments in tests to clarify when certain test data is irrelevant to the scenario
In tests, import Ably usingimport Ably
In tests, import AblyLiveObjects using@testable import AblyLiveObjects
In tests, import_AblyPluginSupportPrivateusingimport _AblyPluginSupportPrivate(do not useinternal import)
When passing a logger to internal components in tests, useTestLogger()
When unwrapping optionals in tests, prefer#requireoverguard let
Files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
🧠 Learnings (9)
📓 Common learnings
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift:239-250
Timestamp: 2025-09-03T11:54:29.104Z
Learning: In the ably-liveobjects-swift-plugin codebase (following ably-cocoa conventions), the userCallbackQueue and internalQueue are always different queues by design. This prevents deadlock scenarios with nested withLock calls when callbacks need to update internal state, making the nested withLock pattern safe.
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift:239-250
Timestamp: 2025-09-03T11:54:29.105Z
Learning: In the ably-liveobjects-swift-plugin codebase (following ably-cocoa conventions), the userCallbackQueue and internalQueue are always different queues by design. This prevents deadlock scenarios with nested withLock calls when callbacks need to update internal state, making the nested withLock pattern safe.
📚 Learning: 2025-09-03T11:54:29.105Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift:239-250
Timestamp: 2025-09-03T11:54:29.105Z
Learning: In the ably-liveobjects-swift-plugin codebase (following ably-cocoa conventions), the userCallbackQueue and internalQueue are always different queues by design. This prevents deadlock scenarios with nested withLock calls when callbacks need to update internal state, making the nested withLock pattern safe.
Applied to files:
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftCONTRIBUTING.md
📚 Learning: 2025-09-03T11:58:36.335Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift:152-163
Timestamp: 2025-09-03T11:58:36.335Z
Learning: The updateSelfLater closures in the ably-liveobjects-swift-plugin codebase are stored by the subscription system and invoked asynchronously later (e.g., during unsubscribe operations), not immediately during the current execution context. Therefore, using withLock inside these closures is safe and correct, as they execute outside the original locking context.
Applied to files:
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : In tests, import AblyLiveObjects using `testable import AblyLiveObjects`
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftCONTRIBUTING.md
📚 Learning: 2025-09-03T11:58:36.335Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift:152-163
Timestamp: 2025-09-03T11:58:36.335Z
Learning: The updateSelfLater closures in the ably-liveobjects-swift-plugin subscription system are stored during the subscribe call and only invoked later during unsubscribe operations (via SubscribeResponse.unsubscribe()). This means using withLock inside these closures is safe since they execute in separate contexts from the original subscription call.
Applied to files:
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
📚 Learning: 2025-09-03T11:55:37.381Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift:8-16
Timestamp: 2025-09-03T11:55:37.381Z
Learning: In Swift 5.10 and later, `nonisolated(unsafe)` can be used on stored properties and variables in classes (not just actors) to opt out of actor isolation checks. This eliminates the need for unchecked Sendable wrappers in scenarios where specific instances require opt-out from Sendable constraints across isolation boundaries. The pattern `private nonisolated(unsafe) var value: T` in a `final class DispatchQueueMutex<T>: Sendable` is valid and compiles correctly.
Applied to files:
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Utility/DispatchQueueMutex.swift
📚 Learning: 2025-09-03T11:56:46.656Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift:8-8
Timestamp: 2025-09-03T11:56:46.656Z
Learning: `nonisolated(unsafe)` is valid on class properties in Swift 5.10+, not just actor properties. The attribute can be used on stored properties and variables in classes to opt out of actor isolation checks and Sendable constraints. This corrects the misconception that it's only valid in actor contexts.
Applied to files:
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift
📚 Learning: 2025-08-22T20:27:49.043Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#79
File: Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift:246-258
Timestamp: 2025-08-22T20:27:49.043Z
Learning: In Swift, `mutex.withLock { ... }` closures with single expressions automatically return the result of that expression, so explicit return statements are not needed when the closure contains only a call like `mutableState.on(...)` that returns the expected type.
Applied to files:
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
📚 Learning: 2025-08-26T11:32:26.789Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-08-26T11:32:26.789Z
Learning: Applies to Sources/AblyLiveObjects/**/*.swift : In AblyLiveObjects library (non-test) code, import modules as: Ably with `import Ably`, and _AblyPluginSupportPrivate with `internal import _AblyPluginSupportPrivate`
Applied to files:
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
🧬 Code graph analysis (9)
Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
entries(136-140)
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (4)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (6)
createInternalQueue(688-691)counterObjectState(229-244)objectOperation(336-356)counterCreateOperation(374-383)counterOp(386-388)mapCreateOperation(359-371)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (4)
createZeroValued(87-104)nosync_replaceData(277-293)nosync_mergeInitialValue(296-307)nosync_apply(324-344)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_replaceData(82-107)nosync_apply(33-58)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (3)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (8)
createInternalQueue(688-691)simpleMapMessage(651-661)mapCreateOperationMessage(597-611)mapObjectMessage(286-298)counterObjectMessage(301-313)objectReferenceMapEntry(508-522)rootObjectMessage(316-326)stringMapEntry(421-435)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (7)
nosync_handleObjectSyncProtocolMessage(327-339)nosync_handleObjectSyncProtocolMessage(480-580)nosync_handleObjectProtocolMessage(309-320)nosync_handleObjectProtocolMessage(583-613)testsOnly_createZeroValueLiveObject(344-354)nosync_onChannelAttached(294-302)nosync_onChannelAttached(451-477)
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (6)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (3)
nosync_subscribe(84-100)on(102-120)unsubscribeAll(130-132)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
createListener(36-58)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
subscribe(215-229)on(237-251)unsubscribeAll(231-235)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withLock(25-35)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
get(124-128)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (4)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (5)
nosync_getObjectFromPool(144-148)on(264-278)on(670-688)offAll(280-284)offAll(698-700)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withLock(25-35)withoutLock(43-47)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (4)
nosync_subscribe(84-100)unsubscribeAll(130-132)on(102-120)offAll(134-136)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
createZeroValueObject(222-272)
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (1)
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (4)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withLock(25-35)withoutLock(43-47)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
nosync_getObjectFromPool(21-23)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
entries(136-140)on(237-251)nosync_apply(324-344)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (7)
nosync_getOrCreateMap(434-471)nosync_getOrCreateCounter(387-423)createZeroValueObject(222-272)nosync_performGarbageCollection(486-518)nosync_reset(474-483)nosync_applySyncObjectsPool(275-376)nosync_apply(33-58)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (6)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (7)
createZeroValued(87-104)get(124-128)nosync_replaceData(277-293)entries(136-140)nosync_mergeInitialValue(296-307)nosync_apply(324-344)set(152-182)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_replaceData(82-107)nosync_apply(33-58)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
createListener(36-58)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
setPublishHandler(33-37)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~83-~83: Consider a more concise word here.
Context: ...eady on the queue; favour our extension in order to avoid deadlock. ### Testing guidelines...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Generate code coverage
🔇 Additional comments (45)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved (2)
2-2: Origin hash bump looks correct after dependency state changes.No action needed.
9-9: Verified revision pin and TODOs Package.swift pins ably-cocoa to revision 8dde3e841aa1f861176c1341cf44e92014b95857 (matching Package.resolved) and includes TODO comments to unpin before the next release.Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (3)
191-195: Good switch to DispatchQueueMutex and internalQueue injectionUsing DispatchQueueMutex with a per-test internalQueue matches the nosync model and keeps tests deterministic. LGTM.
213-215: Emit/unsubscribeAll under mutex: correct synchronization boundariesBoth emit and unsubscribeAll are protected by the same mutex, aligning with the internalQueue serialization. LGTM.
Also applies to: 219-221
231-235: Consistent internalQueue propagation across testsCreating a per-test internalQueue and threading it through MockCoreSDK and stores is consistent and clear. LGTM.
Also applies to: 253-257, 279-283, 306-309
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
24-32: nosync_realtimeObjects accessor: clear failure mode and internal-only castThe guarded lookup with a strong failure message is appropriate. Forced cast is fine internally given we control the set path. LGTM.
56-58: Forwarders to InternalDefaultRealtimeObjects: aligned with nosync modelSimple delegations into the channel-scoped objects with nosync entry points look good.
Also applies to: 107-134
Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (2)
103-110: Rename and nosync_ usage look correctThe
nosync_creationOperationForLiveMaprename and switch tonosync_toObjectDataalign with the internal-queue model.
103-106: All call sites updated; no stale references found
Search forcreationOperationForLiveMap(returned no matches in Sources/** or Tests/**.Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (1)
3-15: Deadlock-guarded sync helpers look solidThe precondition + sync pairing is concise and correctly generic. Good internal scoping.
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
37-47: withoutLock on-queue precondition is appropriateThe on-queue precondition enforces correct usage and matches the nosync_ pattern elsewhere.
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (2)
38-40: Good internalQueue plumbing and nosync usage.All mutating reads/writes are correctly wrapped in internalQueue.ably_syncNoDeadlock and use nosync_* paths, matching the concurrency model.
Also applies to: 57-59, 79-81, 91-93, 109-111, 122-126, 145-147, 166-169, 193-195, 203-205, 222-224, 241-244, 268-271, 312-314, 339-345, 353-361, 388-396, 424-427, 436-444, 470-478, 492-495, 515-518, 565-568, 590-593
496-505: Error assertions are precise and stable.Asserting ARTErrorInfo code/status and message substrings keeps tests robust. LGTM.
Also applies to: 519-528, 573-581
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (5)
11-22: Factory helper reads well; explicit internalQueue is clear.createDefaultRealtimeObjects wires logger/internalQueue/userCallbackQueue/clock cleanly.
31-47: Correct sequencing for OBJECT_SYNC (single and multi-message).The tests exercise sequence start/continuation/end and assert pool mutations and sync clearing as expected.
Also applies to: 74-81, 108-114
150-163: Buffered OBJECT handling during sync is well-covered.Buffering, discard-on-new-sequence, and post-sync application paths are asserted clearly.
Also applies to: 168-174, 188-205, 216-223
424-431: Subscription assertions validate callback threading.Using Subscriber with callbackQueue: .main aligns with userCallbackQueue; emitted updates are checked deterministically.
Also applies to: 433-445, 468-501
600-619: getRoot waiting logic is exercised across all completion modes.Solid coverage for ATTACHED(HAS_OBJECTS=false), single OBJECT_SYNC, multi-message sync, and already-synced cases.
Also applies to: 624-648, 665-714
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (6)
14-19: Read-only accessors validate error and value-matrix well.Matrix of RTLM5d2 value kinds, tombstones, and missing refs is thorough.
Also applies to: 34-38, 49-52, 58-63, 70-76, 83-88, 95-101, 108-113, 121-126, 134-139, 145-151, 162-171
193-205: replaceData paths cover siteTimeserials, createOp flag, and entry merges.Nice separation of cases with/without createOp and combined merging.
Also applies to: 200-204, 212-217, 221-233
346-361: Accessors (size/entries/keys/values) filter tombstones consistently.The consistency checks across properties are helpful to catch drift.
Also applies to: 395-406, 432-458
498-527: MAP_SET applicability and side-effects are well asserted.Covers timeserial rules, pool zero-value creation, and returned updates.
Also applies to: 533-597, 615-659
718-777: MAP_REMOVE tests cover canApply, state updates, and return updates.Good separation of existing vs. non-existing entry cases.
Also applies to: 786-801, 805-814
1096-1115: Publishing tests assert full ObjectMessage shape.Message structure (action, objectId, op payload) and error propagation are checked precisely.
Also applies to: 1161-1169, 1218-1226, 1274-1282, 1296-1321, 1357-1372, 1408-1424, 1454-1484, 1489-1505
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (8)
44-62: Initialization and zero-valued constructor are thread-safe and explicit.mutableStateMutex is bound to internalQueue and state is fully initialized; explicit ACLs match guidelines.
Also applies to: 64-80, 82-104
276-292: Pass-through of internalQueue/userCallbackQueue in replaceData is correct.The nosync_* boundary is respected via withoutLock; logger/clock are forwarded.
296-307: nosync_ mutators correctly require caller to be on internalQueue.*withoutLock usage with mutableStateMutex.dispatchQueue passed downstream is consistent.
Also applies to: 323-344
835-845: resetData emits .removed per prior contents.Emission on userCallbackQueue preserves callback isolation.
877-895: Channel state validation is enforced before access.Using nosync_validateChannelState with precise operation descriptions is good.
Also applies to: 896-905
926-943: Pool lookups and tombstone propagation are correct.nosync_getObjectFromPool + nosync_isTombstone ensures references don't surface tombstoned objects.
Also applies to: 984-1007
1-3: Module imports conform to repo guidelines.internal import _AblyPluginSupportPrivate and import Ably are correct.
712-718: No typo in ObjectsPool.swift The identifierobjects_pooldoes not exist—nosync_applycorrectly forwards to.mapand.counterwithobjectMessageSerial.Likely an incorrect or invalid review comment.
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (13)
6-7: LGTM! The threading model switch to DispatchQueue looks good.The replacement of
NSLockwithDispatchQueueMutexaligns well with ably-cocoa's conventions and provides a cleaner concurrency model. From learnings, I understand this pattern is safe becauseuserCallbackQueueandinternalQueueare always different queues by design.
91-115: Good implementation of the threading architecture.The initialization properly threads the
internalQueuethrough all components, ensuring consistent isolation of mutable state to the ARTRealtime instance's internal queue as intended.
144-148: Correct use ofwithoutLockfor nosync_ methods.The
nosync_prefix appropriately indicates this method must be called on the internal queue, andwithoutLockcorrectly enforces this precondition.
268-276: Good implementation of the updateSelfLater closure pattern.The pattern of storing closures for asynchronous invocation avoids deadlocks as documented in the learnings. The weak self capture prevents retain cycles.
294-302: Consistent nosync_ pattern implementation.The method correctly uses
withoutLockand threads through required parameters while maintaining the nosync_ convention.
309-320: Proper threading of internalQueue through object protocol message handling.The
nosync_variant correctly passes the internalQueue from the mutex's dispatchQueue to downstream operations.
327-339: Consistent implementation of nosync_ pattern for sync messages.The method properly threads the internalQueue through to the mutable state operations.
345-353: Test helper properly updated for new threading model.The test helper correctly uses
withLockand passes the internalQueue to the object creation method.
367-374: Garbage collection correctly updated for nosync_ pattern.The method properly passes all required parameters to the nosync_ variant.
552-558: Consistent threading of internalQueue through sync operations.Both
nosync_applySyncObjectsPoolandnosync_applyObjectProtocolMessageObjectMessagecorrectly receive and use the internalQueue parameter.Also applies to: 564-571
616-640: Object creation properly threads internalQueue.The zero-value object creation correctly passes the internalQueue parameter when creating new objects during OBJECT message processing.
653-659: nosync_apply correctly handles object pool mutations.The method properly applies operations with the object pool passed by reference.
668-668: Good use of @sendable for closure type alias.The
UpdateMutableStatetype alias correctly marks the closure as@Sendableto ensure thread safety.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
Outdated
Show resolved
Hide resolved
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
Outdated
Show resolved
Hide resolved
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
5540135 to
a99fb43
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
Isolate the plugin's mutable state to the ARTRealtime instance's internal queue, for consistency with ably-cocoa. Will unpin ably-cocoa and ably-cocoa-plugin-support before our next release. Resolves #3.
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
from
a99fb43 to
b86e09a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/85/AblyLiveObjects
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (24)
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (2)
21-26: Use the local pluginAPI variable for consistencyPass the pluginAPI you already fetched instead of Plugin.defaultPluginAPI again.
Apply this diff:
- pluginAPI: Plugin.defaultPluginAPI, + pluginAPI: pluginAPI,
37-40: Remove explanatory doc comment on testsOnly_ APIPer repo guidance, testsOnly_ properties shouldn't carry generic explanatory comments.
As per coding guidelines
- /// For tests to access the non-public API of `PublicDefaultRealtimeObjects`. internal var testsOnly_nonTypeErasedObjects: PublicDefaultRealtimeObjects { nonTypeErasedObjects }Sources/AblyLiveObjects/Internal/ObjectsPool.swift (6)
82-87: Remove unused userCallbackQueue parameteruserCallbackQueue is not used in Entry.nosync_replaceData and the callees already capture their own callback queues. Simplify the API and its call site.
Apply this diff:
- fileprivate func nosync_replaceData( + fileprivate func nosync_replaceData( using state: ObjectState, objectMessageSerialTimestamp: Date?, objectsPool: inout ObjectsPool, - userCallbackQueue: DispatchQueue, ) -> DeferredUpdate {And update the call site:
- let deferredUpdate = existingEntry.nosync_replaceData( + let deferredUpdate = existingEntry.nosync_replaceData( using: syncObjectsPoolEntry.state, objectMessageSerialTimestamp: syncObjectsPoolEntry.objectMessageSerialTimestamp, - objectsPool: &self, - userCallbackQueue: userCallbackQueue, + objectsPool: &self, )Also applies to: 299-304
128-146: Drop testsOnly_ doc commentsThese comments are redundant per repo standards.
As per coding guidelines
- /// Test-only accessor for isTombstone that handles locking internally. internal var testsOnly_isTombstone: Bool { switch self { case let .counter(counter): counter.testsOnly_isTombstone case let .map(map): map.testsOnly_isTombstone } } - /// Test-only accessor for tombstonedAt that handles locking internally. internal var testsOnly_tombstonedAt: Date? { switch self { case let .counter(counter): counter.testsOnly_tombstonedAt case let .map(map): map.testsOnly_tombstonedAt } }
222-228: Assert on the internal queue for nosync mutationGuarding with a dispatchPrecondition helps catch misuse early since this mutates entries and is intended to run on internalQueue.
internal mutating func createZeroValueObject( forObjectID objectID: String, logger: Logger, internalQueue: DispatchQueue, userCallbackQueue: DispatchQueue, clock: SimpleClock, ) -> Entry? { + dispatchPrecondition(condition: .onQueue(internalQueue))
275-281: Add queue precondition to nosync_applySyncObjectsPoolThis function mutates the pool and calls nosync_ APIs; assert we’re on internalQueue.
internal mutating func nosync_applySyncObjectsPool( _ syncObjectsPool: [SyncObjectsPoolEntry], logger: Logger, internalQueue: DispatchQueue, userCallbackQueue: DispatchQueue, clock: SimpleClock, ) { + dispatchPrecondition(condition: .onQueue(internalQueue))
387-393: Add queue precondition to nosync_getOrCreateCounterEnsures all mutations happen on the intended serial queue.
internal mutating func nosync_getOrCreateCounter( creationOperation: ObjectCreationHelpers.CounterCreationOperation, logger: Logger, internalQueue: DispatchQueue, userCallbackQueue: DispatchQueue, clock: SimpleClock, ) -> InternalDefaultLiveCounter { + dispatchPrecondition(condition: .onQueue(internalQueue))
434-440: Add queue precondition to nosync_getOrCreateMapSame rationale as for the counter path.
internal mutating func nosync_getOrCreateMap( creationOperation: ObjectCreationHelpers.MapCreationOperation, logger: Logger, internalQueue: DispatchQueue, userCallbackQueue: DispatchQueue, clock: SimpleClock, ) -> InternalDefaultLiveMap { + dispatchPrecondition(condition: .onQueue(internalQueue))Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
91-96: Remove testsOnly_ doc commentAvoid generic explanatory comments on testsOnly_ accessors.
As per coding guidelines
- /// Test-only accessor for objectID that handles locking internally. internal var testsOnly_objectID: String { mutableStateMutex.withSync { mutableState in mutableState.liveObjectMutableState.objectID } }
278-283: Remove testsOnly_ doc commentSame guideline applies here.
As per coding guidelines
- /// Test-only accessor for isTombstone that handles locking internally. internal var testsOnly_isTombstone: Bool { mutableStateMutex.withSync { mutableState in mutableState.liveObjectMutableState.isTombstone } }
292-297: Remove testsOnly_ doc commentSame guideline applies here.
As per coding guidelines
- /// Test-only accessor for tombstonedAt that handles locking internally. internal var testsOnly_tombstonedAt: Date? { mutableStateMutex.withSync { mutableState in mutableState.liveObjectMutableState.tombstonedAt } }Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
24-31: Avoid force-cast; fail with clearer message if the stored plugin data is wrongSafer unwrap improves diagnostics without changing behavior.
Apply this diff:
- // swiftlint:disable:next force_cast - return pluginData as! InternalDefaultRealtimeObjects + guard let objects = pluginData as? InternalDefaultRealtimeObjects else { + preconditionFailure("LiveObjects plugin data had unexpected type; ensure nosync_prepare stored InternalDefaultRealtimeObjects") + } + return objectsSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)
236-241: Align naming: use nosync_creationOperationForLiveCounter for consistencyMap creation uses the nosync_ variant; counter should too for API consistency.
Apply this diff:
- let creationOperation = ObjectCreationHelpers.creationOperationForLiveCounter( + let creationOperation = ObjectCreationHelpers.nosync_creationOperationForLiveCounter( count: count, timestamp: timestamp, )
451-457: Minor log message typoMissing closing parenthesis in the log string.
Apply this diff:
- logger.log("onChannelAttached(hasObjects: \(hasObjects)", level: .debug) + logger.log("onChannelAttached(hasObjects: \(hasObjects))", level: .debug)
54-57: Drop generic docs on testsOnly_ properties per guidelinesPer our coding guidelines, avoid explanatory docs on testsOnly_ properties in Sources. Consider removing these doc comments.
As an example, remove the doc above testsOnly_waitingForSyncEvents:
- /// Emits an element whenever `getRoot()` starts waiting for the object sync sequence to complete per RTO1c. internal var testsOnly_waitingForSyncEvents: AsyncStream<Void> { waitingForSyncEvents }Based on coding guidelines
Also applies to: 288-292, 381-383
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (2)
16-22: Guard publish handler read with the same lock to avoid racesSetter is locked but the read isn’t; lock the read for symmetry and thread-safety (even in tests).
Apply this diff:
- func publish(objectMessages: [OutboundObjectMessage]) async throws(InternalError) { - if let handler = _publishHandler { - try await handler(objectMessages) - } else { - protocolRequirementNotImplemented() - } - } + func publish(objectMessages: [OutboundObjectMessage]) async throws(InternalError) { + let handler = mutex.withLock { _publishHandler } + if let handler { + try await handler(objectMessages) + } else { + protocolRequirementNotImplemented() + } + }
24-26: Implement testsOnly_overridePublish to actually overrideWire this to the backing handler so tests can use it.
Apply this diff:
- func testsOnly_overridePublish(with _: @escaping ([OutboundObjectMessage]) async throws(InternalError) -> Void) { - protocolRequirementNotImplemented() - } + func testsOnly_overridePublish(with handler: @escaping ([OutboundObjectMessage]) async throws(InternalError) -> Void) { + mutex.withLock { _publishHandler = handler } + }Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (1)
242-245: Prefer explicit Set literal for clarityUse Set([...]) on the RHS for consistency with the rest of the file and to avoid relying on type inference.
Apply:
- #expect(Set(pool.entries.keys) == ["root", "counter:hash@456"]) + #expect(Set(pool.entries.keys) == Set(["root", "counter:hash@456"]))Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (2)
351-355: Fix misleading comment about tombstoningThe comment says “tombstonedAt is false” but a Date is passed; this entry is tombstoned.
Apply:
- // tombstonedAt is false, so not considered tombstoned + // tombstonedAt is set, so considered tombstoned
1122-1128: Correct the comment to reflect the assertionYou state “will be applied” when ts1 < ts2; the code asserts it’s discarded. Update the comment.
Apply:
- // Apply operation with serial "ts1" which is lexicographically less than existing "ts2" and thus will be applied per RTLO4a (this is a non-pathological case of RTOL4a, that spec point being fully tested elsewhere) + // Apply operation with serial "ts1" which is lexicographically less than existing "ts2" and thus will be discarded per RTLO4a (this is a non-pathological case of RTLO4a, that spec point being fully tested elsewhere)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (4)
901-903: Ensure count(where:) is available, or use a stdlib-safe alternativeSequence.count(where:) isn’t in older stdlib versions. If you don’t define this extension, this won’t compile.
If you want to avoid relying on an extension, use filter.count:
- return data.values.count { entry in - !Self.nosync_isEntryTombstoned(entry, delegate: delegate) - } + return data.values + .filter { !Self.nosync_isEntryTombstoned($0, delegate: delegate) } + .countIf you already have a project-wide count(where:) extension, ignore this change. Based on guidelines
114-119: Remove generic comments on testsOnly_ accessorsPer repo guidelines, avoid generic explanatory comments for testsOnly_ properties.
Apply:
- /// Test-only accessor for objectID that handles locking internally. internal var testsOnly_objectID: String {As per coding guidelines
407-412: Remove generic comments on testsOnly_ accessorsSame guideline applies here.
Apply:
- /// Test-only accessor for isTombstone that handles locking internally. internal var testsOnly_isTombstone: Bool {As per coding guidelines
421-426: Remove generic comments on testsOnly_ accessorsSame guideline applies here.
Apply:
- /// Test-only accessor for tombstonedAt that handles locking internally. internal var testsOnly_tombstonedAt: Date? {As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)CONTRIBUTING.md(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(3 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(4 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(9 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(32 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(22 hunks)Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift(2 hunks)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(22 hunks)Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift(1 hunks)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift(1 hunks)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(22 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(51 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(53 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(3 hunks)Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift(8 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift
- Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift
- Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift
- Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift
- Package.swift
- Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift
- CONTRIBUTING.md
- Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift
- Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
- Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)
Prefer implicit .init(...) when the type can be inferred in initializer expressions
Prefer enum case shorthand (.caseName) when the type can be inferred
For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Prefer Swift raw string literals for JSON strings instead of escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swiftTests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftSources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest, in test files
Do not usefatalErrorfor expectation failures; prefer Swift Testing’s#require
Only add labels to test cases or suites when the label differs from the suite struct or test method name
Tag tests per CONTRIBUTING.md’s "Attributing tests to a spec point" with exact comment format; distinguish@specvs@specPartial; do not repeat@specfor the same spec point
Add comments in tests to clarify when certain test data is irrelevant to the scenario
In tests, import Ably usingimport Ably
In tests, import AblyLiveObjects using@testable import AblyLiveObjects
In tests, import_AblyPluginSupportPrivateusingimport _AblyPluginSupportPrivate(do not useinternal import)
When passing a logger to internal components in tests, useTestLogger()
When unwrapping optionals in tests, prefer#requireoverguard let
Files:
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swiftTests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift
Sources/AblyLiveObjects/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
In AblyLiveObjects library (non-test) code, import modules as: Ably with
import Ably, and _AblyPluginSupportPrivate withinternal import _AblyPluginSupportPrivate
Files:
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)
Files:
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
🧠 Learnings (1)
📚 Learning: 2025-09-03T11:54:29.136Z
Learnt from: lawrence-forooghian
PR: ably/ably-liveobjects-swift-plugin#85
File: Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift:239-250
Timestamp: 2025-09-03T11:54:29.136Z
Learning: In the ably-liveobjects-swift-plugin codebase (following ably-cocoa conventions), the userCallbackQueue and internalQueue are always different queues by design. This prevents deadlock scenarios with nested withLock calls when callbacks need to update internal state, making the nested withLock pattern safe.
Applied to files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
🧬 Code graph analysis (12)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withoutSync(43-47)
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (5)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
createZeroValued(66-81)value(100-104)subscribe(150-164)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
createZeroValued(87-104)entries(136-140)subscribe(215-229)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
createZeroValueObject(222-272)nosync_applySyncObjectsPool(275-376)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (2)
Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
nosync_realtimeObjects(24-32)nosync_realtimeObjects(56-58)
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (4)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (6)
createInternalQueue(688-691)counterObjectState(229-244)objectOperation(336-356)counterCreateOperation(374-383)counterOp(386-388)mapCreateOperation(359-371)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
createZeroValued(66-81)nosync_replaceData(211-224)value(100-104)nosync_mergeInitialValue(227-231)nosync_apply(248-267)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_replaceData(82-107)nosync_apply(33-58)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (7)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (9)
createInternalQueue(688-691)internalMapEntry(408-418)objectState(189-205)mapCreateOperation(359-371)stringMapEntry(421-435)mapObjectState(208-226)internalStringMapEntry(440-454)objectOperation(336-356)counterCreateOperation(374-383)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (6)
createZeroValued(66-81)value(100-104)nosync_replaceData(211-224)nosync_mergeInitialValue(227-231)nosync_apply(248-267)subscribe(150-164)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (8)
createZeroValued(87-104)get(124-128)nosync_replaceData(277-293)entries(136-140)nosync_mergeInitialValue(296-307)nosync_apply(324-344)subscribe(215-229)set(152-182)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_replaceData(82-107)nosync_apply(33-58)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
setPublishHandler(33-37)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
publishesCorrectObjectMessage(534-560)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (4)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (3)
nosync_handleObjectSyncProtocolMessage(123-134)nosync_handleObjectProtocolMessage(111-121)nosync_onChannelAttached(107-109)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (7)
nosync_handleObjectSyncProtocolMessage(327-339)nosync_handleObjectSyncProtocolMessage(480-580)nosync_handleObjectProtocolMessage(309-320)nosync_handleObjectProtocolMessage(583-613)testsOnly_createZeroValueLiveObject(344-354)nosync_onChannelAttached(294-302)nosync_onChannelAttached(451-477)
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (6)
nosync_onChannelAttached(294-302)nosync_onChannelAttached(451-477)nosync_handleObjectProtocolMessage(309-320)nosync_handleObjectProtocolMessage(583-613)nosync_handleObjectSyncProtocolMessage(327-339)nosync_handleObjectSyncProtocolMessage(480-580)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withSync(25-35)withoutSync(43-47)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (4)
nosync_subscribe(84-100)unsubscribeAll(130-132)on(102-120)offAll(134-136)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (8)
unsubscribeAll(231-235)on(237-251)offAll(253-257)nosync_emit(264-268)nosync_replaceData(277-293)nosync_mergeInitialValue(296-307)mergeInitialValue(518-576)nosync_apply(324-344)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
nosync_emit(66-73)nosync_replaceData(82-107)nosync_apply(33-58)
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (5)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)Sources/AblyLiveObjects/Utility/DispatchQueue+Extensions.swift (2)
ably_syncNoDeadlock(5-8)ably_syncNoDeadlock(11-14)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (4)
nosync_subscribe(84-100)emit(138-147)on(102-120)unsubscribeAll(130-132)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
createListener(36-58)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withSync(25-35)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (7)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withSync(25-35)withoutSync(43-47)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
nosync_getObjectFromPool(21-23)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
entries(136-140)on(237-251)nosync_apply(324-344)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (7)
nosync_getOrCreateMap(434-471)nosync_getOrCreateCounter(387-423)createZeroValueObject(222-272)nosync_performGarbageCollection(486-518)nosync_reset(474-483)nosync_applySyncObjectsPool(275-376)nosync_apply(33-58)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (2)
on(172-186)nosync_apply(248-267)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (3)
nosync_onChannelAttached(107-109)nosync_handleObjectProtocolMessage(111-121)nosync_handleObjectSyncProtocolMessage(123-134)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
nosync_apply(248-267)nosync_emit(199-203)nosync_replaceData(211-224)createZeroValued(66-81)nosync_mergeInitialValue(227-231)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (7)
nosync_apply(324-344)nosync_emit(264-268)nosync_replaceData(277-293)createZeroValued(87-104)nosync_mergeInitialValue(296-307)nosync_resetData(385-389)nosync_releaseTombstonedEntries(392-396)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (6)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)
nosync_getObjectFromPool(144-148)on(264-278)on(670-688)Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
nosync_getObjectFromPool(21-23)Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (2)
withSync(25-35)withoutSync(43-47)Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
nosync_validateChannelState(99-111)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (2)
nosync_subscribe(84-100)on(102-120)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
createZeroValueObject(222-272)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Generate code coverage
- GitHub Check: lint
🔇 Additional comments (23)
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (1)
13-16: Good use of ably_syncNoDeadlock to avoid re-entrancy deadlocksRunning the lookup on the client's internal queue behind ably_syncNoDeadlock is the right pattern here.
Package.resolved (1)
2-2: Pin updates acknowledged; verify CI across the pinned commitsLooks fine. Please confirm CI builds and tests pass against these revisions; plan to unpin before release as noted in the PR.
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
32-57: nosync_apply correctly delegates to map/counter variantsThe split to nosync_apply for each object type is consistent with the new locking model.
66-73: Deferred updates emitted via nosync_emit is correctDeferring emission until after pool reconciliation matches spec intent.
Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
84-86: Channel state via nosync_ accessor looks goodDirectly reading via pluginAPI.nosync_state(for:) aligns with the new model.
99-111: nosync_validateChannelState is correctValidation and error mapping match expectations.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (6)
101-104: Value path conforms to nosync/withSync disciplineReads under withSync then propagates typed error via CoreSDK; good.
108-136: Increment path validates and builds message under lock, publishes outsideThis follows spec points and avoids holding the lock during await.
152-164: Subscription wiring consistent with LiveMapUsing withSync at subscribe time and deferring state updates via updateSelfLater is appropriate.
174-185: Lifecycle on(event:) mirrors the subscribe pathLooks correct and consistent.
199-203: Emit on userCallbackQueue withoutSync is correctPreconditioned on internal queue; emission happens on callback queue.
470-476: nosync_value validates channel state then returns dataMatches RTLC5 behavior.
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
37-53: LGTM: queues threaded correctly via nosync_prepareConstructs InternalDefaultRealtimeObjects with internalQueue/callbackQueue and stores it through nosync_setPluginDataValue. Matches the threading model.
146-156: No qualification needed for PluginAPIProtocol
DefaultInternalPlugin.swift hasinternal import _AblyPluginSupportPrivate, so the unqualifiedPluginAPIProtocolcorrectly refers to_AblyPluginSupportPrivate.PluginAPIProtocol.Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
686-691: LGTM: test queue helperCreates a unique serial internalQueue with appropriate QoS; fits the new threading model.
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (3)
131-133: Replace fatalError with #require in testsPer testing guidelines, avoid crashing the process.
Apply this diff:
- try mutableState.nosync_subscribe(listener: subscriber.createListener(), coreSDK: coreSDK, updateSelfLater: { _ in fatalError("Not expected") }) + try mutableState.nosync_subscribe( + listener: subscriber.createListener(), + coreSDK: coreSDK, + updateSelfLater: { _ in #require(false, "Not expected to be invoked") } + )As per coding guidelines
154-156: Replace fatalError with #require in testsSame rationale as above.
Apply this diff:
- _ = try mutableState.nosync_subscribe(listener: subscriber.createListener(), coreSDK: coreSDK, updateSelfLater: { _ in fatalError("Not expected") }) + _ = try mutableState.nosync_subscribe( + listener: subscriber.createListener(), + coreSDK: coreSDK, + updateSelfLater: { _ in #require(false, "Not expected to be invoked") } + )As per coding guidelines
176-178: Replace fatalError with #require in testsSame rationale as above.
Apply this diff:
- _ = try mutableState.nosync_subscribe(listener: subscriber.createListener(), coreSDK: coreSDK, updateSelfLater: { _ in fatalError("Not expected") }) + _ = try mutableState.nosync_subscribe( + listener: subscriber.createListener(), + coreSDK: coreSDK, + updateSelfLater: { _ in #require(false, "Not expected to be invoked") } + )As per coding guidelines
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (1)
109-111: Good use of internalQueue for nosync pathWrapping nosync_applySyncObjectsPool in internalQueue.ably_syncNoDeadlock is correct and aligns with the intended threading model.
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (2)
41-47: LGTM: Sync handling on internalQueueUsing internalQueue.ably_syncNoDeadlock with nosync_handleObjectSyncProtocolMessage is correct and faithfully exercises the internal queue constraints.
155-162: LGTM: Discarding in-flight sync on new sequenceThe test accurately validates RTO5a2 behavior with buffering and discard semantics. Nice setup via OBJECT before switching sequences.
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
1454-1460: LGTM: Missing @test was addressedThe test now has @test and will execute under the Swift Testing runner.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
239-250: Nested withSync in callbacks is acceptable under your queue separationGiven internalQueue and userCallbackQueue are distinct by design, this pattern won’t deadlock. No change needed.
Based on learnings
Mistake in d9c327e.
Isolate the plugin's mutable state to the
ARTRealtimeinstance's internal queue, for consistency with ably-cocoa.Will unpin ably-cocoa and ably-cocoa-plugin-support before our next release.
Related:
Resolves #3.
Summary by CodeRabbit
Refactor
Documentation
Chores
Tests