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.

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 attachment requirements #249

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chat: room attachment requirements #249

wants to merge 2 commits into from

Conversation

AndyTWF
Copy link
Contributor

@AndyTWF AndyTWF commented Nov 28, 2024

Adding room attachment requirements to Typing and Presence.leave

@sacOO7
Copy link
Collaborator

sacOO7 commented Nov 28, 2024

PR is upgrade of missing spec in #232

@sacOO7
Copy link
Collaborator

sacOO7 commented Nov 28, 2024

Can we also update
@(CHA-PR10e)@ @[Testable]@ If the room status is @Attached@, then the @update@ call will invoke the underlying @presence.enter()@ call.
to
@(CHA-PR10e)@ @[Testable]@ If the room status is @Attached@, then the @update@ call will invoke the underlying @presence.update()@ call.

@sacOO7
Copy link
Collaborator

sacOO7 commented Nov 28, 2024

While I agree that we need to ensure the room state is ATTACHED before performing any room operations, we currently face a lot of specification duplication for user-triggered room actions.

In Kotlin, we've defined the ensureAttached method as part of the DefaultRoom class, which returns a promise (in JavaScript) or a callback in other languages.

DefaultRoom.ensureAttached() method:

  • This method is simply called before executing any user-triggered real-time operations, at multiple places.
  • It's easy to test and avoid lot of code duplication/ simplifies spec understanding.

@sacOO7
Copy link
Collaborator

sacOO7 commented Nov 28, 2024

Perhaps we can streamline the specification so that we don't need to define multiple spec points (currently 3) for basic user-triggered real-time room operations. This will help in future specifications as well.

@AndyTWF
Copy link
Contributor Author

AndyTWF commented Nov 29, 2024

While I agree that we need to ensure the room state is ATTACHED before performing any room operations, we currently face a lot of specification duplication for user-triggered room actions.

In Kotlin, we've defined the ensureAttached method as part of the DefaultRoom class, which returns a promise (in JavaScript) or a callback in other languages.

DefaultRoom.ensureAttached() method:

  • This method is simply called before executing any user-triggered real-time operations, at multiple places.
  • It's easy to test and avoid lot of code duplication/ simplifies spec understanding.

I'm cautious to define exact implementation details (e.g. specific method names) in the spec, so as to give latitude to different SDKs to be idiomatic to their ecosystems.

But in a future PR I'll look at consolidating the points - perhaps a GA item

*** @(CHA-T4a3)@ @[Testable]@ If the room status is @Attaching@, then the operation shall invoke a @CHA-RL9@ pending room status operation and wait for it to complete. If this operation fails, then the error from @CHA-RL9@ must be raised. If this succeeds (i.e the room becomes @ATTACHED@), the library will proceed per @CHA-T4a1@.
*** @(CHA-T4a4)@ @[Testable]@ If the room status is anything other than @Attached@ or @Attaching@, an @ErrorInfo@ using the @RoomInInvalidState@ error code from the "chat-specific error codes":#error-codes with a status code of @400@ must be thrown. The message shall explain that attach must be called first.
*** @(CHA-T4a1)@ @[Testable]@ If the room status is @Attached@, then the @start@ call will invoke the underlying @presence.enter()@ call.
*** @(CHA-T4a2)@ @[Testable]@ When a typing session is started per @CHA-T4a1@, a timeout is set according to the @CHA-T3@ timeout interval. When this timeout expires, the typing session is automatically ended by leaving presence.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
*** @(CHA-T4a2)@ @[Testable]@ When a typing session is started per @CHA-T4a1@, a timeout is set according to the @CHA-T3@ timeout interval. When this timeout expires, the typing session is automatically ended by leaving presence.
*** @(CHA-T4a2)@ @[Testable]@ When a typing session is started per @CHA-T4a1@, a timeout is set according to the @CHA-T3@ timeout interval. When this timeout expires, the typing session automatically ends by leaving presence as per @CHA-T5c@.

Copy link
Collaborator

@sacOO7 sacOO7 Dec 3, 2024

Choose a reason for hiding this comment

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

I think we should implement an internal retry mechanism if the implicit presence.leave fails due to a timeout.
This can become important because we won't be notifying the user about errors produced by leave event due to implicit typing timeout.
Though, we do have an explicit stop method that takes notifies user whether typing stopped or not, most of the times user will rely on start method to produce stop event on timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - this is a known bug that we intend to fix at a later date, I would prefer not to spec for it until we know what that is

Copy link
Collaborator

@sacOO7 sacOO7 Dec 3, 2024

Choose a reason for hiding this comment

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

if implicit leave event fails due to typing timeout, receivers will see eternal typing in progress : (
Unless, user disconnects explicitly. Also, on reconnection, presence members are entered again.

Copy link
Contributor Author

@AndyTWF AndyTWF Dec 3, 2024

Choose a reason for hiding this comment

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

Yes - it's a known bug tracked in a Chat JIRA ticket. Once someone fixes it in a satisfactory way (in any of the SDKs), we'll update the spec - I just don't want to write a spec point for it until we have a tried and tested fix :)

Copy link
Collaborator

@sacOO7 sacOO7 Dec 3, 2024

Choose a reason for hiding this comment

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

If we are tracking it somewhere then good 👍

  • Otherwise you might like to create separate github issue for this WDYT
  • I think use-case is pretty clear => if leave fails on timeout then we have eternal typing
    event on receiver side 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an idea,

  • All implicit ops that don't notify user about the error are retried for max_retries = 3 and then added to the internal queue of implicit failed operations.
  • All those operations will be performed again once the connection goes connected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems as per RTP16b, presence events will be queued at channel level when channel in Initialized or Attaching state.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For other states, operation will result in error as per RTP16c

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might like to add those points to JIRA ticket, I will approve the PR for now, to unblock the spec

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

LGTM.
You can add some points from conversation into internal JIRA ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants