Add side effects of more connection states for DETACHING channels
#318
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I want to copy some of the #attach rules for #detach, so let's clarify a few things first.
On a first read of the preceding spec points, I found it a bit hard to reason about under which conditions we actually end up putting a DETACH on the wire. I think a bit of commentary here is helpful.
The ATTACHING part was already redundant to RTL3d. I'm moving the DETACHING part there for consistency (also, the rest of RTN19 is specifically about ACKs so it doesn't really fit there) and to provide a natural place to add upcoming spec points about how a DETACHING channel should respond to other connection states.
Since this behaviour relates to what to do when you've sent a DETACH but haven't yet received a DETACHED, I want to make sure I understand it before moving on to considering the same thing in the context of some not-currently-handled connection state changes. The current language of "previous state" is a bit vague to me — I think that you can interpret it as meaning either the start-of-RTL5d ATTACHED or the triggered-by-RTL5d DETACHING. ably-js [1] and ably-cocoa [2] transition back to ATTACHED so that's the behaviour that I've specified here, but I don't fully understand the logic — it seems a bit misleading that you'd describe a channel as ATTACHED when in fact the detach request may have been honoured (in which case Realtime is not going to send you any further messages on that channel). (TODO: Would be good to get some insight on this one from someone involved in this decision, so that I can add an explanatory comment. I guess the thinking might be that the only way you're not going to receive the DETACHED is because of a connection issue, in which case you're eventually going to try reattaching and your ATTACHED state will not be a lie any more. And if you receive the DETACHED late, you'll reattach per RTL13) [1] https://github.com/ably/ably-js/blob/b5fbb90c805d560813eec0637f140c39106dd105/src/common/lib/client/realtimechannel.ts#L842-L846 [2] https://github.com/ably/ably-cocoa/blob/ebe0e752e8d35ac9cceb0b4f2508675a4d519e15/Source/ARTRealtimeChannel.m#L1025
We add handling for the following connection states: FAILED: We're never going to receive the DETACHED that we're waiting for, but we don't want to remain in DETACHING forever. So, let's put the channel into FAILED and fail the #detach request. CLOSED: We're never going to receive the DETACHED that we're waiting for, but we don't want to remain in DETACHING forever. I don't think we should consider this as a failure of the #detach request, because by becoming CLOSED we've decided that we consider the server-side connection state to have been torn down, and so it seems like we should also consider the server-side attachment state to have also been torn down. So, succeed the #detach request. SUSPENDED: By becoming SUSPENDED, we've decided that there is no longer any server-side connection state, and so I think we can conclude there is also no longer any server-side attachment state and thus consider the request to tear it down (that is, the #detach request) to have succeeded. It's also consistent with what we do in RTL5j when #detach is called on a SUSPENDED channel. Now that there are more ways in which a #detach request can resolve, we bring #detach in line with #attach and make its callback be driven by the channel state. Note: we noticed the need for these spec points because we noticed ([1], [2]) that, in the absence of guidance from the spec, in ably-js a DETACHING channel actually ends up becoming SUSPENDED when the connection becomes SUSPENDED, which means it ends up reattaching when the connection becomes CONNECTED again. This is not what we want — a DETACHING channel should never autonomously reattach. [1] https://ably-real-time.slack.com/archives/C8SPU4589/p1732273028523529?thread_ts=1732191641.458019&cid=C8SPU4589 [2] https://github.com/ably/ably-js/blob/18a255948c38d1e60715c8f5d6173369b57cb8d6/src/common/lib/client/baserealtime.ts#L178
lawrence-forooghian
force-pushed
the
242-clarify-connection-state-effects-for-DETACHING-channels
branch
from
24b64a3 to
34e53e5
Compare
lawrence-forooghian
added a commit
to ably/ably-js
that referenced
this pull request
May 22, 2025
Implements the changes of 34e53e5 from [1]. [1] ably/specification#318
|
Will return to this to also think about whether the connection error should be propagated to the channel (and making this clear for the other RTL4* spec points that don't make it explicit). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Specifies what should happen to a
DETACHINGchannel when the connection becomesFAILED,CLOSED, orSUSPENDED.Please take a look at the commit messages, since there are some things where I'd appreciate either confirmation that my reasoning is correct, or better understanding of why things are the way they currently are.
Resolves #242.