Implicit attach on channel.objects.getRoot() call#472
Implicit attach on channel.objects.getRoot() call#472lawrence-forooghian wants to merge 3 commits into
channel.objects.getRoot() call#472Conversation
As already implemented in JS in 9bde15e. Co-Authored-By: Lawrence Forooghian <lawrence.forooghian@ably.com>
Review against ably-js implementationI've cross-checked this PR against ably-js 🟠
|
| State | Behaviour |
|---|---|
ATTACHED, SUSPENDED |
Pass through; do not re-attach. |
INITIALIZED, DETACHED, DETACHING, ATTACHING |
Implicit attach and wait. |
FAILED |
Throw ErrorInfo 400/90001. |
🟠 Behaviour during pending implicit-attach if the channel transitions to FAILED
RTO1e says "implicitly attach the RealtimeChannel and wait for the attach to complete" but doesn't define what happens if the attach attempt itself fails, or if the channel transitions to FAILED during the wait. The impl simply propagates the attach() rejection (realtimechannel.ts:1144 → standard attach() failure semantics).
Worth being explicit:
- (RTO1e1) If the implicit attach fails (e.g. the channel transitions to `FAILED` during the wait), the returned promise MUST reject with an `ErrorInfo` 400/90001.
🟡 RFC 2119 keyword precision
(RTO1f) says "the library should throw" — should this be MUST? The pre-existing (RTO1b) used the same "should" wording, but per CLAUDE.md's writing principles ("Use RFC 2119 keywords precisely"), throwing for a FAILED channel is a hard requirement. Suggest "MUST throw".
⚪ Minor: cross-reference to RTL27 lifecycle
For readers landing on this clause without context, a one-line cross-reference to channel-state transitions (RTL2) and the implicit-attach pattern used elsewhere (e.g. RTP11b for presence.get(), mentioned in the PR description) would help with consistency framing.
These observations correspond to issue B-3 in LIVEOBJECTS_SPEC_REVIEW.md (cross-branch review) and Task B-6 in LIVEOBJECTS_UPDATE_SPEC_ACTIONABLE_PLAN.md.
07e89c9 to
969aade
Compare
There was a problem hiding this comment.
LGTM.
Addressed changes as per review comment and now behaviour is exactly similar to JS code.
969aade to
b5d9a12
Compare
b5d9a12 to
2c9b91b
Compare
| - `(RTO1a)` Requires the `OBJECT_SUBSCRIBE` channel mode to be granted per [RTO2](#RTO2) | ||
| - `(RTO1b)` If the channel is in the `DETACHED` or `FAILED` state, the library should throw an `ErrorInfo` error with `statusCode` 400 and `code` 90001 | ||
| - `(RTO1b)` This clause has been replaced by [RTO1e](#RTO1e). | ||
| - `(RTO1e)` Performs the *ensure-attached* procedure to make sure the underlying `RealtimeChannel` is in the `ATTACHED` state before proceeding. |
There was a problem hiding this comment.
As per ably-js#ensureAttached method comment description,
* This method is intended for use by features like Presence or Objects that need to
* implicitly attach the channel when an operation is called (e.g., `presence.get()` per RTP11b,
* or `objects.get()`). This guarantees that the corresponding sync sequence will start and
* that the operation will resolve for callers even if they did not explicitly attach beforehand.
*/
Currently, I have limited scope to objects spec, otherwise RTO1e is supposed to be shared by both presence.get and objects.get. Currently, the spec RTP11b is incomplete for presence.get : (
So, maybe we can extract RTO1e and share it with both presence.get and objects.get
wdyt @lawrence-forooghian @ttypic
| - `(RTO1a)` Requires the `OBJECT_SUBSCRIBE` channel mode to be granted per [RTO2](#RTO2) | ||
| - `(RTO1b)` If the channel is in the `DETACHED` or `FAILED` state, the library should throw an `ErrorInfo` error with `statusCode` 400 and `code` 90001 | ||
| - `(RTO1b)` This clause has been replaced by [RTO1e](#RTO1e). | ||
| - `(RTO1e)` Performs the *ensure-attached* procedure to make sure the underlying `RealtimeChannel` is in the `ATTACHED` state before proceeding. |
There was a problem hiding this comment.
to make sure the underlying
RealtimeChannelis in theATTACHEDstate before proceeding
This isn't quite true since the procedure indicates success when SUSPENDED, too. Ditto calling it "ensure attached" (I know the name is taken from ably-js but I think it's wrong there too)
And yes, to your other question I think we should refactor the spec to share the same logic between presence and objects (but this shared procedure should go in the main spec, not objects spec)
| - `(RTO1b)` If the channel is in the `DETACHED` or `FAILED` state, the library should throw an `ErrorInfo` error with `statusCode` 400 and `code` 90001 | ||
| - `(RTO1b)` This clause has been replaced by [RTO1e](#RTO1e). | ||
| - `(RTO1e)` Performs the *ensure-attached* procedure to make sure the underlying `RealtimeChannel` is in the `ATTACHED` state before proceeding. | ||
| - `(RTO1e1)` If the channel is in the `INITIALIZED`, `DETACHED`, `DETACHING`, or `ATTACHING` state, implicitly attach the `RealtimeChannel` and wait for the attach to complete |
There was a problem hiding this comment.
Can this be more explicit i.e. literally just defer to calling RTL4 RealtimeChannel#attach, and just state that we fail if it fails (and then we don't need the "for example" in RTO1e1a)
There was a problem hiding this comment.
Actually, when performing callback-style operations in some languages ( like java channel#attach), it's easy to miss out on error callback where it can be silently omitted, so we need explicit spec point to make it clear.
b0638be to
0d680ca
Compare
…ve-channel` procedure to avoid spec duplication
0d680ca to
582f2e2
Compare
As already implemented in JS in ably/ably-js@9bde15.
Split out of #427 since it's a standalone behaviour.