[ECO-5056] use server-provided GC grace period for garbage collection#1158
[ECO-5056] use server-provided GC grace period for garbage collection#1158
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdapter's connection accessor changed from ConnectionManager to Connection; ConnectionDetails and ConnectionManager now carry objectsGCGracePeriod; LiveObjects runtime GC uses a server-provided grace period (parameterized through GC APIs); MAP_CLEAR operation and clearTimeserial were added to LiveMap; tests and helpers updated accordingly. Changes
sequenceDiagram
autonumber
actor App
participant Pool as ObjectsPool
participant Adapter as ObjectsAdapter
participant Conn as Connection
participant CM as ConnectionManager
participant Srv as Server
rect rgba(230,240,255,0.5)
App->>Pool: init()
Pool->>Adapter: onGCGracePeriodUpdated(cb)
Adapter->>Conn: getConnection()
Conn->>CM: access connectionManager
alt CM.objectsGCGracePeriod present
CM-->>Adapter: objectsGCGracePeriod
Adapter-->>Pool: cb(value)
else
Conn-->>Adapter: register connected listener
end
end
rect rgba(235,255,235,0.5)
Srv-->>CM: ConnectionDetails(objectsGCGracePeriod)
CM->>CM: objectsGCGracePeriod = ...
Conn-->>Adapter: emit connected
Adapter-->>Pool: cb(CM.objectsGCGracePeriod or null)
Pool->>Pool: gcGracePeriod updated or default retained
end
rect rgba(255,245,230,0.5)
loop every interval
Pool->>Pool: for each object
Pool->>Pool: obj.isEligibleForGc(gcGracePeriod)?
alt eligible
Pool->>Pool: remove object
else
Pool->>Pool: obj.onGCInterval(gcGracePeriod)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
sacOO7
force-pushed
the
feature/liveobjects-use-server-gc-period
branch
from
9610b41 to
c8c84e7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
sacOO7
force-pushed
the
feature/liveobjects-use-server-gc-period
branch
from
12ecee1 to
367a751
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
liveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (1)
124-131: Fix publication/visibility race when tombstoning
tombstonedAtis written after the volatileisTombstoned. A GC thread may observeisTombstoned == truebut a stale/nulltombstonedAt. WritetombstonedAtfirst so the subsequent volatile write publishes it; optionally marktombstonedAtvolatile.- @Volatile - internal var isTombstoned = false // Accessed from public API for LiveMap/LiveCounter - - private var tombstonedAt: Long? = null + @Volatile + internal var isTombstoned = false // Accessed from public API for LiveMap/LiveCounter + + @Volatile + private var tombstonedAt: Long? = null @@ - isTombstoned = true - tombstonedAt = serialTimestamp?: System.currentTimeMillis() + tombstonedAt = serialTimestamp ?: System.currentTimeMillis() + isTombstoned = true
🧹 Nitpick comments (13)
lib/src/main/java/io/ably/lib/types/ConnectionDetails.java (1)
77-81: Clarify null semantics and units for objectsGCGracePeriodDocument that null means “server did not provide a value” and that clients must fall back to the default (24h). This reduces ambiguity for callers.
- /** - * The duration in milliseconds used to retain tombstoned objects at client side. - */ - public Long objectsGCGracePeriod; + /** + * The duration, in milliseconds, to retain tombstoned objects on the client. + * Null indicates the server did not provide a value; callers MUST fall back to the default (24 hours). + */ + public Long objectsGCGracePeriod; // may be nullliveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (1)
23-26: Ensure extension property stubbing is robust; prefer deterministic payloads
The tests rely on adapter.connectionManager. Since this is an extension property (Helpers.kt), ensure TestHelpers stubs it via mockkStatic("io.ably.lib.objects.HelpersKt") and an explicit stub for any().connectionManager to avoid brittle resolution.
For determinism/speed, avoid Random when constructing large strings.
- val mockAdapter = getMockObjectsAdapter() + val mockAdapter = getMockObjectsAdapter() mockAdapter.connectionManager.maxMessageSize = Defaults.maxMessageSize // 64 kb assertEquals(65536, mockAdapter.connectionManager.maxMessageSize)In TestHelpers.kt (not shown here), prefer:
mockkStatic("io.ably.lib.objects.HelpersKt") val cm = mockk<ConnectionManager>(relaxed = true) +every { any<ObjectsAdapter>().connectionManager } returns cmAnd make payload generation deterministic:
- clientId = CharArray(60 * 1024) { ('a'..'z').random() }.concatToString() + clientId = CharArray(60 * 1024) { 'a' }.concatToString()- clientId = CharArray(5 * 1024) { ('a'..'z').random() }.concatToString() + clientId = CharArray(5 * 1024) { 'a' }.concatToString()Also applies to: 148-151
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
14-15: Bridge property is fine; watch out for test stubbing of extension propertyThe extension property keeps the adapter API stable while migrating to Connection. Ensure tests stub this extension explicitly (via mockkStatic and any().connectionManager) to avoid brittle behavior.
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
103-104: Add doc and consider naming to make units explicit.objectsGCGracePeriod is nullable and in milliseconds; document it for clarity and future readers. If you prefer stronger clarity, consider objectsGCGracePeriodMs (optional, keep consistent with existing fields).
Apply minimal docs:
- public Long objectsGCGracePeriod = null; + /** Server-provided GC grace period in milliseconds for client-side tombstone retention; null if not provided. */ + public Long objectsGCGracePeriod = null;
1295-1303: Validate server value before publishing to readers.Guard against non-positive values to avoid accidental immediate GC or overflow if a bad value is ever sent.
- objectsGCGracePeriod = connectionDetails.objectsGCGracePeriod; + Long gc = connectionDetails.objectsGCGracePeriod; + if (gc != null && gc <= 0) { + Log.w(TAG, "Ignoring non-positive objectsGCGracePeriod: " + gc); + gc = null; + } + objectsGCGracePeriod = gc;If you want stronger visibility guarantees for readers in other threads (tests/plugins) consider:
- making objectsGCGracePeriod volatile, or
- always reading it in response to the Connected event (your Helpers.kt path likely already provides the happens-before).
Confirm your HelpersTest covers “negative value ignored” and “null → default (24h) fallback”.
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
185-187: Slightly simplify and avoid key destructuring overhead.Operate on values; no need to touch keys. Equivalent behavior on ConcurrentHashMap and a touch leaner.
- override fun onGCInterval(gcGracePeriod: Long) { - data.entries.removeIf { (_, entry) -> entry.isEligibleForGc(gcGracePeriod) } - } + override fun onGCInterval(gcGracePeriod: Long) { + data.values.removeIf { entry -> entry.isEligibleForGc(gcGracePeriod) } + }Ensure LiveMapEntry.isEligibleForGc(gcGracePeriod) also considers referenced object tombstones (for values that reference other LiveObjects) so we don’t leak dereferenced children.
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt (1)
63-66: Simplify boolean and guard against nulls explicitlyMinor readability: avoid the
== truepattern and coalesce null to false.-internal fun LiveMapEntry.isEligibleForGc(gcGracePeriod: Long): Boolean { - val currentTime = System.currentTimeMillis() - return isTombstoned && tombstonedAt?.let { currentTime - it >= gcGracePeriod } == true -} +internal fun LiveMapEntry.isEligibleForGc(gcGracePeriod: Long): Boolean { + val currentTime = System.currentTimeMillis() + return isTombstoned && (tombstonedAt?.let { currentTime - it >= gcGracePeriod } ?: false) +}liveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (2)
149-152: Mirror the boolean simplification used for entriesSame readability nit as in LiveMapEntry: avoid
== true.-internal fun isEligibleForGc(gcGracePeriod: Long): Boolean { - val currentTime = System.currentTimeMillis() - return isTombstoned && tombstonedAt?.let { currentTime - it >= gcGracePeriod } == true -} +internal fun isEligibleForGc(gcGracePeriod: Long): Boolean { + val currentTime = System.currentTimeMillis() + return isTombstoned && (tombstonedAt?.let { currentTime - it >= gcGracePeriod } ?: false) +}
205-225: Enforce documented lower bound (> 2 minutes) where the value is setDocs now state a >2min requirement; ensure the configured value is clamped at assignment (ObjectsPool) so implementors of
onGCInterval(gcGracePeriod)can rely on it.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (3)
66-66: Prefer direct field assignment over reflectivesetPrivateField
ConnectionManager.objectsGCGracePeriodis a Java field; MockK relaxed mocks allow direct assignment. Reflection adds noise and brittleness.-connManager.setPrivateField("objectsGCGracePeriod", 123L) +connManager.objectsGCGracePeriod = 123L-connManager.setPrivateField("objectsGCGracePeriod", 456L) +connManager.objectsGCGracePeriod = 456LAlso applies to: 83-83
62-73: Also assert no listener registered via other connection APIsYou verify
once(connected, …)isn’t used; consider verifying no other connection listener API was touched to avoid false negatives with relaxed mocks.
75-91: Add a test for min-GC clampIf we clamp server values below 2 minutes (see ObjectsPool comment), add a test asserting the callback yields the clamped value when server returns small periods (e.g., 60_000 ms).
liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (1)
136-144: Minor: reduce repeated time readsIf GC scans many objects, consider computing
val now = System.currentTimeMillis()once per cycle and threading it through to checks to avoid per-object syscalls. Optional; current approach is fine if pool size is modest.
📜 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 (14)
lib/src/main/java/io/ably/lib/objects/Adapter.java(2 hunks)lib/src/main/java/io/ably/lib/objects/ObjectsAdapter.java(2 hunks)lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(2 hunks)lib/src/main/java/io/ably/lib/types/ConnectionDetails.java(2 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt(3 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt(3 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt(2 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt(1 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt(1 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt(1 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(7 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(2 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(2 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt(4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-07T07:19:59.979Z
Learnt from: sacOO7
PR: ably/ably-java#1139
File: live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt:86-92
Timestamp: 2025-08-07T07:19:59.979Z
Learning: In DefaultLiveCounter.notifyUpdated method, sacOO7 prefers to keep the unchecked cast `update as LiveCounterUpdate` without type safety checks, as they are confident the type system guarantees the correct type will always be passed.
Applied to files:
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt
📚 Learning: 2025-08-14T10:29:31.544Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt:25-28
Timestamp: 2025-08-14T10:29:31.544Z
Learning: In MockK with relaxed = true, accessing Java fields on mocked objects automatically creates mock instances, so assignments like `mockAdapter.connectionManager.maxMessageSize = value` work correctly without needing to explicitly create and assign the nested mock object.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
📚 Learning: 2025-08-01T09:53:16.730Z
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.730Z
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:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt
📚 Learning: 2025-08-06T09:22:40.964Z
Learnt from: sacOO7
PR: ably/ably-java#1135
File: live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt:100-134
Timestamp: 2025-08-06T09:22:40.964Z
Learning: In DefaultLiveObjects.kt, the object creation pattern using objectsPool.get() followed by withContext(sequentialScope.coroutineContext) { objectsPool.createZeroValueObjectIfNotExists() } is thread-safe because sequentialScope uses limitedParallelism(1) ensuring sequential execution, and createZeroValueObjectIfNotExists() performs an internal get() check before creating, preventing duplicate object creation even when multiple coroutines initially see null from the first get() call.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.ktliveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt
📚 Learning: 2025-08-01T05:54:07.024Z
Learnt from: sacOO7
PR: ably/ably-java#1113
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:640-640
Timestamp: 2025-08-01T05:54:07.024Z
Learning: In the ably-java codebase test files, JUnit's Assert.assertEquals method is used which has the signature assertEquals(String message, Object expected, Object actual) where the message parameter comes first, not last like in Kotlin test's assertEquals method.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
📚 Learning: 2025-08-15T10:25:24.011Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:24-25
Timestamp: 2025-08-15T10:25:24.011Z
Learning: In the ably-java codebase, the ObjectsHelper.java log messages that reference "LiveObjects plugin not found" and "LiveObjects functionality" are also part of the deliberate exceptions to the renaming effort, similar to other "LiveObjectsPlugin" references, due to ongoing internal technical debate about isolating API naming from product naming as discussed in LODR-033.
Applied to files:
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java
📚 Learning: 2025-08-07T07:17:33.340Z
Learnt from: sacOO7
PR: ably/ably-java#1137
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:6-6
Timestamp: 2025-08-07T07:17:33.340Z
Learning: In the ably-java LiveObjects test code, there are extension properties defined in TestHelpers.kt that provide access to private fields of classes for testing purposes. For example, `internal var DefaultLiveMap.LiveMapManager: LiveMapManager` allows tests to access the private `liveMapManager` field. These extension imports (like `import io.ably.lib.objects.unit.LiveMapManager`) should not be removed as they are necessary for test functionality and are not conflicting imports.
Applied to files:
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
📚 Learning: 2025-08-01T05:50:33.039Z
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.039Z
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.
Applied to files:
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt
📚 Learning: 2025-08-01T05:50:33.039Z
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.039Z
Learning: In LiveMapManagerTest.kt, the private field `livemapManager` is used in the `shouldCalculateMapDifferenceCorrectly` test method to test the `calculateUpdateFromDataDiff` functionality, so it should not be removed as unused.
Applied to files:
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt
📚 Learning: 2025-08-01T10:30:27.049Z
Learnt from: sacOO7
PR: ably/ably-java#1130
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt:241-241
Timestamp: 2025-08-01T10:30:27.049Z
Learning: In DefaultLiveMapTest.kt integration tests, operations are performed sequentially one after another, and subscription callback list updates happen one at a time, so thread-safe collections are not needed for collecting updates in test scenarios.
Applied to files:
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt
🧬 Code graph analysis (5)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getMockObjectsAdapter(49-55)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getMockObjectsAdapter(49-55)
lib/src/main/java/io/ably/lib/objects/Adapter.java (2)
lib/src/main/java/io/ably/lib/realtime/Connection.java (1)
Connection(19-170)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt (1)
connection(13-32)
lib/src/main/java/io/ably/lib/objects/ObjectsAdapter.java (1)
lib/src/main/java/io/ably/lib/realtime/Connection.java (1)
Connection(19-170)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (3)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getMockObjectsAdapter(49-55)liveobjects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
setPrivateField(22-26)liveobjects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(37-37)
⏰ 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-rest-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check-realtime-okhttp
- GitHub Check: check
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (29)
🔇 Additional comments (7)
lib/src/main/java/io/ably/lib/types/ConnectionDetails.java (1)
122-124: ConnectionDetails.objectsGCGracePeriod is sent in milliseconds—no change required.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt (1)
22-22: Good move to centralized test fixtureSwitching to getMockObjectsAdapter() simplifies setup and improves consistency across tests.
Also applies to: 47-47, 81-81, 110-110
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
5-5: Import is appropriateConnectionEvent is required by the new one-shot listener.
liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt (1)
112-115: All overrides updated with gcGracePeriod parameterNo stale overrides remain—
BaseRealtimeObjectdeclaresonGCInterval(gcGracePeriod: Long)exactly once, and all implementations (e.g.DefaultLiveMap,DefaultLiveCounter) have been updated accordingly.lib/src/main/java/io/ably/lib/objects/Adapter.java (1)
25-28: No getConnectionManager references found—no downstream breakage
Search across the codebase returned no calls togetConnectionManager(), so replacinggetConnectionManager()withgetConnection()poses no risk. The suggested deprecated shim remains purely optional for transitional support.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
49-55: I've added searches to confirm the helper function definitions, usages of static mocking, and the signature ofgetChannel. Once we have results, we can finalize the review.lib/src/main/java/io/ably/lib/objects/ObjectsAdapter.java (1)
21-28: All call-sites migrated togetConnection(); no legacygetConnectionManager()remains. Only the internalHelpers.ktextension property referencesconnectionManagerfor live-objects plugin and tests.
sacOO7
force-pushed
the
feature/liveobjects-use-server-gc-period
branch
from
367a751 to
c6e3303
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
52-65: Resolved: “already connected + null” edge-case; consider terminal-state fallback and minor Kotlin style fix
- The added “already connected” guard addresses the callback-never-fired issue noted earlier. Thanks for closing that gap.
- Two small follow-ups:
- Style: remove the extra space before the colon in the parameter type.
- Optional resilience: also fire the callback with null when the connection is already in a terminal/non-progressing state (failed/closing/closed/suspended), so callers aren’t left waiting indefinitely if no future connected event will occur.
Apply this diff within this hunk:
-internal fun ObjectsAdapter.retrieveObjectsGCGracePeriod(block : (Long?) -> Unit) { +internal fun ObjectsAdapter.retrieveObjectsGCGracePeriod(block: (Long?) -> Unit) { connectionManager.objectsGCGracePeriod?.let { block(it) return } // If already connected, no further `connected` event is guaranteed; return immediately. - if (connection.state == ConnectionState.connected) { - block(connectionManager.objectsGCGracePeriod) + if (connection.state == ConnectionState.connected) { + block(null) return } + // Also bail out early for terminal/non-progressing states + when (connection.state) { + ConnectionState.failed, + ConnectionState.closing, + ConnectionState.closed, + ConnectionState.suspended -> { + block(null) + return + } + else -> { /* wait for connected */ } + } connection.once(ConnectionEvent.connected) { block(connectionManager.objectsGCGracePeriod) } }
🧹 Nitpick comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (1)
2582-2603: Test correctly asserts plugin-absence error pathAsserts code (40019), status (400), and stable message fragment, matching the intended behavior when LiveObjects is not installed.
Optionally drop the throws clause (not needed with assertThrows):
- public void channel_get_objects_throws_exception() throws AblyException { + public void channel_get_objects_throws_exception() {
📜 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 (6)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java(2 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt(3 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(7 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt(2 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(2 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1158
File: liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt:61-67
Timestamp: 2025-08-28T10:45:52.247Z
Learning: In the Ably LiveObjects system, the server will never send an objectsGCGracePeriod value less than 2 minutes (120000 ms), so client-side validation/clamping of this value is not required.
📚 Learning: 2025-08-28T10:45:52.247Z
Learnt from: sacOO7
PR: ably/ably-java#1158
File: liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt:61-67
Timestamp: 2025-08-28T10:45:52.247Z
Learning: In the Ably LiveObjects system, the server will never send an objectsGCGracePeriod value less than 2 minutes (120000 ms), so client-side validation/clamping of this value is not required.
Applied to files:
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
📚 Learning: 2025-08-15T10:25:24.011Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:24-25
Timestamp: 2025-08-15T10:25:24.011Z
Learning: In the ably-java codebase, the ObjectsHelper.java log messages that reference "LiveObjects plugin not found" and "LiveObjects functionality" are also part of the deliberate exceptions to the renaming effort, similar to other "LiveObjectsPlugin" references, due to ongoing internal technical debate about isolating API naming from product naming as discussed in LODR-033.
Applied to files:
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java
📚 Learning: 2025-06-03T09:15:15.338Z
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.
Applied to files:
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java
⏰ 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). (12)
- GitHub Check: check-liveobjects
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check (21)
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-realtime
- GitHub Check: check (19)
- GitHub Check: check (29)
🔇 Additional comments (4)
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (3)
5-6: Required imports added appropriatelyConnectionEvent/ConnectionState imports are necessary for the new retrieval helper. Looks good.
15-15: Good: centralized access to ConnectionManager via adapterThe extension keeps adapter code concise and consistent with surrounding helpers.
52-65: Default GC grace period is initialized upfront; callback only overrides
ThegcGracePeriodproperty in ObjectsPool is set to the 24 h default at declaration, beforeretrieveObjectsGCGracePeriodis ever called. The callback merely overrides this default if the server provides a value. No caller waits on the callback to establish the default.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (1)
49-49: Import for assertThrows is correctJUnit 4.13+ static import enables method reference usage below. Good.
sacOO7
force-pushed
the
feature/liveobjects-use-server-gc-period
branch
from
c6e3303 to
592fa58
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (2)
44-45: Same pattern as above — consistent setupReuse of getMockObjectsAdapter() and connectionManager is correct.
402-404: Same pattern — correctKeeps tests coherent with the Helpers changes.
🧹 Nitpick comments (4)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (4)
62-73: Prefer direct field assignment over reflective setPrivateField; tighten verify matcherConnectionManager.objectsGCGracePeriod is now a public field; assigning it directly is clearer and less brittle. Also, use a typed matcher for the listener for clarity.
- connManager.setPrivateField("objectsGCGracePeriod", 123L) + connManager.objectsGCGracePeriod = 123L @@ - verify(exactly = 0) { adapter.connection.once(ConnectionEvent.connected, any()) } + verify(exactly = 0) { adapter.connection.once(ConnectionEvent.connected, any<ConnectionStateListener>()) }Note: Per learnings, the server never sends a value < 120_000 ms, so no client-side clamping assertions are needed.
75-92: Solid deferred-path test; use typed matcher for listenerBehavior is correct (defer until connected, then read CM value). Minor nit: prefer typed matcher in verify.
- verify(exactly = 1) { connection.once(ConnectionEvent.connected, any()) } + verify(exactly = 1) { connection.once(ConnectionEvent.connected, any<ConnectionStateListener>()) }
94-108: Covers deferred-with-null; add connected-with-null case and tighten matcherThis verifies null propagation after connect. Consider also covering “already connected, value null” to ensure no listener registration in that path. Tighten matcher type.
- verify(exactly = 1) { adapter.connection.once(ConnectionEvent.connected, any()) } + verify(exactly = 1) { adapter.connection.once(ConnectionEvent.connected, any<ConnectionStateListener>()) }Additional test suggestion (outside this hunk):
@Test fun testRetrieveObjectsGCGracePeriodAlreadyConnectedInvokesImmediatelyWithNull() { val adapter = getMockObjectsAdapter() every { adapter.connection.state } returns io.ably.lib.realtime.ConnectionState.connected var value: Long? = 0L // sentinel adapter.retrieveObjectsGCGracePeriod { v -> value = v } assertNull(value) verify(exactly = 0) { adapter.connection.once(ConnectionEvent.connected, any<ConnectionStateListener>()) } }
291-309: Good negative-path coverage for ensureAttached on non-attached transition; strengthen matcherTest correctly surfaces the reason and error code when attach does not reach attached. Prefer typed matcher for the channel listener.
- verify(exactly = 1) { channel.once(any()) } + verify(exactly = 1) { channel.once(any<ChannelStateListener>()) }
📜 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 (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java(2 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1158
File: liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt:61-67
Timestamp: 2025-08-28T10:45:52.258Z
Learning: In the Ably LiveObjects system, the server will never send an objectsGCGracePeriod value less than 2 minutes (120000 ms), so client-side validation/clamping of this value is not required.
📚 Learning: 2025-08-07T07:17:33.340Z
Learnt from: sacOO7
PR: ably/ably-java#1137
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:6-6
Timestamp: 2025-08-07T07:17:33.340Z
Learning: In the ably-java LiveObjects test code, there are extension properties defined in TestHelpers.kt that provide access to private fields of classes for testing purposes. For example, `internal var DefaultLiveMap.LiveMapManager: LiveMapManager` allows tests to access the private `liveMapManager` field. These extension imports (like `import io.ably.lib.objects.unit.LiveMapManager`) should not be removed as they are necessary for test functionality and are not conflicting imports.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
📚 Learning: 2025-08-14T10:29:31.544Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt:25-28
Timestamp: 2025-08-14T10:29:31.544Z
Learning: In MockK with relaxed = true, accessing Java fields on mocked objects automatically creates mock instances, so assignments like `mockAdapter.connectionManager.maxMessageSize = value` work correctly without needing to explicitly create and assign the nested mock object.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.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). (12)
- GitHub Check: build
- GitHub Check: check (29)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (19)
- GitHub Check: check
- GitHub Check: check (21)
- GitHub Check: check-liveobjects
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check-realtime
- GitHub Check: check-rest
🔇 Additional comments (4)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (4)
8-11: Imports correctly align with new realtime event handlingAdded imports are necessary for the new connection event-based tests; mockk wildcard in tests is acceptable.
22-23: Good switch to getMockObjectsAdapter() and using adapter.connectionManagerThis aligns tests with the new Helpers.kt extensions and avoids stubbing a separate ConnectionManager.
112-113: Consistent setup here as wellKeeps the tests aligned with the new Helpers.kt extensions.
389-390: Valid move to adapter/CM-based setupAccurately reflects the new connection/CM flow.
sacOO7
force-pushed
the
feature/liveobjects-use-server-gc-period
branch
from
592fa58 to
d39929d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (2)
280-290: Also assert we don’t redundantly call attach() while waitingStrengthen the contract for the “attaching → attached” path by ensuring attach isn’t invoked again while waiting.
Apply this diff:
adapter.ensureAttached("ch") verify(exactly = 1) { channel.once(any<ChannelStateListener>()) } + verify(exactly = 0) { channel.attach(any()) }
292-310: Great negative-path coverage; add one more assertionGood check that a non-attached terminal state surfaces an error including the reason. Also verify no attach attempt is made on this failure path.
Apply this diff:
val ex = assertFailsWith<AblyException> { adapter.ensureAttached("ch") } assertEquals(ErrorCode.ChannelStateError.code, ex.errorInfo.code) assertTrue(ex.errorInfo.message.contains("Not attached")) verify(exactly = 1) { channel.once(any<ChannelStateListener>()) } + verify(exactly = 0) { channel.attach(any()) }
📜 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 (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1158
File: liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt:61-67
Timestamp: 2025-08-28T10:45:52.258Z
Learning: In the Ably LiveObjects system, the server will never send an objectsGCGracePeriod value less than 2 minutes (120000 ms), so client-side validation/clamping of this value is not required.
📚 Learning: 2025-08-07T07:17:33.340Z
Learnt from: sacOO7
PR: ably/ably-java#1137
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:6-6
Timestamp: 2025-08-07T07:17:33.340Z
Learning: In the ably-java LiveObjects test code, there are extension properties defined in TestHelpers.kt that provide access to private fields of classes for testing purposes. For example, `internal var DefaultLiveMap.LiveMapManager: LiveMapManager` allows tests to access the private `liveMapManager` field. These extension imports (like `import io.ably.lib.objects.unit.LiveMapManager`) should not be removed as they are necessary for test functionality and are not conflicting imports.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
📚 Learning: 2025-08-14T10:29:31.544Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt:25-28
Timestamp: 2025-08-14T10:29:31.544Z
Learning: In MockK with relaxed = true, accessing Java fields on mocked objects automatically creates mock instances, so assignments like `mockAdapter.connectionManager.maxMessageSize = value` work correctly without needing to explicitly create and assign the nested mock object.
Applied to files:
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
🧬 Code graph analysis (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (3)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getMockObjectsAdapter(49-55)liveobjects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
setPrivateField(22-26)liveobjects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(37-37)
⏰ 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). (12)
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (29)
🔇 Additional comments (4)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (4)
22-23: Queue flag wiring looks correctGood use of ClientOptions.queueMessages to drive the second-arg boolean to ConnectionManager.send; verification matches behavior.
44-45: Error propagation test is solidAsserting AblyException with expected status/code from clientError is correct and guards regression in sendAsync error handling.
391-405: Good coverage of inactive connection rejectionValidates propagation of ConnectionManager.stateErrorInfo when isActive is false. Looks correct and aligns with expected write-path gating.
62-73: Avoid false negatives by stubbing a stable Connection instanceVerifying zero calls on adapter.connection.once risks checking a different mock instance than the one used inside the code under test. Stub adapter.connection to return a fixed mock and verify against that instance.
Apply this diff in this test to stabilize the mock:
fun testRetrieveObjectsGCGracePeriodImmediateInvokesBlock() { val adapter = getMockObjectsAdapter() val connManager = adapter.connectionManager connManager.setPrivateField("objectsGCGracePeriod", 123L) var value: Long? = null - adapter.retrieveObjectsGCGracePeriod { v -> value = v } + val connection = mockk<io.ably.lib.realtime.Connection>(relaxed = true) + every { adapter.connection } returns connection + adapter.retrieveObjectsGCGracePeriod { v -> value = v } assertEquals(123L, value) - verify(exactly = 0) { adapter.connection.once(ConnectionEvent.connected, any<ConnectionStateListener>()) } + verify(exactly = 0) { connection.once(ConnectionEvent.connected, any<ConnectionStateListener>()) } }If you prefer importing the type, add:
import io.ably.lib.realtime.Connection⛔ Skipped due to learnings
Learnt from: sacOO7 PR: ably/ably-java#1148 File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.kt:25-28 Timestamp: 2025-08-14T10:29:31.544Z Learning: In MockK with relaxed = true, accessing Java fields on mocked objects automatically creates mock instances, so assignments like `mockAdapter.connectionManager.maxMessageSize = value` work correctly without needing to explicitly create and assign the nested mock object.
github-actions
bot
temporarily deployed
to
staging/pull/1158/features
github-actions
bot
temporarily deployed
to
staging/pull/1158/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt(3 hunks)liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt(4 hunks)liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🧰 Additional context used
🧬 Code graph analysis (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (3)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getMockObjectsAdapter(49-55)liveobjects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
setPrivateField(22-26)liveobjects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
clientError(37-37)
⏰ 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). (12)
- GitHub Check: check (29)
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (2)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (2)
77-92: Stabilize the connection mock before stubbing.on(…).
getMockObjectsAdapter()returns a relaxed mock, so repeatedadapter.connectioncalls can yield different mock instances. If the implementation fetches a fresh instance, your stub andverify(exactly = 1)assertion won’t observe the call. Please pin the instance by stubbingadapter.connectionto return the locally-declared mock before configuring.on(...).- val connection = adapter.connection + val connection = mockk<io.ably.lib.realtime.Connection>(relaxed = true) + every { adapter.connection } returns connection
95-109: Mirror the connection stub in the null-path test.Same reasoning as the previous test: without stubbing
adapter.connectionto return your local mock, the implementation can calladapter.connectionand receive a different instance, so the listener wiring and verification become brittle. Pin it to the local mock first.- val connection = adapter.connection + val connection = mockk<io.ably.lib.realtime.Connection>(relaxed = true) + every { adapter.connection } returns connection
| gcPeriodSubscription = realtimeObjects.adapter.onGCGracePeriodUpdated { period -> | ||
| period?.let { | ||
| gcGracePeriod = it | ||
| Log.i(tag, "Using objectsGCGracePeriod from server: $gcGracePeriod ms") | ||
| } ?: Log.i(tag, "Server did not provide objectsGCGracePeriod, using default: $gcGracePeriod ms") | ||
| } |
There was a problem hiding this comment.
Reset the grace period to the documented default when the server omits it
When a later onGCGracePeriodUpdated callback receives null, we continue using whatever server value we last saw. That contradicts the documented behavior (“fall back to this default value”) and can leave us running with a stale (possibly much shorter) grace period after reconnects, risking premature tombstone eviction. The log message also becomes misleading in that case. Please restore gcGracePeriod to the default before logging.
gcPeriodSubscription = realtimeObjects.adapter.onGCGracePeriodUpdated { period ->
period?.let {
gcGracePeriod = it
Log.i(tag, "Using objectsGCGracePeriod from server: $gcGracePeriod ms")
- } ?: Log.i(tag, "Server did not provide objectsGCGracePeriod, using default: $gcGracePeriod ms")
+ } ?: run {
+ gcGracePeriod = ObjectsPoolDefaults.GC_GRACE_PERIOD_MS
+ Log.i(tag, "Server did not provide objectsGCGracePeriod, using default: $gcGracePeriod ms")
+ }
}🤖 Prompt for AI Agents
In liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt around lines
62-67, the onGCGracePeriodUpdated null branch must reset gcGracePeriod to the
documented default before logging; change the null path to assign the default
grace period (use the existing default constant or the module's documented
default value) to gcGracePeriod and then log that the default is being used so
we don't retain a stale server value or emit a misleading message.
- Introduced support for the `MAP_CLEAR` operation across LiveMap components. - Updated `MsgpackSerialization` to handle `MAP_CLEAR` serialization/deserialization. - Added `clearTimeserial` tracking in `DefaultLiveMap` to skip outdated operations. - Enhanced `LiveMapManager` to process `MAP_CLEAR` and synchronize state updates. - Comprehensive unit tests added for various `MAP_CLEAR` scenarios.
…yState` - Verified that `applyState` filters out `createOp` entries with null or older-than-clear serials as per RTLM7h. - Added unit test to ensure only entries with newer serials than `clearTimeserial` persist in the LiveMap.
- Add support for server-configured objectsGCGracePeriod from connection details, with fallback to default 24-hour period. Updates ObjectsPool and all LiveObjects - Implementations to use the server-provided value for tombstone cleanup timing.
1. Encouraged use of global mocck for Adapter 2. Updated failing tests accordingly 3. Added unit tests covering all cases for retrieveObjectsGCGracePeriod
…ery time CONNECTED message is received, updated tests for the same
ttypic
force-pushed
the
feature/liveobjects-use-server-gc-period
branch
from
9f8263f to
debbcd0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
54-62:⚠️ Potential issue | 🟠 MajorHandle the already-connected/null path so subscribers are not starved.
If the client is already connected and
objectsGCGracePeriodisnull, this callback won’t fire until a future reconnect. Invokeblock(connectionManager.objectsGCGracePeriod)immediately when already connected.Proposed fix
+import io.ably.lib.realtime.ConnectionState import io.ably.lib.realtime.ConnectionEvent import io.ably.lib.realtime.ConnectionStateListener @@ internal fun ObjectsAdapter.onGCGracePeriodUpdated(block : (Long?) -> Unit) : ObjectsSubscription { - connectionManager.objectsGCGracePeriod?.let { block(it) } + val current = connectionManager.objectsGCGracePeriod + if (current != null || connection.state == ConnectionState.connected) { + block(current) + } // Return new objectsGCGracePeriod whenever connection state changes to connected val listener: (_: ConnectionStateListener.ConnectionStateChange) -> Unit = { block(connectionManager.objectsGCGracePeriod) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt` around lines 54 - 62, The onGCGracePeriodUpdated subscription currently only calls block(...) immediately when objectsGCGracePeriod is non-null, so if the client is already connected but the value is null subscribers are never notified; update ObjectsAdapter.onGCGracePeriodUpdated to detect if the connection is already connected (e.g., check connection.state or connection.isConnected) and in that case call block(connectionManager.objectsGCGracePeriod) unconditionally (instead of the current nullable let), then proceed to register the ConnectionEvent.connected listener as before and return ObjectsSubscription that removes that listener; keep the listener behavior unchanged so future connects still invoke block(connectionManager.objectsGCGracePeriod).liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (1)
62-67:⚠️ Potential issue | 🟠 MajorReset
gcGracePeriodwhen server value is absent to avoid stale retention behavior.The null branch only logs “using default” but keeps the last server value. This breaks the documented fallback behavior and can retain an outdated grace period across reconnects.
Suggested fix
gcPeriodSubscription = realtimeObjects.adapter.onGCGracePeriodUpdated { period -> period?.let { gcGracePeriod = it Log.i(tag, "Using objectsGCGracePeriod from server: $gcGracePeriod ms") - } ?: Log.i(tag, "Server did not provide objectsGCGracePeriod, using default: $gcGracePeriod ms") + } ?: run { + gcGracePeriod = ObjectsPoolDefaults.GC_GRACE_PERIOD_MS + Log.i(tag, "Server did not provide objectsGCGracePeriod, using default: $gcGracePeriod ms") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt` around lines 62 - 67, The null branch of the realtimeObjects.adapter.onGCGracePeriodUpdated callback currently only logs and leaves gcGracePeriod set to the last server value, which causes stale retention; update the null branch so it resets gcGracePeriod to the configured default (e.g. a DEFAULT_GC_GRACE_PERIOD or the original initial value captured at construction), and log that the default is being used. Modify the callback installed in gcPeriodSubscription (realtimeObjects.adapter.onGCGracePeriodUpdated) to assign the default to gcGracePeriod when period is null and include the chosen value in the Log.i(tag, ...) message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt`:
- Around line 54-62: The onGCGracePeriodUpdated subscription currently only
calls block(...) immediately when objectsGCGracePeriod is non-null, so if the
client is already connected but the value is null subscribers are never
notified; update ObjectsAdapter.onGCGracePeriodUpdated to detect if the
connection is already connected (e.g., check connection.state or
connection.isConnected) and in that case call
block(connectionManager.objectsGCGracePeriod) unconditionally (instead of the
current nullable let), then proceed to register the ConnectionEvent.connected
listener as before and return ObjectsSubscription that removes that listener;
keep the listener behavior unchanged so future connects still invoke
block(connectionManager.objectsGCGracePeriod).
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt`:
- Around line 62-67: The null branch of the
realtimeObjects.adapter.onGCGracePeriodUpdated callback currently only logs and
leaves gcGracePeriod set to the last server value, which causes stale retention;
update the null branch so it resets gcGracePeriod to the configured default
(e.g. a DEFAULT_GC_GRACE_PERIOD or the original initial value captured at
construction), and log that the default is being used. Modify the callback
installed in gcPeriodSubscription
(realtimeObjects.adapter.onGCGracePeriodUpdated) to assign the default to
gcGracePeriod when period is null and include the chosen value in the Log.i(tag,
...) message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cee01e8-5aa4-446e-810a-5886fd45df32
📒 Files selected for processing (19)
lib/src/main/java/io/ably/lib/objects/Adapter.javalib/src/main/java/io/ably/lib/objects/ObjectsAdapter.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ConnectionDetails.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.javaliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.ktliveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.ktliveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.ktliveobjects/src/main/kotlin/io/ably/lib/objects/serialization/MsgpackSerialization.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/ObjectMessageSizeTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsPoolTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/src/main/java/io/ably/lib/types/ConnectionDetails.java
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt
- lib/src/main/java/io/ably/lib/transport/ConnectionManager.java
- liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests