-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chat: room lifecycle specification #200
base: main
Are you sure you want to change the base?
Conversation
AndyTWF
commented
Aug 5, 2024
•
edited
Loading
edited
- Adds chat room lifecycle feature specification.
- Also adds feature specs for presence, messages and room reactions
8c569ea
to
aa7455d
Compare
I’ve started implementing this spec in the Swift SDK. In order to be sure that this spec is sufficiently detailed and unambigious, I’ve made the deliberate decision not to consult the JS implementation. I’ll post feedback here as it comes up 🙂 |
** @(CHA-RS1h)@ The @RELEASING@ status means that the room is being released and the underlying resources are being cleaned up. | ||
** @(CHA-RS1i)@ The @RELEASED@ status means that the room has been cleaned up and the object can no longer be used. | ||
* @(CHA-RS2)@ A room must expose its current status. | ||
** @(CHA-RS2a)@ @[Testable]@ A room must expose its current status, a single value from the list provided above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it a conscious decision to not describe the API of the SDK in terms of signatures / types (e.g. the way that we do in the main spec, which also has the IDL)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IDL we have for the core SDKs is quite java-centric and in that sense may not be the most idiomatic approach for other ecosystems, so I tried to write the spec in a way that describes the intended interactions and behaviour, rather than prescribe an exact implementation.
Is it worth a note somewhere in the spec to treat the JS implementation as a reference implementation and explain the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe having IDL for chat-SDK will be super helpful.
Almost all of the languages these days are encouraging use of types.
So, having language-agnostic types will surely help.
textile/chat-features.textile
Outdated
|
||
* @(CHA-RL1)@ A room must be explicitly @ATTACHED@ to. | ||
** @(CHA-RL1a)@ @[Testable]@ If the room is already in the @ATTACHED@ status, then this operation is no-op. | ||
** @(CHA-RL1b)@ @[Testable]@ If the room is in the @RELEASING@ status, the operation shall be rejected with error code @102102@. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what rejected with error code @102102@
means here. (I know from looking at the JS interface that it reuses the ErrorInfo
type from the core SDK, but it would be good to make that explicit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if the intention is to reuse ErrorInfo
, then all of the places where this spec says to throw such an error should also specify the statusCode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see where the status codes are specified; it seems like you've only added one for CHA-RL1h2. It seems like there should be status codes specified for:
- everything in the "Chat-specific Error Codes" section
- all the places where we specify a numerical error code directly in the spec point (e.g. CHA-RC1c, CHA-RC2b, CHA-RL1b, CHA-RL1c).
** @(CHA-RL1f)@ @[Testable]@ The underlying @contributors@ will have their Realtime Channels attached in sequence. | ||
** @(CHA-RL1g)@ When all @contributors@ Realtime Channels successfully attach, the operation is now complete and the room is considered attached. | ||
*** @(CHA-RL1g1)@ @[Testable]@ The room status shall be transitioned to @ATTACHED@. | ||
*** @(CHA-RL1g2)@ @[Testable]@ Any contributors to room that have a pending discontinuity event against them, must be notified of this fact, using the error that caused the original discontinuity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a spec point that describes what this means? (I might have not got there yet.) If so, it would be good to have a reference to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sorta felt that it didn't fit into a spec point, but I've added some discussion/blurb at the head of the lifecycle section about it!
** @(CHA-RL1g)@ When all @contributors@ Realtime Channels successfully attach, the operation is now complete and the room is considered attached. | ||
*** @(CHA-RL1g1)@ @[Testable]@ The room status shall be transitioned to @ATTACHED@. | ||
*** @(CHA-RL1g2)@ @[Testable]@ Any contributors to room that have a pending discontinuity event against them, must be notified of this fact, using the error that caused the original discontinuity. | ||
*** @(CHA-RL1g3)@ @[Testable]@ Any transient disconnect timeouts shall be cleared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question re whether there is a spec reference that could be added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some guidance / rationale text!
textile/chat-features.textile
Outdated
*** @(CHA-RL1g3)@ @[Testable]@ Any transient disconnect timeouts shall be cleared. | ||
** @(CHA-RL1h)@ If a one of the @contributors@ Realtime Channels fails to attach, then the operation has failed and must be rolled back. | ||
*** @(CHA-RL1h1)@ @[Testable]@ The @attach@ call must throw an @ErrorInfo@. The code for this error is determined by the feature that failed (see the @ErrorCodes@ enum in the JS SDK for more information). | ||
*** @(CHA-RL1h2)@ @[Testable]@ If the underlying Realtime Channel entered the @SUSPENDED@ state, then the room status will transition to @SUSPENDED@, using the Realtime Channel error as the @cause@ field of the @ErrorInfo@. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be more explicit — does it mean "if the call to contributor.attach()
fails, then check the contributor’s state
. If it's SUSPENDED, then transition to SUSPENDED, using the contributor’s errorReason
property as the cause
field of the error
of the emitted status change"?
Also, what code
and statusCode
should this error have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified on all points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the Realtime Channel error" is still a bit unclear — it could mean the error that the contributor attach()
call failed with. Can we explicitly say to use the contributor’s errorReason
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it meant to be using the same language as CHA-RL1h4 now uses — that is, to use the error thrown by channel attach()
?
** @(CHA-RL5d)@ Once all channels have been detached, the room waits until the original channel that caused the retry loop naturally enters the @ATTACHED@ state. | ||
** @(CHA-RL5e)@ @[Testable]@ If the channel state becomes @FAILED@, then the room status is transitioned to @FAILED@ and the retry loop stops. | ||
** @(CHA-RL5f)@ @[Testable]@ If the channel state becomes @ATTACHED@, then the room status is transitioned to @ATTACHING@ to begin a new attachment cycle. | ||
*** @(CHA-RL5f1)@ @[Testable]@ If the attachment cycle succeeds, then the room is transitioned to @ATTACHED@ and any discontinuity errors are broadcast to contributors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the RETRY operation terminates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
** @(CHA-RL5c)@ @[Testable]@ If the operation above fails because a channel has entered the @FAILED@ state, then the retry loop must stop and the room must be placed in the @FAILED@ status. | ||
** @(CHA-RL5d)@ Once all channels have been detached, the room waits until the original channel that caused the retry loop naturally enters the @ATTACHED@ state. | ||
** @(CHA-RL5e)@ @[Testable]@ If the channel state becomes @FAILED@, then the room status is transitioned to @FAILED@ and the retry loop stops. | ||
** @(CHA-RL5f)@ @[Testable]@ If the channel state becomes @ATTACHED@, then the room status is transitioned to @ATTACHING@ to begin a new attachment cycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "attachment cycle" is not defined and I am struggling to even guess what it means. I thought that perhaps this spec point was basically just saying "perform the ATTACH operation again", but that wouldn't make sense because we'd end up in deadlock (because CHA-RL1d would prevent ATTACH from proceeding due to the in-progress RETRY). So I guess it means something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I will clarify on this one - essentially it's "begin the actual process of attaching, but without the need to lock a mutex to prevent other operations". In chat-js it's essentially just calling _doAttach
for reference
|
||
* @(CHA-GP1)@ As far as is practicable, the implementation details such as underlying Realtime Channels should be hidden from the public API. This allows developers to interact with Chat without having to understand many low-level primitives. | ||
* @(CHA-GP2)@ The public API should avoid implicit operations as a side-effect to some other operation. For example, adding a subscriber to messages in a Chat Room should not automatically trigger that Room to attach. This is in contrast to the current core SDKs. Avoiding side-effects provides a clean, easy to understand API. | ||
* @(CHA-GP3)@ Wherever possible, Chat features should be exposed in the public API as properties of their parent. For example, @messages@ would be considered a property of a @room@ object. This allows for greater composability and extensibility in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too confusing statement ->
Chat features should be exposed in the public API as properties of their parent.
I think following terms should be explained properly
- Chat feature
- Chat contributor
- Parent of chat feature
- What is a property exactly? This is a language idiomatic statement and can't be generalized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I look at public API, all of the operations are focused on room which is Atomic element of chat
.
But not sure why we use chat feature
, chat contributor
, this makes spec more confusing : (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead we could call it room feature
, room object
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a language idiomatic statement and can't be generalized
Why do you think it can’t be generalized? I believe that all OOP languages have some concept of properties (members, attributes).
Chat feature
Chat contributor
What is not clear for you in Room Lifecycle where Chat "feature" an "contributor" are explained?
Parent of chat feature
Maybe it’s not the best choice of words, but it’s pretty clear that features are properties of an object, right? This object is essentially the parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can agree with member or attribute, property has a different meaning in dotnet. They have getters and setters only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property has a different meaning in dotnet.
dotnet's property is exactly what we want here
Missing IDL can also be one of the reasons people will face issues understanding the spec.
Why do you need IDL? We already have all public interfaces and data structures defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have it defined, but inconsistent naming makes it difficult to understand. Like DefaultRoom
, DefaultMessages
class etc. Only when we look at public API, we understand the use-case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property
is a technical term that is only applicable for dotnet, kotlin etc. For like java, go languages, member as a generalised term would make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property
often broadly describes a member that is accessible via getters and setters, whilst it has special meaning in some languages, it's quite a general term. In any case, the C# idiom is exactly what we want here (albeit, private setters).
Note that we also use the term property
extensively throughout the core spec in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but it also mentions parent child relationship at the same time,
ParentObject#property, which is way easier to understand.
I would loved if we list out exact types of contributors that exists and their relationship with parent.
i.e. in our case, there's only one -> room
|
||
h3(#rooms-status). Room Statuses | ||
|
||
The status of any given Chat Room is the combination of the states of all of its constituent contributors. For more information on this, see "@Room Lifecycle@":#rooms-lifecycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please use some other keyword than contributors
, this is a super confusing keyword and takes spec into whole different direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can rather use room elements
, room objects
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to any other keyword but surely not contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term contributor
was chosen because in the context of the Room Lifecycle, a contributor is something that contributes to the room lifecycle and status. I clarify this point in the pointed room lifecycle section - which I can try to make clearer.
Element
and Object
are non-descriptive of what the things actually do or mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, element
, and object
are non-descriptive when used individually, but they do mean something when we say room object
or room element
, although less descriptive, they convey the meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When contributor
is used individually, it's difficult to guess if it's related to chat
object or room
object.
Our current implementation is mainly resolved around room
, so it will be great if we can make it explicit there.
@AndyTWF can this be extended to include Typing Indicators please? I expect to get to this a week today if that's okay? 🙏 |
Based on [1] at d8ec6b1. [1] - ably/specification#200
Based on [1] at d8ec6b1. [1] - ably/specification#200
Based on [1] at d8ec6b1. [1] - ably/specification#200
Based on [1] at d8ec6b1. [1] - ably/specification#200
Based on [1] at d8ec6b1. [1] - ably/specification#200
Based on [1] at d8ec6b1. [1] - ably/specification#200
Hi @AndyTWF, general question, We mutex lock/ priority queue executor in chat-js, which seems imp. part of room lifecycle.
I am not sure why it's not mentioned in the chat spec |
Based on [1] at d8ec6b1. [1] - ably/specification#200
Based on [1] at d8ec6b1. [1] - ably/specification#200
- As of [CHADR-066], it was decided that metadata and headers will no longer have the reserved `ably-chat` namespace. - Removed this for both chat messages and room reactions.
f182145
to
6118c46
Compare