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

Fix using signaling settings while being refetched #8427

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Dec 2, 2022

Follow up to #7472

When the external signaling server returns the error token_expired the signaling settings are fetched again. However, if there are further requests and token_expired is returned again the previous fetch is canceled and a new one started instead, which caused the settings to be temporary set to null. The signaling settings are expected to always be an object, so setting them to null could cause an error if they were used during that time (for example, during a reconnection, which caused the signaling object to "hang" and not do any further connection attempt).

Preventing the settings to be nullified is only half of the story, though; token_expired can be thrown when trying to connect, and errors thrown when trying to connect cause a reconnection attempt. This could end in a reconnection loop, as each token_expired error would cause another fetch of the settings, cancelling the previous fetch and thus preventing the settings to be updated, and as the previous settings would be still used any connection attempt would end again in another token_expired error.

To solve all that now any connection attempt done after receiving a token_expired error is deferred until the signaling settings were updated.

Note that the previous signaling settings are kept to ensure that a signaling object is always available, even if outdated. However, any usage of the outdated signaling settings is expected to cause a token_expired error to be thrown; right now that only happens during connections, so only that code needs to wait for the settings to be fetched again.

How to test

window.getSignaling = function() {
	return signaling
}
  • Rebuild the code
  • Setup the HPB
  • Open a conversation in Talk
  • Wait more than one minute to ensure that the authentication token received in the signaling settings has expired
  • In the browser console, run getSignaling().forceReconnect(true)

Result with this pull request

The first reconnection attempt fails with token_expired, and then the connection with the signaling server is established again once the signaling settings were updated

Result without this pull request

The first and second reconnection attempts fail with token_expired, and this.settings is null is thrown in the third and last attempt, as it was done while fetching the settings again for the second time, which cancelled the first fetching and caused the settings to be set to null

@danxuliu danxuliu added 2. developing bug feature: signaling 📶 Internal and external signaling backends labels Dec 2, 2022
@danxuliu danxuliu added this to the 💟 Next Major (26) milestone Dec 2, 2022
@danxuliu
Copy link
Member Author

danxuliu commented Dec 2, 2022

/backport to stable25

@danxuliu danxuliu force-pushed the fix-using-signaling-settings-while-being-refetched branch from 38ad130 to e74272e Compare August 15, 2023 03:41
@danxuliu danxuliu marked this pull request as ready for review August 15, 2023 03:45
@danxuliu danxuliu requested a review from Antreesy August 15, 2023 03:45
@danxuliu
Copy link
Member Author

/backport to stable27

@danxuliu
Copy link
Member Author

/backport to stable26

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested, seems to work as expected.
Code is clean and readable, algorithm is clean.

Thanks for notes and explanations!

src/utils/signaling.js Outdated Show resolved Hide resolved
When the external signaling server returns the error "token_expired" the
signaling settings are fetched again. However, if there are further
requests and "token_expired" is returned again the previous fetch is
canceled and a new one started instead, which caused the settings to be
temporary set to "null". The signaling settings are expected to always
be an object, so setting them to "null" could cause an error if they
were used during that time (for example, during a reconnection, which
caused the signaling object to "hang" and not do any further connection
attempt).

Preventing the settings to be nullified is only half of the story,
though; "token_expired" can be thrown when trying to connect, and errors
thrown when trying to connect cause a reconnection attempt. This could
end in a reconnection loop, as each "token_expired" error would
cause another fetch of the settings, cancelling the previous fetch and
thus preventing the settings to be updated, and as the previous settings
would be still used any connection attempt would end again in another
"token_expired" error.

To solve all that now any connection attempt done after receiving a
"token_expired" error is deferred until the signaling settings were
updated.

Note that the previous signaling settings are kept to ensure that a
signaling object is always available, even if outdated. However, any
usage of the outdated signaling settings is expected to cause a
"token_expired" error to be thrown; right now that only happens during
connections, so only that code needs to wait for the settings to be
fetched again.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@danxuliu danxuliu force-pushed the fix-using-signaling-settings-while-being-refetched branch from fce3a03 to 0a14a20 Compare August 17, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: signaling 📶 Internal and external signaling backends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants