[AIT-934] feat(liveobjects): LiveObjects implementation to new path-based API (Kotlin)#1223
Merged
sacOO7 merged 2 commits intoJul 9, 2026
Conversation
WalkthroughThis PR implements the LiveObjects runtime in the Kotlin ChangesLiveObjects Feature Implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sacOO7
changed the title
LiveObjects implementation to new path-based API (Kotlin)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.kt (1)
63-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnum-based no-op selection is a minor OCP smell.
Branching on
objectTypeto picknoOpMapUpdate/noOpCounterUpdatecouples the base class to the concrete set of subtypes. Consider exposing an abstractnoOpUpdate: ObjectUpdateproperty/hook overridden byInternalLiveMap/InternalLiveCounterinstead, consistent with the other abstract hooks (applyObjectState,clearData, etc.) already used for type-specific behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.kt` around lines 63 - 69, The tombstoned-state handling in BaseRealtimeLiveObject is branching on objectType to choose between noOpMapUpdate and noOpCounterUpdate, which tightly couples the base class to concrete subtypes. Replace that enum-based selection with an abstract noOpUpdate: ObjectUpdate hook on BaseRealtimeLiveObject, and override it in InternalLiveMap and InternalLiveCounter alongside the existing type-specific hooks like applyObjectState and clearData so tombstoned processing can just return noOpUpdate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt`:
- Around line 10-14: The getInstance implementation in DefaultLiveObjectsPlugin
uses ConcurrentHashMap.getOrPut, which can create extra DefaultRealtimeObject
instances during concurrent first access and leave losing instances undisposed.
Replace the getOrPut usage with ConcurrentMap.computeIfAbsent on objects so only
one DefaultRealtimeObject is constructed per channel, and keep the caching
behavior inside getInstance unchanged.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt`:
- Around line 315-322: The exception handling in DefaultRealtimeObject while
deriving the channel error reason is swallowing unexpected failures from
adapter.getChannel(channelName). Update the try/catch in the channel state
handling block to distinguish the expected missing-channel case from real
errors, and log or surface unexpected exceptions before returning null. Use the
surrounding getChannel(channelName) / errorReason logic to locate the fix and
keep the fallback behavior only for the anticipated case.
In `@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsManager.kt`:
- Around line 82-88: The startNewSync() flow is replacing syncCompletionWaiter
and can orphan an ACK waiter already created by applyAckResult() when the
manager is not yet Synced. Update ObjectsManager.startNewSync() so it preserves
any existing waiter and only creates a new CompletableDeferred when none exists,
allowing endSync() to resume the original suspended caller instead of leaving it
hanging.
- Around line 152-155: The early return in ObjectsManager.applySync() skips
stale-object cleanup when syncObjectsPool is empty, so previous objects can
remain in the pool. Update applySync() to always run deleteExtraObjectIds() even
for empty syncs, and only guard the later sync-processing logic behind the empty
check. Use the applySync() and deleteExtraObjectIds() symbols to keep the
cleanup path reachable regardless of syncObjectsPool contents.
In `@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsPool.kt`:
- Around line 136-143: The GC cleanup in ObjectsPool.onGCInterval can remove
ROOT_OBJECT_ID even though other cleanup paths preserve it. Update the removeIf
logic so the root entry is never eligible for removal, and keep only non-root
objects subject to isEligibleForGc(gcGracePeriod) while still calling
obj.onGCInterval(gcGracePeriod) for retained entries. Make the fix in
ObjectsPool and ensure getRootAsync can continue to rely on the root LiveMap
always being present.
In `@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ServerTime.kt`:
- Around line 17-36: Scope the cached server-time offset to the adapter clock
instead of keeping it in the process-global ServerTime.serverTimeOffset. Update
ServerTime.getCurrentTime to store and read the offset per SystemClock derived
from adapter.clientOptions, or move the cache into the owning
realtime/liveobjects instance so each client lifecycle has its own value. Keep
the mutex-based lazy initialization, but ensure the cached offset cannot be
reused across different clocks/adapters.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.kt`:
- Around line 44-47: The tombstone state in BaseRealtimeLiveObject is published
in the wrong order: tombstone() currently sets the volatile isTombstoned flag
before updating tombstonedAt, which can let isEligibleForGc observe a stale null
timestamp. Update the write order in BaseRealtimeLiveObject.tombstone() so
tombstonedAt is assigned first and isTombstoned is set last, preserving the
volatile visibility guarantee for the GC coroutine. Also verify any other
tombstone-related writes in BaseRealtimeLiveObject use the same
guarded-state-then-flag pattern.
- Around line 79-121: `applyObject()` is throwing on malformed per-object
fields, but the current handling is only at the broader protocol-message level,
so one bad object can stop later items in the same batch. Move the error
isolation to the per-item loop in the protocol message handler that calls
`applyObject()` and catch/report exceptions around each object entry
individually. Keep `applyObject()` and `canApplyOperation()` as the validation
points, but ensure a failure for one `WireObjectMessage` does not prevent
processing the remaining `protocolMessage.state` items.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/InternalLiveCounter.kt`:
- Around line 96-104: In InternalLiveCounter.notifyUpdated, the current cast
from ObjectUpdate to LiveCounterChangeEvent is invalid and will break on real
updates. Replace the cast with creation/emission of the appropriate
LiveCounterChangeEvent based on the received ObjectUpdate, and pass that to
liveCounterManager.notify so subscribers continue receiving counter updates.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterManager.kt`:
- Around line 44-48: The CounterCreate path in LiveCounterManager.applyOperation
is always reporting success even when applyCounterCreate returns a no-op update
for an already-merged create. Update the CounterCreate branch to base its return
value on whether applyCounterCreate produced a real change (for example by
checking the returned update against the no-op case) so
BaseRealtimeLiveObject.applyObject can correctly treat duplicate creates as not
meaningfully applied.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/InternalLiveMap.kt`:
- Around line 63-97: Add the same tombstone short-circuit used by
InternalLiveMap.get() to entries(), keys(), values(), and size() so tombstoned
maps do not expose stale data; check isTombstoned early in each method and
return an empty iterable or zero size before touching data or calling
getResolvedValue()/count. Keep the fix localized in InternalLiveMap and reuse
the existing tombstone behavior pattern already present in get().
- Around line 171-179: The notifyUpdated method in InternalLiveMap is casting
ObjectUpdate to LiveMapChangeEvent, which will fail at runtime. Update this path
so liveMapManager.notify receives the actual change event object already
available in this flow instead of the ObjectUpdate instance; keep the noOp guard
and logging intact, but replace the invalid cast with the correct event payload
used by the live map update pipeline.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapManager.kt`:
- Around line 389-397: The validate(state: WireObjectState) method in
LiveMapManager is validating map semantics even for tombstoned states, which can
fail when state.map is null. Update the semantics check in validate() to skip
tombstoned payloads by gating validateMapSemantics on !state.tombstone or
state.map != null, while keeping the existing objectId and createOp validation
intact.
---
Nitpick comments:
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.kt`:
- Around line 63-69: The tombstoned-state handling in BaseRealtimeLiveObject is
branching on objectType to choose between noOpMapUpdate and noOpCounterUpdate,
which tightly couples the base class to concrete subtypes. Replace that
enum-based selection with an abstract noOpUpdate: ObjectUpdate hook on
BaseRealtimeLiveObject, and override it in InternalLiveMap and
InternalLiveCounter alongside the existing type-specific hooks like
applyObjectState and clearData so tombstoned processing can just return
noOpUpdate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb581655-c962-49c0-b607-c041a53ae951
📒 Files selected for processing (23)
android/proguard.txtlib/src/main/java/io/ably/lib/liveobjects/value/LiveCounter.javalib/src/main/java/io/ably/lib/liveobjects/value/LiveMap.javaliveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectId.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsManager.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsOperationSource.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsPool.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsState.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsSyncTracker.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ServerTime.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/WireObjectMessage.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/DefaultLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/InternalLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterChangeCoordinator.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterManager.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/InternalLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapChangeCoordinator.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapManager.kt
Comment on lines
+10
to
+14
| private val objects = ConcurrentHashMap<String, DefaultRealtimeObject>() | ||
|
|
||
| override fun getInstance(channelName: String): RealtimeObject { | ||
| return objects.getOrPut(channelName) { DefaultRealtimeObject(channelName, adapter) } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the plugin and object lifecycle code referenced in the comment.
git ls-files 'liveobjects/src/main/kotlin/io/ably/lib/liveobjects/*.kt'
echo '---'
sed -n '1,120p' liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt
echo '---'
sed -n '1,220p' liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt
echo '---'
sed -n '1,220p' liveobjects/src/main/kotlin/io/ably/lib/liveobjects/RealtimeObject.kt
echo '---'
rg -n "getOrPut|computeIfAbsent|dispose\(|incomingObjectsHandler|launch\(" liveobjects/src/main/kotlin/io/ably/lib/liveobjectsRepository: ably/ably-java
Length of output: 14405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the remaining lifecycle methods and locate the RealtimeObject interface.
sed -n '220,420p' liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt
echo '---'
fd -a 'RealtimeObject.kt' .
echo '---'
rg -n "interface RealtimeObject|class DefaultRealtimeObject|dispose\(" liveobjects/src/main/kotlin/io/ably/lib/liveobjectsRepository: ably/ably-java
Length of output: 8199
🌐 Web query:
Kotlin stdlib ConcurrentMap getOrPut defaultValue may be invoked even if key is already in the map documentation
💡 Result:
Yes, it is explicitly documented that the defaultValue function for ConcurrentMap.getOrPut in the Kotlin standard library may be invoked even if the key is already present in the map [1][2][3]. The official Kotlin documentation states: "This method guarantees not to put the value into the map if the key is already there, but the defaultValue function may be invoked even if the key is already in the map" [2][4][3]. This behavior occurs because of the implementation of the function [1][3]: public inline fun <K, V> ConcurrentMap<K, V>.getOrPut(key: K, defaultValue: -> V): V { // Do not use computeIfAbsent on JVM8 as it would change locking behavior return this.get(key)?: defaultValue.let { default -> this.putIfAbsent(key, default)?: default } } As shown above, the function first attempts to retrieve the value using get(key) [1][3]. If the key is missing or mapped to null, it proceeds to invoke the defaultValue function before attempting to put the result into the map using putIfAbsent [1][3]. Because this check and the subsequent invocation are not performed within a single atomic operation, the defaultValue function may be called unnecessarily if another thread inserts the key into the map after the initial get(key) call but before the defaultValue function is executed [5].
Citations:
- 1: https://github.com/JetBrains/kotlin/blob/master/libraries/stdlib/jvm/src/kotlin/collections/MapsJVM.kt
- 2: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/get-or-put.html
- 3: https://github.com/JetBrains/kotlin/blob/2.3.0/libraries/stdlib/jvm/src/kotlin/collections/MapsJVM.kt
- 4: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.collections/get-or-put.html
- 5: https://stackoverflow.com/questions/76514680/should-we-use-computeifabsent-instead-of-getorput
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect build configuration for Java/Kotlin target compatibility.
git ls-files '*build.gradle*' '*settings.gradle*' 'gradle.properties' 'gradle/libs.versions.toml' 'pom.xml'
echo '---'
rg -n "sourceCompatibility|targetCompatibility|jvmTarget|kotlinOptions|toolchain|javaVersion|JavaVersion|kotlin.*jvm|org.jetbrains.kotlin.jvm|jvmToolchain" \
-g '!*build*' -g '!**/build/**' \
.Repository: ably/ably-java
Length of output: 1287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' liveobjects/build.gradle.kts
echo '---'
sed -n '1,220p' build.gradle.kts
echo '---'
sed -n '1,220p' gradle.propertiesRepository: ably/ably-java
Length of output: 3505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "JavaVersion\.VERSION_|sourceCompatibility|targetCompatibility|jvmTarget|jvmToolchain|toolchain|VERSION_1_8|VERSION_11|VERSION_17" \
liveobjects build.gradle.kts gradle.properties gradle/libs.versions.toml java build.gradle.ktsRepository: ably/ably-java
Length of output: 302
Avoid getOrPut here; concurrent first-time calls can start and drop extra DefaultRealtimeObject instances.
ConcurrentMap.getOrPut may invoke the factory even when another thread wins the race, and DefaultRealtimeObject starts a long-lived coroutine in init. That leaves losing instances undisposed. Use computeIfAbsent so only one object is constructed per channel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt`
around lines 10 - 14, The getInstance implementation in DefaultLiveObjectsPlugin
uses ConcurrentHashMap.getOrPut, which can create extra DefaultRealtimeObject
instances during concurrent first access and leave losing instances undisposed.
Replace the getOrPut usage with ConcurrentMap.computeIfAbsent on objects so only
one DefaultRealtimeObject is constructed per channel, and keep the caching
behavior inside getInstance unchanged.
Collaborator
Author
There was a problem hiding this comment.
We will never have concurrent first time call on getInstance. Whenever channel is first created, we call getInstance method on it for given channelName, you should check the source code
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "getInstance\(" liveobjects lib -g '*.kt' -g '*.java' -C3Length of output: 10534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "new ChannelBase|class ChannelBase|synchronized|ConcurrentHashMap|computeIfAbsent" lib/src/main/java/io/ably/lib/realtime/*.java | grep -i channelLength of output: 3407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "class Channels|get\(String|ConcurrentHashMap|synchronized|computeIfAbsent" lib/src/main/java/io/ably/lib/realtime/Channels.javaLength of output: 565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i "Channels" lib/src/main/java -e java
echo '---'
rg -n "class Channels" -g '*.java' libLength of output: 556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "extends ChannelBase|new .*Channel\(|class .*Channels\b" -g '*.java' lib/src/main/java | head -50Length of output: 914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i "InternalMap" -e java
echo '---'
sed -n '1,80p' $(fd -i "InternalMap" -e java)
echo '---'
sed -n '180,230p' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.javaLength of output: 4161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat gradle.properties 2>/dev/null | head -50
echo '---'
rg -n "JavaVersion|sourceCompatibility|targetCompatibility|jvmTarget|jvmToolchain" -g '*.gradle*' .Length of output: 2059
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat liveobjects/build.gradle.kts
echo '---'
rg -n "minSdk|minSdkVersion|compileSdk" android/build.gradle.ktsLength of output: 1605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "desugar|coreLibraryDesugaring|isCoreLibraryDesugaring" android/build.gradle.ktsLength of output: 424
@sacOO7 Thanks for pushing back — I checked the call path more closely and wanted to share what I found, since it's the opposite of what I assumed initially but still relevant here.
getInstance(channelName) is only called from Channel's constructor (ChannelBase.java, lines 1687-1693), and that constructor is invoked by AblyRealtime.InternalChannels.get(channelName, channelOptions):
// AblyRealtime.java
final Channel newChannel = new Channel(AblyRealtime.this, channelName, channelOptions, liveObjectsPlugin);
final Channel existingChannel = map.putIfAbsent(channelName, newChannel);Notice this constructs newChannel (which triggers getInstance(channelName) in its constructor) before checking whether an entry already exists, and it explicitly avoids computeIfAbsent:
// We're not using computeIfAbsent because that requires Java 1.8.
// Hence there's the slight inefficiency of creating newChannel when it may not be
// needed because there is an existingChannel.
So if two threads call channels.get("foo") concurrently for a channel that doesn't exist yet (which is a supported usage pattern — nothing in Channels.get() synchronizes on the channel name), both threads will construct a Channel object, and each Channel constructor will call getInstance("foo") before putIfAbsent resolves which one wins. That's precisely the race getOrPut is exposed to in DefaultLiveObjectsPlugin, and it now means an extra DefaultRealtimeObject — with its coroutine started in init — could be created and dropped.
That said, I want to flag one nuance for the fix itself: liveobjects module's Kotlin builds as a plain JVM library, but it's consumed by the android module which has minSdk = 19 without core library desugoring configured. ConcurrentHashMap.computeIfAbsent requires API 24+, which is likely why the existing AblyRealtime/AblyBase code avoided it in the first place. Worth double-checking whether computeIfAbsent is safe to use here for that reason before swapping it in — if it's not viable, a putIfAbsent-based approach (construct-then-putIfAbsent-then-dispose-the-loser, mirroring the existing Channels.get() pattern) would sidestep both the race and the API-level concern.
Let me know if you'd like me to open a follow-up issue to track this, or if you'd prefer to handle it as-is given the current usage patterns.
Collaborator
Author
There was a problem hiding this comment.
So, computeIfAbsent is best one recommended here? I will use the same in next PR then 👍
…alue types
Fills in the Kotlin implementation behind the LiveObjects public interfaces
(lib/.../liveobjects), replacing the TODO stubs in DefaultRealtimeObject with
a working objects sync engine and the LiveCounter / LiveMap value types.
Sync engine (io.ably.lib.liveobjects):
- ObjectsManager - processes OBJECT / OBJECT_SYNC messages, buffers ops
during sync, creates zero-value objects (RTO5-RTO7).
- ObjectsPool - object store keyed by objectId with tombstone GC
(grace period from connectionDetails, RTO3).
- ObjectsSyncTracker/ObjectsState - track sync sequence + state transitions
and emit SYNCING/SYNCED events (RTO2, RTO5).
- ObjectId - SHA-256 based object identity from initialValue+nonce
(RTO6b1, RTO14).
- ServerTime - server-time offset with double-checked locking (RTO16).
- ObjectsOperationSource - LOCAL vs CHANNEL operation origin (RTO22).
- DefaultRealtimeObject - wires the above into the RealtimeObject entry point.
Value types (io.ably.lib.liveobjects.value):
- LiveCounter and LiveMap are each split into an Internal* value holder, a
*Manager (applies state/operations), and a *ChangeCoordinator (subscriber
notification), over a shared BaseRealtimeLiveObject. DefaultLiveCounter /
DefaultLiveMap move into livecounter/ and livemap/ subpackages.
Reflection / R8 consistency (see prior review):
- LiveCounter.java / LiveMap.java IMPLEMENTATION_CLASS constants updated to the
new value.livecounter / value.livemap package locations (previously pointed
at the old value.* path and would have thrown ClassNotFoundException).
- android/proguard.txt keep-rules migrated to io.ably.lib.liveobjects.*, aligned
with the actual class locations and reflective constructor signatures, plus
added keeps for WireObjectDataJsonSerializer and the Wire* message model that
Gson (de)serializes by field-name reflection on the JSON transport path.
sacOO7
force-pushed
the
feature/liveobjects-kotlin-implementation
branch
from
4e0d9df to
8006b9e
Compare
…PI preconditions to callers Spec-point migration (path-based API): - Replace deprecated/renamed spec-point references throughout the liveobjects impl and the ObjectsMapEntry interface with their current equivalents: access/write preconditions -> RTO25/RTO26, map/counter create steps -> RTLMV4*/RTLCV4*/RTO14, "v6.0.0" renumbers (e.g. RTLM15d2->RTLM15d6, RTLM7c->RTLM7g, RTLC7d2->RTLC7d5), RTO5f2c->RTO5f3, RTO6b4->RTO6b2/RTO6b3, RTTS9d2->RTTS9d, and ME1->OME1. Move access/write API preconditions to callers (RTO25/RTO26): - Remove throwIfInvalidAccessApiConfiguration / throwIfInvalidWriteApiConfiguration from InternalLiveMap (get/entries/size/subscribe/set/remove) and InternalLiveCounter (value/subscribe/increment). The spec now checks these preconditions in the callers; PathObject and Instance already enforce them. - Make the InternalLiveMap/InternalLiveCounter value methods `internal` so they cannot be reached bypassing those wrappers; rename the ctor param realtimeObjects -> realtimeObject. Correctness: - calculateUpdateFromDataDiff returns the no-op update (ObjectUpdate(null)) for an empty map diff / zero counter delta, so notifyUpdated() short-circuits and no spurious change event is emitted when clearing an empty root object.
sacOO7
changed the title
LiveObjects implementation to new path-based API (Kotlin)LiveObjects implementation to new path-based API (Kotlin)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/InternalLiveMap.kt (1)
60-91: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing tombstone guard on
entries()/keys()/values()/size().
get()short-circuits withif (isTombstoned) return null(line 51), butentries(),keys(),values(), andsize()do not. If the map is tombstoned whiledataisn't yet empty — e.g., during concurrentclearData()or due toConcurrentHashMapiterator weak consistency — these methods can surface stale entries or a non-zero size for a deleted object. Adding the guard toentries()coverskeys()andvalues()since they delegate to it.🐛 Proposed fix
internal fun entries(): Iterable<Map.Entry<String, LiveMapValue>> { + if (isTombstoned) { + return emptyList() + } return sequence<Map.Entry<String, LiveMapValue>> { for ((key, entry) in data.entries) { val value = entry.getResolvedValue(objectsPool) // RTLM11d, RTLM11d2 value?.let { yield(AbstractMap.SimpleImmutableEntry(key, it)) } } }.asIterable() }internal fun size(): Long { + if (isTombstoned) { + return 0L + } return data.values.count { !it.isEntryOrRefTombstoned(objectsPool) }.toLong() // RTLM10d }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/InternalLiveMap.kt` around lines 60 - 91, Add the same tombstone early-return used by get() to InternalLiveMap.entries() so it yields nothing when the map is tombstoned; since keys() and values() delegate to entries(), they will inherit the fix automatically. Also guard InternalLiveMap.size() with isTombstoned so it reports zero for deleted maps even if data still contains stale entries during concurrent clearData() or weakly consistent iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt`:
- Around line 217-224: `publishAndApply()` in `DefaultRealtimeObject` is
treating deferred local application as success, which causes `createMapAsync()`
and `createCounterAsync()` to fail after a publish that already went out. Update
the create flow around `publishAndApply()`, `createMapAsync()`, and
`createCounterAsync()` so that when `siteCode` or `publishResult.serials` are
unavailable you return an explicit deferred-apply state and wait for the server
echo before resolving, instead of letting the create path continue as if the
object were locally applied; alternatively, fail before publishing if local
ACK-apply requirements cannot be met.
- Around line 289-292: The catch block in DefaultRealtimeObject’s message
handling is swallowing coroutine cancellation by catching Exception, so update
the exception handling to explicitly rethrow CancellationException before the
existing Log.e skip-and-continue behavior. Keep the current logging for real
failures in the same message-processing path, but ensure cancellation from the
collector propagates immediately instead of being treated as a recoverable
message error.
---
Duplicate comments:
In
`@liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/InternalLiveMap.kt`:
- Around line 60-91: Add the same tombstone early-return used by get() to
InternalLiveMap.entries() so it yields nothing when the map is tombstoned; since
keys() and values() delegate to entries(), they will inherit the fix
automatically. Also guard InternalLiveMap.size() with isTombstoned so it reports
zero for deleted maps even if data still contains stale entries during
concurrent clearData() or weakly consistent iteration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d49ae11-9bec-4412-ae6b-92ee5eccf1c5
📒 Files selected for processing (25)
android/proguard.txtlib/src/main/java/io/ably/lib/liveobjects/message/ObjectsMapEntry.javalib/src/main/java/io/ably/lib/liveobjects/value/LiveCounter.javalib/src/main/java/io/ably/lib/liveobjects/value/LiveMap.javaliveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/Errors.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectId.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsManager.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsOperationSource.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsPool.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsState.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsSyncTracker.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/ServerTime.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/WireObjectMessage.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/DefaultLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/InternalLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterChangeCoordinator.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterManager.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/InternalLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapChangeCoordinator.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapManager.kt
✅ Files skipped from review due to trivial changes (3)
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsOperationSource.kt
- lib/src/main/java/io/ably/lib/liveobjects/message/ObjectsMapEntry.java
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/Errors.kt
🚧 Files skipped from review as they are similar to previous changes (19)
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/DefaultLiveCounter.kt
- lib/src/main/java/io/ably/lib/liveobjects/value/LiveCounter.java
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/message/WireObjectMessage.kt
- lib/src/main/java/io/ably/lib/liveobjects/value/LiveMap.java
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/DefaultLiveMap.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsSyncTracker.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ServerTime.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterManager.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapEntry.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsState.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livecounter/LiveCounterChangeCoordinator.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapChangeCoordinator.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/livemap/LiveMapManager.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectId.kt
- android/proguard.txt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsPool.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/ObjectsManager.kt
- liveobjects/src/main/kotlin/io/ably/lib/liveobjects/value/BaseRealtimeLiveObject.kt
Base automatically changed from
feature/uts-liveobjects-unit-tests
to
feature/path-based-liveobjects-implementation
July 9, 2026 11:38
Collaborator
Author
|
Current PR mainly focuses on migrating old implmentation to new one, so we don't need to review the existing codebase changes. So, resolving all of unnecessary review comments : ) |
sacOO7
merged commit 2ea697c
into
feature/path-based-liveobjects-implementation
14 of 15 checks passed
This was referenced Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
The LiveObjects public interfaces (
lib/src/main/java/io/ably/lib/liveobjects) were already in place, but theliveobjectsKotlin module behind them was mostly TODO stubs —DefaultRealtimeObjectreturnedTODO("Not yet implemented")for its core entry points, and there was no engine to process object sync messages or maintain object state.This PR fills in that implementation: a working objects sync engine plus the
LiveCounter/LiveMapvalue types, following theRTO*/RTLC*/RTLM*spec points (referenced inline in the code as@spec/Spec:tags).What's included
Sync engine —
io.ably.lib.liveobjectsObjectsManagerOBJECT/OBJECT_SYNCmessages, buffers operations during a sync sequence, creates zero-value objectsObjectsPoolobjectId, with tombstone GC (grace period sourced fromconnectionDetails, default fallback 24h)ObjectsSyncTrackerObjectsStateSYNCING/SYNCEDeventsObjectIdinitialValue+nonceServerTimeObjectsOperationSourceLOCAL(on ACK) vsCHANNELoperation originDefaultRealtimeObjectRealtimeObjectentry point (was stubs)Value types —
io.ably.lib.liveobjects.valueBoth
LiveCounterandLiveMapfollow the same three-part split over a sharedBaseRealtimeLiveObject:Internal*— the value holder / state.*Manager— applies sync state and operations to the value.*ChangeCoordinator— notifies subscribers of updates.The concrete
DefaultLiveCounter/DefaultLiveMap(reflectively instantiated by the publicLiveCounter#create/LiveMap#createfactories) move intovalue/livecounter/andvalue/livemap/subpackages alongside their managers.Reflection / R8 consistency
A companion review of the runtime-reflection surface (
Class.forName, Gson@JsonAdapter) surfaced two issues, fixed here:LiveCounter.java/LiveMap.javaIMPLEMENTATION_CLASSconstants still pointed at the pre-refactorvalue.DefaultLive*path —Class.forNamewould have thrownClassNotFoundExceptionon every build. Updated to the newvalue.livecounter.*/value.livemap.*locations.android/proguard.txt: keep-rules migrated from the oldio.ably.lib.objects.*package toio.ably.lib.liveobjects.*, aligned with the real class locations and reflective constructor signatures. Added keeps forWireObjectDataJsonSerializerand the wholeWire*message model, which Gson (de)serializes by field-name reflection on the JSON transport path (the MessagePack path uses explicit keys and is unaffected). Mirrors the existingio.ably.lib.types.**rule.Verification
./gradlew checkWithCodenarc checkstyleMain checkstyleTest— passing../gradlew :liveobjects:compileKotlin— passing.consumerProguardFilesrules are only exercised by a downstream minified build, so they are not validated by this repo's CI; an end-to-end check would need a minified consumer round-tripping a LiveObjects JSON message.Notes for reviewers
feature/uts-liveobjects-unit-tests(notmain) — this stacks under the UTS unit-test work.settings.gradle.ktstweak are intentionally not part of this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary of changes
New Features
Bug Fixes
Documentation