[ECO-5076][LiveObjects] Implement accessors for objects, livemap and livecounter #1120
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis update introduces major enhancements to the Live Objects feature set. It implements the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LiveObjects
participant ObjectsManager
participant ObjectsStateCoordinator
participant LiveMap
User->>LiveObjects: getRoot()
LiveObjects->>ObjectsManager: ensureSynced()
ObjectsManager->>ObjectsStateCoordinator: (await SYNCED)
ObjectsManager-->>LiveObjects: (sync complete)
LiveObjects->>LiveMap: retrieve root map
LiveObjects-->>User: return LiveMap
sequenceDiagram
participant User
participant LiveObjects
participant ObjectsStateCoordinator
User->>LiveObjects: on(event, listener)
LiveObjects->>ObjectsStateCoordinator: register listener
ObjectsStateCoordinator-->>User: return ObjectsSubscription
Note over ObjectsStateCoordinator: On state change
ObjectsStateCoordinator->>User: listener.onStateChanged(event)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the linked issues. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: in the ably-java liveobjects test code, extension properties with capital letter names (like `state`...Applied to files:
📚 Learning: in the ably-java liveobjects test code, an extension property `state` (capital s) is defined on the ...Applied to files:
📚 Learning: in the ably-java codebase, the defaultliveobjectserializer class methods like writemsgpackarray will...Applied to files:
📚 Learning: the liveobjects interface does not currently include public api methods for resource management (dis...Applied to files:
📚 Learning: in liveobjects implementation (lib/src/main/java/io/ably/lib/objects/liveobjectsadapter.java), the s...Applied to files:
📚 Learning: in test utility code for the ably java live objects module, the team prefers to keep reflection-base...Applied to files:
📚 Learning: the sandbox.kt file in ably-java live-objects module already has comprehensive http retry mechanism ...Applied to files:
📚 Learning: the ably-java project prefers to use the latest available versions of testing dependencies (includin...Applied to files:
📚 Learning: in the ably-java codebase, the defaultliveobjectserializer class uses intentional unsafe casting (`o...Applied to files:
📚 Learning: in the ably java liveobjects messagepack deserialization code, the `action` field in objectoperation...Applied to files:
📚 Learning: in livemapmanagertest.kt, the private field `livemapmanager` is used extensively in the `shouldcalcu...Applied to files:
📚 Learning: in livemapmanagertest.kt, the private field `livemapmanager` is used in the `shouldcalculatemapdiffe...Applied to files:
⏰ 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). (11)
🔇 Additional comments (10)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sacOO7
changed the title
1. Added getChannelModes and getChannelState methods to adapter 2. Added extension helper methods to adapter to ensure channel config. is valid
sacOO7
force-pushed
the
feature/objects-getroot
branch
from
3de1532 to
111e15a
Compare
1. Added ObjectsEvents and emitter for the same 2. Implemented stateChange mechanism for objectsState changes 3. updated getRootAsync to ensure objects are synced
• Add ObjectsStateEvent enum for tracking Live Objects synchronization states (SYNCING/SYNCED) • Implement ObjectsStateListener interface for receiving state change notifications • Create ObjectsStateSubscription interface for managing listener subscriptions and cleanup • Update LiveObjects, DefaultLiveObjects, ObjectsManager, ObjectsState, and Utils classes to support state management
sacOO7
changed the title
… polluting LiveObjects - Added impl. for the same in ObjectsState.kt - Extended the impl. in objectsmanager
ttypic
reviewed
Jul 17, 2025
| /** | ||
| * Coroutine scope for handling callbacks asynchronously. | ||
| */ | ||
| private val callbackScope = CoroutineScope(Dispatchers.Default + CoroutineName("LiveObjectsCallback-$channelName")) |
There was a problem hiding this comment.
I think we can use single scope to manage live objects, I don't think we need two. Scope is mostly to group coroutines together and cancel them at the same time. For sequential execution we just need context, I suggest:
private val scope =
CoroutineScope(Dispatchers.Default + CoroutineName("LiveObjects-$channelName") + SupervisorJob())
private val sequentialContext = Dispatchers.Default.limitedParallelism(1)
// ...
override fun getRootAsync(callback: Callback<LiveMap>) {
scope.launchWithCallback(callback) { getRootAsync() }
}
private suspend fun getRootAsync(): LiveMap = withContext(sequentialContext) {
adapter.throwIfInvalidAccessApiConfiguration(channelName)
objectsManager.ensureSynced(state)
objectsPool.get(ROOT_OBJECT_ID) as LiveMap
}There was a problem hiding this comment.
Actually, I was planning to do something like that, but we are already using sequentialScope in non-suspend methods initializeHandlerForIncomingObjectMessages and handleStateChange.
There was a problem hiding this comment.
Okay, I updated getRootAsync to use withContext to avoid new coroutine overhead.
And removed callbackScope from DefaultLiveObjects.kt.
There was a problem hiding this comment.
Added GlobalCallbackScope to utils.
This will be shared by
DefaultLiveObjects, DefaultLiveMap and DefaultLiveCounter async public API methods.
There was a problem hiding this comment.
I thought about this for some time, I am not sure GlobalCallbackScope is good thing, because we never close it even if we release channel, also no sure it has any advantage over built-in GlobalScope. Ideally I would use internal scope bind to the channel lifecycle or RealtimeClient lifecycle
There was a problem hiding this comment.
Hmm.. as per GlobalScope doc, global scope has limited use-cases and can run coroutines indefinitely since it's not meant for structured concurrency. Will create objects level scope and will dispose it when dispose method is called.
There was a problem hiding this comment.
Okay, updated code to have channel/objects lifecycle specific scope -> 8b5ea81
|
|
||
| override fun off(listener: ObjectsStateChange.Listener) = objectsManager.off(listener) | ||
|
|
||
| override fun offAll() = objectsManager.offAll() |
There was a problem hiding this comment.
let's not expose offAll() at least for now. offAll, unsubscribeAll - are "gigantic foot guns" (c) Andy
There was a problem hiding this comment.
Same exists in case of ably-js objects#offAll, need to disusss with team yes
There was a problem hiding this comment.
Also, we have unsubscribeAll in case of object-subscriptions. You can check ably/specification#346
sacOO7
force-pushed
the
feature/objects-getroot
branch
from
1bacaf5 to
113c1b3
Compare
sacOO7
force-pushed
the
feature/objects-getroot
branch
from
113c1b3 to
f737cad
Compare
- Updated usage doc for the same
sacOO7
force-pushed
the
feature/objects-getroot
branch
from
f737cad to
67c2b62
Compare
lifecycle tied to given objects instance
sacOO7
force-pushed
the
feature/objects-getroot
branch
from
cc4335e to
67cfb14
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (2)
62-79: Solid async callback implementation with robust error handling.The nested try-catch pattern correctly handles both operation errors and callback errors separately. The distinction between
AblyExceptionand other exceptions is appropriate, and logging callback errors without rethrowing prevents cascade failures.Consider making the error message more specific:
- val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) + val ex = ablyException("Error executing async operation", ErrorCode.BadRequest, cause = throwable)
81-98: Correct implementation for void operations.The method correctly handles
Unit-returning operations by passingnulltocallback.onSuccess(null), which is appropriate for Java interop withObjectsCallback<Void>.Consider extracting the common error handling logic to reduce duplication:
private fun handleCallbackExecution(callback: ObjectsCallback<*>, action: () -> Unit) { try { action() } catch (t: Throwable) { Log.e(tag, "Error occurred while executing callback's onSuccess handler", t) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/src/main/java/io/ably/lib/objects/LiveCounter.java(2 hunks)lib/src/main/java/io/ably/lib/objects/LiveMap.java(7 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjects.java(7 hunks)lib/src/main/java/io/ably/lib/objects/ObjectsCallback.java(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt(6 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt(2 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/UtilsTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjectsPlugin.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
- lib/src/main/java/io/ably/lib/objects/ObjectsCallback.java
- lib/src/main/java/io/ably/lib/objects/LiveCounter.java
- lib/src/main/java/io/ably/lib/objects/LiveMap.java
- lib/src/main/java/io/ably/lib/objects/LiveObjects.java
- live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/UtilsTest.kt
- live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1106
File: live-objects/src/main/kotlin/io/ably/lib/objects/serialization/Serialization.kt:25-29
Timestamp: 2025-06-23T14:18:25.315Z
Learning: In the ably-java codebase, the DefaultLiveObjectSerializer class methods like writeMsgpackArray will have their type signatures updated to be non-nullable since the calling sites ensure objects are never null when passed to these methods.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:21-32
Timestamp: 2025-06-03T09:15:15.338Z
Learning: In test utility code for the Ably Java Live Objects module, the team prefers to keep reflection-based field access utilities simple without additional error handling, allowing tests to fail fast if incorrect field names are used.
Learnt from: sacOO7
PR: ably/ably-java#1085
File: lib/src/main/java/io/ably/lib/objects/LiveObjects.java:0-0
Timestamp: 2025-05-20T13:12:19.013Z
Learning: The LiveObjects interface does not currently include public API methods for resource management (dispose) or change listeners, as these features are not yet implemented.
Learnt from: sacOO7
PR: ably/ably-java#1120
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt:0-0
Timestamp: 2025-08-01T09:53:16.717Z
Learning: In the ably-java LiveObjects test code, an extension property `State` (capital S) is defined on the `LiveObjects` interface in live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/Utils.kt to provide access to the internal `state` field by casting to `DefaultLiveObjects`. This allows tests to access internal state for verification purposes.
Learnt from: sacOO7
PR: ably/ably-java#1120
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt:0-0
Timestamp: 2025-08-01T09:53:16.717Z
Learning: In the ably-java LiveObjects test code, extension properties with capital letter names (like `State`, `ObjectId`) are defined in live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/Utils.kt to provide access to internal fields of concrete implementations through their public interfaces. For example, `LiveObjects.State` casts to `DefaultLiveObjects` to access the internal `state` field for testing purposes.
Learnt from: sacOO7
PR: ably/ably-java#1087
File: lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java:32-34
Timestamp: 2025-05-27T12:11:25.084Z
Learning: In LiveObjects implementation (lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java), the send method intentionally hardcodes queueEvents to true rather than respecting ably.options.queueMessages. This is because LiveObjects requires reliable message delivery to ensure proper state synchronization and acknowledgment, unlike other realtime components that may allow configurable queuing behavior.
Learnt from: sacOO7
PR: ably/ably-java#1113
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:16-16
Timestamp: 2025-08-01T05:50:33.016Z
Learning: In LiveMapManagerTest.kt, the private field `livemapManager` is used extensively in the `shouldCalculateMapDifferenceCorrectly` test method to test the `calculateUpdateFromDataDiff` functionality across multiple test scenarios, so it should not be removed as unused.
Learnt from: sacOO7
PR: ably/ably-java#1092
File: live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt:48-61
Timestamp: 2025-06-03T09:15:18.827Z
Learning: User sacOO7 prefers simple test utilities without extensive error handling, believing tests should fail fast if incorrect field/method names are used rather than having defensive programming.
📚 Learning: in the ably-java liveobjects test code, extension properties with capital letter names (like `state`...
Learnt from: sacOO7
PR: ably/ably-java#1120
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt:0-0
Timestamp: 2025-08-01T09:53:16.717Z
Learning: In the ably-java LiveObjects test code, extension properties with capital letter names (like `State`, `ObjectId`) are defined in live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/Utils.kt to provide access to internal fields of concrete implementations through their public interfaces. For example, `LiveObjects.State` casts to `DefaultLiveObjects` to access the internal `state` field for testing purposes.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt
📚 Learning: the sandbox.kt file in ably-java live-objects module already has comprehensive http retry mechanism ...
Learnt from: sacOO7
PR: ably/ably-java#1095
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt:87-89
Timestamp: 2025-06-06T09:28:12.298Z
Learning: The Sandbox.kt file in ably-java live-objects module already has comprehensive HTTP retry mechanism using HttpRequestRetry with 5 retries, exponential backoff, and automatic retry on non-success responses and timeout exceptions.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt
⏰ 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). (11)
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (24)
- GitHub Check: build
- GitHub Check: check (19)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check
🔇 Additional comments (3)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (3)
5-7: LGTM!The new imports are appropriate and necessary for the ObjectsAsyncScope implementation.
56-60: Well-designed coroutine scope setup.The use of
SupervisorJobensures child coroutine failures don't affect siblings, andDispatchers.DefaultwithCoroutineNameprovides good performance and debugging capabilities. The channel-specific naming is excellent for troubleshooting.
100-102: Proper cancellation implementation.Using
cancelChildren(cause)correctly cancels all child coroutines while preserving the scope, and accepting aCancellationExceptionparameter allows for meaningful cancellation context.
- Updated ObjectsAsyncScope to handle ObjectsCallback - Added launchWithVoidCallback method to handle void callbacks - Added unit tests covering all scenarios for ObjectsAsyncScope
sacOO7
force-pushed
the
feature/objects-getroot
branch
from
67cfb14 to
2ddd598
Compare
ttypic
approved these changes
Aug 1, 2025
|
Closing in favor of #1138 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores