-
Notifications
You must be signed in to change notification settings - Fork 448
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
End pending signaling when user is no longer in the room #1330
Conversation
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>
…w query Signed-off-by: Joas Schilling <coding@schilljs.com>
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.
Works good and makes it much more reactive.
I removed your added method again and used the unified method which existed already
👍 |
Any opinion on?:
|
Follow up |
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 [@​rfermann]) > - [`no-unused-modules`]: make sure that rule with no options will not fail ([#1330], [#1334], thanks [@​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>
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 therefresh-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
Scenario 2
refresh-participants-list
message will be sent for userB due to the change in the participants instead of due topostDeleteRoom
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.