Repository navigation
Make releasing a realtime channel that isn't detached an error (RTS4c-e) - #1251
SimonWoolf wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
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 |
There was a problem hiding this comment.
🟡 Changes recommended
Address concurrency and released-channel callback safety, and add FAILED-state coverage.
2 open findings
What changed in this PR
Implements RTS4c–e for next-major realtime channel release semantics, rejecting release of non-terminal channels with error 90011.
Changes:
- Removes implicit detach-on-release behavior.
- Adds UTS-derived release tests.
- Updates teardown, documentation, and obsolete tests.
| File | Summary |
|---|---|
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt |
Updates release documentation. |
liveobjects/src/test/kotlin/io/ably/lib/liveobjects/integration/setup/IntegrationTest.kt |
Avoids releasing attached channels during teardown. |
lib/src/test/kotlin/io/ably/lib/uts/unit/realtime/ChannelsCollectionTest.kt |
Adds RTS4c–e release coverage. |
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java |
Removes obsolete detach-on-release coverage. |
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java |
Removes obsolete released-channel detach handling. |
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java |
Enforces terminal-state release and LiveObjects disposal. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ChannelState state = channel.state; | ||
| if (state != ChannelState.initialized && state != ChannelState.detached && state != ChannelState.failed) { | ||
| // RTS4e | ||
| throw AblyException.fromErrorInfo(new ErrorInfo( | ||
| "Can only release a channel in a state where there is no possibility of further updates from the server being received (initialized, detached, or failed). The current state is " + state, |
There was a problem hiding this comment.
@coderabbitai can you validate and expand on the above
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd '^(ChannelBase|Channel|InternalMap)\.java$'
fd '^(ChannelBase|Channel|InternalMap)\.java$' --exec ast-grep outline {} --items all
rg -n -C3 --type java '\b(markAsReleased|released|setState|requestState|attach|detach)\b' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.javaLength of output: 8697
🏁 Script executed:
#!/bin/bash
rg -n -C5 'volatile|synchronized|isReleased|released|state =|markAsReleased|checkChannelIsNotReleased' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
ast-grep run --lang java --pattern '$RET $NAME($$$ARGS) { $$$BODY }' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | head -n 20
rg -n -C25 'void (attachImpl|detachImpl|markAsReleased|checkChannelIsNotReleased|setState|attachWithTimeout)\(' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
cat -n lib/src/main/java/io/ably/lib/util/InternalMap.java
fd '^LiveObjectsPlugin\.java$' --exec ast-grep outline {} --items all
rg -n -C4 --type java 'void dispose\(String|void dispose\(.*channel|new ErrorInfo\(' lib/src/main/java/io/ably/lib/liveobjectsLength of output: 39767
🏁 Script executed:
#!/bin/bash
sed -n '65,102p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
sed -n '195,233p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
sed -n '210,254p' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
sed -n '1670,1690p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.javaLength of output: 6505
@sacOO7 The core finding is valid. I checked the current code. The proposed plugin cleanup rule needs one additional condition.
Why the state check is unsafe
In lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
stateis notvolatile.setState()writesstateundersynchronized (this)at Line 150.attachWithTimeout()holds the channel monitor and checks the released flag at Line 528.
In AblyRealtime.java, release() reads state without that monitor. It then removes the channel and marks it released.
This sequence is possible:
| Step | Release thread | Attach thread |
|---|---|---|
| 1 | Reads initialized; validation passes. |
|
| 2 | Acquires the channel monitor; the released flag is still false. |
|
| 3 | Changes the state to attaching and sends ATTACH. |
|
| 4 | Removes the channel and marks it released. |
The result violates the release restriction. The channel leaves the collection while an attach is in progress. Subsequent channel messages can reach the “non-existent channel” path.
A stale state read is also possible because the read does not use the writer’s monitor. However, the sequence above proves the race without relying on a stale read.
What the fix must coordinate
Use the channel monitor, not just the collection monitor, for the state check, conditional removal, and markAsReleased().
This gives concurrent attach and release a defined order:
- If attach changes the state first, release sees
attachingand throws error90011. - If release completes first, attach sees the released flag and throws the existing
IllegalStateException.
Making state volatile alone would not fix the check-then-remove race.
Also check the return value of map.remove(channelName, channel). The current code ignores it. If another release already removed that instance, this invocation must not repeat plugin disposal.
Plugin cleanup needs additional coordination
Successful removal is necessary, but it is not sufficient.
dispose(channelName) identifies plugin resources by name. Meanwhile, channels.get(channelName) can create a replacement channel after removal. The replacement constructor calls liveObjectsPlugin.getInstance(name).
Therefore, old-channel cleanup can overlap with replacement-channel initialization. Synchronizing on the old channel does not protect the replacement channel.
The complete fix must either:
- Coordinate removal and plugin disposal with channel creation for the same name, including constructor initialization; or
- Make plugin disposal specific to the old channel’s resource instance.
The lock design must also respect existing lock ordering and callback behavior.
Useful regression cases are attach winning the race, release winning the race, two concurrent releases, and release overlapping with recreation of the same channel name. This validation used source inspection; I did not run tests.
You are interacting with an AI system.
| map.remove(channelName, channel); | ||
| channel.markAsReleased(); | ||
| if (liveObjectsPlugin != null) { | ||
| liveObjectsPlugin.dispose(channelName); |
There was a problem hiding this comment.
@coderabbitai can you validate and expand on the above
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i '^(LiveObjectsPlugin|DefaultRealtimeObject|Channel|InternalMap)\.(java|kt)$'
rg -n -C3 'void dispose\(|fun dispose\(|class DefaultRealtimeObject|class LiveObjectsPlugin' --glob '*.java' --glob '*.kt' .
fd -i '^(LiveObjectsPlugin|DefaultRealtimeObject|Channel|InternalMap)\.(java|kt)$' -x ast-grep outline {}Length of output: 14726
🏁 Script executed:
#!/bin/bash
ast-grep outline liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt --items all
fd '^ChannelBase\.java$' -x ast-grep outline {} --items all
ast-grep outline lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java --items all
nl -ba liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt | sed -n '1,150p'
nl -ba lib/src/main/java/io/ably/lib/liveobjects/LiveObjectsPlugin.java | sed -n '15,68p'
nl -ba lib/src/main/java/io/ably/lib/util/InternalMap.java | sed -n '1,100p'
nl -ba core/src/main/java/io/ably/lib/realtime/Channel.java | sed -n '1,75p'
fd '^ChannelBase\.java$' -x rg -n -C8 'getInstance\(|ChannelBase\(|markAsReleased\(|liveObjectsPlugin|RealtimeObject' {}
nl -ba liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt | sed -n '315,345p'
nl -ba lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java | sed -n '208,270p'Length of output: 26072
@sacOO7 The race is valid. I checked channel construction, the plugin registry, and object disposal. One detail needs correction: the replacement channel normally reuses the old DefaultRealtimeObject, rather than creating a new object.
How the race occurs
Assume channel C1 owns LiveObjects instance O1.
| Step | Thread A: release(name) |
Thread B: get(name) |
|---|---|---|
| 1 | Removes C1 from the channel map. |
|
| 2 | Constructs replacement channel C2. |
|
| 3 | The constructor calls liveObjectsPlugin.getInstance(name). The plugin still contains O1, so C2.object receives O1. |
|
| 4 | Inserts C2 into the channel map and returns it. |
|
| 5 | Calls dispose(name), which removes and disposes O1. |
The code supports this sequence:
AblyRealtime.java, Line 249–252: channel removal and plugin disposal are separate operations.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java, Line 1685: channel construction retrieves the LiveObjects instance by name.liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt, Line 12–14:getInstance()usescomputeIfAbsent, so it returns the existing instance.- The same plugin file, Line 26–28:
dispose(name)removes whichever instance is registered under that name.
Disposal cancels the incoming-object handler, disposes the object pool and manager, and removes path subscriptions. C2.object still references that disposed instance. This breaks the replacement channel’s LiveObjects functionality; it does not establish that ordinary channel messaging is unusable.
Both registries use ConcurrentHashMap. That protects individual map operations, not the lifecycle across both maps.
What a fix must cover
The simplest approach is to serialize get(name, options) and release(name) with the same lifecycle lock.
- In
get(), acquire the lock before constructingChannel. The constructor already accesses the plugin registry. - In
release(), hold the lock through channel removal,markAsReleased(), and plugin disposal. - Prefer checking for an existing channel before constructing a new channel.
Locking only release(), or only putIfAbsent(), does not prevent this race.
There is also a second race: release() ignores the result of map.remove(name, channel). Two concurrent release calls can capture C1. After the first call removes C1, a replacement can be created. The second call can then fail to remove C1 but still dispose the replacement’s LiveObjects instance. Cleanup should therefore depend on successful removal.
An ownership-based design is another option. However, conditional removal by object identity alone is insufficient here, because C2 can already share O1. That design must also prevent a replacement channel from acquiring the releasing channel’s LiveObjects instance.
A deterministic regression test should pause release after channel-map removal, start a concurrent get(), and check that the replacement retains a live, registered LiveObjects instance after release completes. A second test should cover two concurrent release calls.
This validation is based on source inspection. I did not run tests.
You are interacting with an AI system.
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Seems, there are race conditions we need to take care of as a part of the fix
Spec 6.3.0 replaces RTS4a, under which release() detaches the channel and then removes it, with RTS4c-e: release() of a non-existent channel is a no-op, a channel in the INITIALIZED, DETACHED or FAILED state is removed synchronously, and release() of a channel in any other state throws 90011 and does nothing else. Channels#release now declares AblyException. Users must call channel.detach() and wait for it to complete before releasing. Since a released channel can no longer be mid-detach, drop the released-channel special cases from the detach path, the test of the old behaviour, and the liveobjects test teardown's release of channels that may still be attached (close() already disposes of them). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
01fee79 to
d87239a
Compare
|
@sacOO7 done the check/remove/markAsReleased, that now happen under the channel's monitor. The dispose-by-name race was not introduced by this PR and is independent of it, and looks like the fix needs a lifecycle lock spanning get() plus a plugin API change, so not gonna pick it up in this PR, I'll leave it to you as someone more familiar with the sdk. |
Implements RTS4c–e from spec 6.3.0 (ably/specification#557) for the next major version.
Breaking change:
channels.release(name)no longer detaches the channel implicitly.INITIALIZED,DETACHEDorFAILED, it is removed from the collection beforerelease()returns.release()throwsAblyExceptionwith code 90011 and status 400, and the channel is left unchanged.AblyRealtime.Channels.releasenow declaresthrows AblyException.Migration: call
channel.detach(listener)and wait for it to complete before callingchannels.release(name).Also:
90011 is registered in ably/ably-common#367. The deprecation warning for the current major is in #1250. Reference ably-js PRs: ably/ably-pubsub-js#2322 (deprecation) and ably/ably-pubsub-js#2323 (next major).
🤖 Generated with Claude Code