chat: clarify room lifecycle in progress assumption #255
+16
−16
Conversation
This file contains 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
sacOO7
reviewed
Dec 18, 2024
| @@ -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. | |||
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
isLocked() returns true if there is ongoing/pending operations, otherwise false if no operation.
sacOO7
reviewed
Dec 18, 2024
| *** @(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. | ||
| *** @(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. |
There was a problem hiding this comment.
sacOO7
reviewed
Dec 18, 2024
| ** @(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. |
There was a problem hiding this comment.
sacOO7
reviewed
Dec 18, 2024
| *** @(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. |
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Clarifies the assumption that room lifecycle operation in progress does indeed refer to the given room.
Resolves #251