Skip to content
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.

Sign up for GitHub

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: clarify room lifecycle in progress assumption #255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chat: clarify room lifecycle in progress assumption #255

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions textile/chat-features.textile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ h4(#rooms-lifecycle-operations). Room Lifecycle Operations
** @(CHA-RL1j)@ This specification point has been removed.
** @(CHA-RL1b)@ @[Testable]@ If the room is in the @RELEASING@ status, the operation shall be rejected with an @ErrorInfo@ with the @RoomIsReleasing@ error code from the "chat-specific error codes":#error-codes.
** @(CHA-RL1c)@ @[Testable]@ If the room is in the @RELEASED@ status, the operation shall be rejected with an @ErrorInfo@ with the @RoomIsReleased@ error code from the "chat-specific error codes":#error-codes.
** @(CHA-RL1d)@ @[Testable]@ If a room lifecycle operation is already in progress, this operation shall wait until the current operation completes before continuing, subject to @CHA-RL7@.
** @(CHA-RL1d)@ @[Testable]@ If a room lifecycle operation is already in progress for the given room, this operation shall wait until the current operation completes before continuing, subject to @CHA-RL7@.
** @(CHA-RL1e)@ @[Testable]@ Notwithstanding the above points, when the attachment operation begins, the room shall be transitioned to the @ATTACHING@ status.
** @(CHA-RL1f)@ @[Testable]@ The underlying @contributors@ will have their Realtime Channels attached in sequence - an attach call must complete successfully before the next is started.
** @(CHA-RL1g)@ When all @contributors@ Realtime Channels successfully attach (the calls to @attach()@ complete successfully), the operation is now complete and the room is considered attached.
Expand Down Expand Up @@ -141,7 +141,7 @@ h4(#rooms-lifecycle-operations). Room Lifecycle Operations
*** @(CHA-RL2h1)@ @[Testable]@ If a channel enters the @FAILED@ state during detachment (i.e. the @detach()@ operation fails and upon subsequently checking the channel state, it is @FAILED@), then the room will transition to the @FAILED@ status. The detach operation continues until, for every channel, either a call to @detach()@ has succeeded, or the channel enters the @FAILED@ state, with the operation throwing an @ErrorInfo@ with the feature-specific error code of the first feature to fail, using the error returned by the call to @detach()@ as the @cause@. The same @ErrorInfo@ must accompany the @FAILED@ room status.
*** @(CHA-RL2h2)@ @[Testable]@ If the room is already in a @FAILED@ status during the detach operation and another channel fails, the @FAILED@ status will not be emitted twice.
*** @(CHA-RL2h3)@ @[Testable]@ A channel may enter another state during detachment (namely @ATTACHED@, which happens when detach times out), whereby the call to @detach()@ on the realtime channel will throw an error and the channel status can be ascertained by inspecting the channel object. If this happens, the @CHA-RL2f@ detachment cycle shall be retried after a 250ms pause. The room status shall not change during this retry process.
** @(CHA-RL2i)@ @[Testable]@ If a room lifecycle operation is already in progress, this operation shall wait until the current operation completes before continuing, subject to @CHA-RL7@.
** @(CHA-RL2i)@ @[Testable]@ If a room lifecycle operation is already in progress for the given room, this operation shall wait until the current operation completes before continuing, subject to @CHA-RL7@.
* @(CHA-RL3)@ A room must be explicitly released via the @RELEASE@ operation. This operation is not performed directly on a Room object by the client, but is described here.
** @(CHA-RL3a)@ @[Testable]@ If the room is already in the @RELEASED@ status, this operation is no-op.
** @(CHA-RL3b)@ @[Testable]@ If the room is in the @DETACHED@ status, then the room is immediately transitioned to @RELEASED@ and the operation succeeds.
Expand All @@ -154,7 +154,7 @@ h4(#rooms-lifecycle-operations). Room Lifecycle Operations
** @(CHA-RL3f)@ @[Testable]@ If a channel fails to detach (i.e. the call to @detach()@ returns an error), the @CHA-RL3d@ channel detach sequence will be retried after a 250ms delay. Retries are continued until @CHA-RL3g@ is met.
** @(CHA-RL3g)@ @[Testable]@ Once all channels have entered the @DETACHED@ (i.e. the call to @detach()@ succeeds) or @FAILED@ (i.e. the call to @detach()@ fails and the channel state is subsequently @FAILED@) state, then the room state is transitioned to @RELEASED@ and the operation completes.
** @(CHA-RL3h)@ @[Testable]@ Upon operation completion, the underlying Realtime Channels are released from the core SDK prevent leakage.
** @(CHA-RL3k)@ @[Testable]@ If a room lifecycle operation is already in progress, this operation shall wait until the current operation completes before continuing, subject to @CHA-RL7@.
** @(CHA-RL3k)@ @[Testable]@ If a room lifecycle operation is already in progress for the given room, this operation shall wait until the current operation completes before continuing, subject to @CHA-RL7@.
* @(CHA-RL5)@ A room must @RETRY@ whenever it enters the @SUSPENDED@ state. This specification point describes the behavior that is executed as a result of @CHA-RL4b9@ and @CHA-RL1h3@. The @RETRY@ operation is summarized as a loop that terminates either when a realtime channel enters the @FAILED@ state, or all channels are back in @ATTACHED@.
** @(CHA-RL5a)@ @[Testable]@ When entering a @RETRY@ operation, the room must first @DETACH@ all contributors underlying realtime channels using a @CHA-RL2f@ detachment cycle, with the exception of the channel that became @SUSPENDED@.
*** @(CHA-RL5a1)@ @[Testable]@ As many chat features share channels, the equality of contributors when deciding not to detach is based on their realtime channel, and not the contributor themselves. i.e. if two features share a realtime channel, and that channel is suspended, then that channel should not be detached.
Expand Down Expand Up @@ -188,19 +188,19 @@ As well as user-initiated operations, the room must monitor its underlying resou
** @(CHA-RL4a)@ The state monitor must handle @UPDATE@ events from each contributors underlying Realtime Channel
*** @(CHA-RL4a1)@ @[Testable]@ If the @resumed@ flag of the update is set to @true@, then no action should be taken (i.e. @CHA-RL4a3@ and @CHA-RL4a4@ behaviours are not performed).
*** @(CHA-RL4a2)@ @[Testable]@ If the given contributor has not yet successfully managed to attach its Realtime Channel (i.e. no call to @attach()@ on the channel, per @CHA-RL1f@, has succeeded), then no action should be taken (i.e. @CHA-RL4a3@ and @CHA-RL4a4@ behaviours are not performed).
*** @(CHA-RL4a3)@ @[Testable]@ If a room lifecycle operation is in progress, then a pending discontinuity event shall be recorded for this contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change. The event will be notified to the contributor at a later point, as noted in this specification.
*** @(CHA-RL4a4)@ @[Testable]@ If a room lifecycle operation is not in progress, then a discontinuity event will immediately be emitted to the contributor. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change.
*** @(CHA-RL4a3)@ @[Testable]@ If a room lifecycle operation is in progress for the given room, then a pending discontinuity event shall be recorded for this contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change. The event will be notified to the contributor at a later point, as noted in this specification.
Copy link
Collaborator

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*** @(CHA-RL4a3)@ @[Testable]@ If a room lifecycle operation is in progress for the given room, then a pending discontinuity event shall be recorded for this contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change. The event will be notified to the contributor at a later point, as noted in this specification.
*** @(CHA-RL4a3)@ @[Testable]@ If a room lifecycle operation is in progress and has pending operations (queued as per CHA-RL7), then given discontinuity event shall be recorded for this contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change. The event will be notified to the contributor at a later point, as noted in this specification.

Copy link
Collaborator

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to conform to room atomicity as per CHA-RL7 and events emitted by ongoing operation, captured by room monitoring shouldn't affect pending queued operations WDYT

Copy link
Collaborator

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all atomic RETRY, RELEASE, ATTACH OR DETACH operations takes precedence over Room monitoring, hence room monitoring events triggered as a side effect of any of those operations shouldn't cause any ripple effect for queued atomic operations.

Copy link
Collaborator

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In chat-js,

private _operationInProgress = false;

can simply be updated to

private operationInProgress = () -> _mtx.isLocked()

Changing code to above will result in the exact same behaviour ( since JS is single threaded )

At the same time, we will have better readability and will significantly reduce current/future efforts of setting/resetting _operationInProgress flag.

Copy link
Collaborator

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLocked() returns true if there is ongoing/pending operations, otherwise false if no operation.

*** @(CHA-RL4a4)@ @[Testable]@ If a room lifecycle operation is not in progress for the given room, then a discontinuity event will immediately be emitted to the contributor. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/ably/specification/pull/255/files#r1890223205

** @(CHA-RL4b)@ The state monitor must handle non-@UPDATE@ channel state events.
*** @(CHA-RL4b1)@ @[Testable]@ If a room lifecycle operation is in progress, and the new channel state is @ATTACHED@, and the @resumed@ flag is false, @and@ the particular contributor has been attached previously (i.e. a previous call to @attach()@ on the channel, per @CHA-RL1f@, has succeeded), then a pending discontinuity event will be recorded for the contributor. The error associated with this event shall be the @reason@ for the channel state change.
*** @(CHA-RL4b1)@ @[Testable]@ If a room lifecycle operation is in progress for the given room, and the new channel state is @ATTACHED@, and the @resumed@ flag is false, @and@ the particular contributor has been attached previously (i.e. a previous call to @attach()@ on the channel, per @CHA-RL1f@, has succeeded), then a pending discontinuity event will be recorded for the contributor. The error associated with this event shall be the @reason@ for the channel state change.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/ably/specification/pull/255/files#r1890223205

*** @(CHA-RL4b2)@ This specification point has been removed.
*** @(CHA-RL4b3)@ This specification point has been removed.
*** @(CHA-RL4b4)@ This specification point has been removed.
*** @(CHA-RL4b5)@ @[Testable]@ If a room lifecycle operation is not in progress and the channel state is @FAILED@, then the room status shall be transitioned to @FAILED@, using the @reason@ for the channel state change as the @error@ for the room status change. All @transient disconnect timeouts@ are cancelled and a @CHA-RL2f@ detach procedure is performed.
*** @(CHA-RL4b5)@ @[Testable]@ If a room lifecycle operation is not in progress for the given room and the channel state is @FAILED@, then the room status shall be transitioned to @FAILED@, using the @reason@ for the channel state change as the @error@ for the room status change. All @transient disconnect timeouts@ are cancelled and a @CHA-RL2f@ detach procedure is performed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/ably/specification/pull/255/files#r1890223205

*** @(CHA-RL4b6)@ This specification point has been removed.
*** @(CHA-RL4b7)@ @[Testable]@ If a room lifecycle operation is not in progress and the channel state is @ATTACHING@ and no transient disconnect timeout exists for the contributor, then a transient disconnect timeout with a 5 second limit is created for the contributor. Upon timeout, the room status is transitioned to @ATTACHING@, using the @reason@ from the initial channel state change as the error for the transition.
*** @(CHA-RL4b10)@ @[Testable]@ If a room lifecycle operation is not in progress and the channel state is @ATTACHED@ and a transient disconnect timeout exists for the contributor, the timeout is cleared.
*** @(CHA-RL4b8)@ @[Testable]@ If a room lifecycle operation is not in progress, the channel state is @ATTACHED@, the room status is NOT @ATTACHED@ and all contibutors channel are now @ATTACHED@, the room status is transitioned to @ATTACHED@.
*** @(CHA-RL4b9)@ @[Testable]@ If a room lifecycle operation is not in progress and the channel state is @SUSPENDED@, then the room status is transitioned to @SUSPENDED@, using the @reason@ of the channel state change as the error. Any transient disconnect timeouts are cancelled and the room enters the @RETRY@ loop.
*** @(CHA-RL4b7)@ @[Testable]@ If a room lifecycle operation is not in progress for the given room and the channel state is @ATTACHING@ and no transient disconnect timeout exists for the contributor, then a transient disconnect timeout with a 5 second limit is created for the contributor. Upon timeout, the room status is transitioned to @ATTACHING@, using the @reason@ from the initial channel state change as the error for the transition.
*** @(CHA-RL4b10)@ @[Testable]@ If a room lifecycle operation is not in progress for the given room and the channel state is @ATTACHED@ and a transient disconnect timeout exists for the contributor, the timeout is cleared.
*** @(CHA-RL4b8)@ @[Testable]@ If a room lifecycle operation is not in progress for the given room, the channel state is @ATTACHED@, the room status is NOT @ATTACHED@ and all contibutors channel are now @ATTACHED@, the room status is transitioned to @ATTACHED@.
*** @(CHA-RL4b9)@ @[Testable]@ If a room lifecycle operation is not in progress for the given room and the channel state is @SUSPENDED@, then the room status is transitioned to @SUSPENDED@, using the @reason@ of the channel state change as the error. Any transient disconnect timeouts are cancelled and the room enters the @RETRY@ loop.


h2(#room-configuration). Room Configuration
Expand All @@ -212,8 +212,8 @@ Each chat room can be configured individually, allowing options to be passed as
** @(CHA-RC1f)@ @[Testable]@ Requesting a room from the Chat Client shall return a future, that eventually resolves to an instance of a room with the provided id and options.
*** @(CHA-RC1f1)@ @[Testable]@ If the room id exists in the room map, but the room has been requested with different options, then an @ErrorInfo@ with code @40000@ is thrown.
*** @(CHA-RC1f2)@ @[Testable]@ If the room id exists in the room map, and it is requested with the same options, then the same instance of the room must be reused.
*** @(CHA-RC1f3)@ @[Testable]@ If no @CHA-RC1g@ release operation is in progress, a new room instance shall be created, added to the room map and returned as the future value.
*** @(CHA-RC1f4)@ @[Testable]@ If a @CHA-RC1g@ release operation is in progress, the entry shall be added to the room map, but the new room instance must not be created until the release operation has resolved. The future shall then subsequently resolve with the new room value.
*** @(CHA-RC1f3)@ @[Testable]@ If no @CHA-RC1g@ release operation is in progress for the given room, a new room instance shall be created, added to the room map and returned as the future value.
*** @(CHA-RC1f4)@ @[Testable]@ If a @CHA-RC1g@ release operation is in progress for the given room, the entry shall be added to the room map, but the new room instance must not be created until the release operation has resolved. The future shall then subsequently resolve with the new room value.
*** @(CHA-RC1f5)@ In relation to @CHA-RC1f4@, we recommend storing a future in the room map.
*** @(CHA-RC1f6)@ It should be noted that the "get" operations in this section are interruptible by a @CHA-RC1g@ release call.
** @(CHA-RC1b)@ This specification point has been removed, it was superseded by @CHA-RC1f2@
Expand All @@ -223,9 +223,9 @@ Each chat room can be configured individually, allowing options to be passed as
** @(CHA-RC1e)@ This specification point has been removed.
** @(CHA-RC1g)@ A room may be @released@, which causes it to be disposed of and eligible for garbage collection.
*** @(CHA-RC1g1)@ The release operation returns a future that shall resolve when the operation completes.
*** @(CHA-RC1g2)@ @[Testable]@ If the room does not exist in the room map, and no release operation is in progress, this operation is no-op.
*** @(CHA-RC1g3)@ @[Testable]@ If the room does not exist in the room map, and a release operation is already in progress, then the associated future will be returned.
*** @(CHA-RC1g4)@ @[Testable]@ If a release operation is already in progress, any pending @CHA-RC1f@ future shall be rejected / throw an error. The error must use the @RoomReleasedBeforeOperationCompleted@ error code from the "chat-specific error codes":#error-codes and a @statusCode@ of 400. The room shall be removed from the room map and the operation must return the future associated with the previous operation.
*** @(CHA-RC1g2)@ @[Testable]@ If the room does not exist in the room map, and no release operation is in progress for the given room, this operation is no-op.
*** @(CHA-RC1g3)@ @[Testable]@ If the room does not exist in the room map, and a release operation is already in progress for the given room, then the associated future will be returned.
*** @(CHA-RC1g4)@ @[Testable]@ If a release operation is already in progress for the given room, any pending @CHA-RC1f@ future shall be rejected / throw an error. The error must use the @RoomReleasedBeforeOperationCompleted@ error code from the "chat-specific error codes":#error-codes and a @statusCode@ of 400. The room shall be removed from the room map and the operation must return the future associated with the previous operation.
*** @(CHA-RC1g5)@ @[Testable]@ The room is removed from the room map and a @CHA-RL3@ release operation is initiated for the room object. A future is returned which resolves when the release operation completes.
* @(CHA-RC2)@ Chat rooms are configurable, so as to enable or disable certain features. When requesting a room, options as to which features should be enabled, and the configuration they should take, must be provided (@RoomOptions@).
** @(CHA-RC2a)@ @[Testable]@ If a room is requested with invalid configuration, for example: a negative typing timeout, an @ErrorInfo@ with code @40001@ must be thrown.
Expand Down
Loading