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

End pending signaling when user is no longer in the room #1330

Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Dec 4, 2018

When there was a ping call the current room was left as soon as the ping returned with an error 404.

Since the ping was removed the current room is left after receiving three signaling errors in a row, each one with a pause of five seconds before the next request.

However, the biggest delay comes from the signaling not being ended when the user is no longer in the room. When a participant is removed or disconnected from a room the refresh-participant-list signaling message is added to all the participants in that room. However, the participant just removed or disconnected is no longer in the room, and thus the participant did not receive the message. This pull request fixes that, as well as adding the refresh-participant-list message to all the participants in a room just deleted.

Note, however, that the participant will never receive that message from the signaling endpoint, but a 404 error due to no longer being in the room. In any case, this ends the pending signaling request as soon as the participant is removed or disconnected or the room is deleted instead of keep waiting until the timeout ends.

Besides that, I am not sure if the current room should be left as soon as a 404 error is returned by the signaling (by appending jqXHR.status === 404 || to the condition to stop retrying, or if there could be any case in which retrying would return a different response.

How to test
Scenario 1

  • As userA, create a group room and add userB
  • As userB, join to the group room
  • As userA, remove userB from the group room

Scenario 2

  • As userA, create a group room and add userB
  • As userA, create a different room and join it
  • As userB, join to the group room
  • As userA, remove the group room without joining to it (otherwise the user will be removed from the room before deleting it, and thus the refresh-participants-list message will be sent for userB due to the change in the participants instead of due to postDeleteRoom which is the scenario tested here).

Result with this pull request
The UI will show This conversation has ended after 15-20 seconds (if the change mentioned above regarding the error 404 is added then the UI will be updated almost immediately).

Result without this pull request
The UI will show This conversation has ended after up to 45 seconds.

When a participant is removed or disconnected from a room the
"refresh-participant-list" signaling message is added to all the
participants in that room. However, the participant just removed or
disconnected is no longer in the room, so the message needs to be
explicitly added for that participant.

Note, however, that the participant will never receive that message from
the signaling endpoint, but a 404 error due to no longer being in the
room. In any case, this ends the pending signaling request as soon as
the participant is removed or disconnected instead of keep waiting until
the timeout ends.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Note, however, that the participant will never receive that message from
the signaling endpoint, but a 404 error due to the room no longer
existing. In any case, this ends the pending signaling request as soon
as the room is deleted instead of keep waiting until the timeout ends.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu added 3. to review bug feature: signaling 📶 Internal and external signaling backends feature: frontend 🖌️ "Web UI" client labels Dec 4, 2018
@danxuliu danxuliu added this to the 💚 Next Major milestone Dec 4, 2018
…w query

Signed-off-by: Joas Schilling <coding@schilljs.com>
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Works good and makes it much more reactive.

I removed your added method again and used the unified method which existed already

@danxuliu
Copy link
Member Author

danxuliu commented Dec 5, 2018

I removed your added method again and used the unified method which existed already

👍

@danxuliu
Copy link
Member Author

danxuliu commented Dec 5, 2018

Any opinion on?:

I am not sure if the current room should be left as soon as a 404 error is returned by the signaling, or if there could be any case in which retrying would return a different response.

@nickvergessen
Copy link
Member

Follow up

@nickvergessen nickvergessen merged commit 56f56f3 into master Dec 5, 2018
@nickvergessen nickvergessen deleted the end-pending-signaling-when-user-is-no-longer-in-the-room branch December 5, 2018 18:53
marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019
Bumps [eslint-plugin-import](https://github.com/benmosher/eslint-plugin-import) from 2.17.1 to 2.17.2.
<details>
<summary>Changelog</summary>

*Sourced from [eslint-plugin-import's changelog](https://github.com/benmosher/eslint-plugin-import/blob/master/CHANGELOG.md).*

> ## [2.17.2] - 2019-04-16
> 
> ### Fixed
> - [`no-unused-modules`]: avoid crash when using `ignoreExports`-option ([#1331], [#1323], thanks [@&#8203;rfermann])
> - [`no-unused-modules`]: make sure that rule with no options will not fail ([#1330], [#1334], thanks [@&#8203;kiwka])
</details>
<details>
<summary>Commits</summary>

- [`eddcfa9`](import-js/eslint-plugin-import@eddcfa9) Bump to v2.17.2
- [`b151d04`](import-js/eslint-plugin-import@b151d04) [fix] `no-unused-modules`: avoid crash when using `ignoreExports`-option
- [`3512563`](import-js/eslint-plugin-import@3512563) [fix] `no-unused-modules`: make sure that rule with no options will not fail
- [`8e0c021`](import-js/eslint-plugin-import@8e0c021) [Test] `no-unused-modules` add failing test case
- [`9b7a970`](import-js/eslint-plugin-import@9b7a970) [meta] add `npm run mocha` for easier unit testing
- See full diff in [compare view](import-js/eslint-plugin-import@v2.17.1...v2.17.2)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=eslint-plugin-import&package-manager=npm_and_yarn&previous-version=2.17.1&new-version=2.17.2)](https://dependabot.com/compatibility-score.html?dependency-name=eslint-plugin-import&package-manager=npm_and_yarn&previous-version=2.17.1&new-version=2.17.2)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
Dependabot will merge this PR once it's up-to-date and CI passes on it, as requested by @skjnldsv.

[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: frontend 🖌️ "Web UI" client feature: signaling 📶 Internal and external signaling backends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants