Skip to content

Don't try to reconnect the socket when we closed it intentionally#3402

Merged
nickvergessen merged 4 commits into
masterfrom
bugfix/noid/dont-reconnect-when-we-closed
May 5, 2020
Merged

Don't try to reconnect the socket when we closed it intentionally#3402
nickvergessen merged 4 commits into
masterfrom
bugfix/noid/dont-reconnect-when-we-closed

Conversation

@nickvergessen

Copy link
Copy Markdown
Member

Extracted from #3292

@nickvergessen nickvergessen added 3. to review bug feature: signaling 📶 Internal and external signaling backends labels Apr 27, 2020
@nickvergessen nickvergessen added this to the 💚 Next Major (19) milestone Apr 27, 2020
@nickvergessen nickvergessen requested a review from danxuliu April 27, 2020 13:31
@nickvergessen nickvergessen force-pushed the bugfix/noid/dont-reconnect-when-we-closed branch from 64d10a3 to f39920d Compare April 27, 2020 21:00
@nickvergessen

Copy link
Copy Markdown
Member Author

/backport to stable18

@danxuliu danxuliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have added some fixes on top of the current strategy.

However, I have realized that the whole beforeunload handling could be removed if we just check the code attribute in the close event. In theory when the code is 1001 the user is navigating away, so if we do not try to reconnect in that case it should be enough.

Right now, if the user tries to navigate away, stays in the call and the external signaling server goes away in that moment then it will not get reconnected. Sure, a very corner case, but possible nevertheless. This would be prevented by checking the code instead of using the current approach (although there could be other drawbacks I am not seeing, I do not know).

@nickvergessen

Copy link
Copy Markdown
Member Author

Maybe I mixed soemthing up, but the real problem I had which is why I did this was when the connection is terminated because we switch the HPB because of the clustering. But maybe the code works there as well

@danxuliu danxuliu force-pushed the bugfix/noid/dont-reconnect-when-we-closed branch from 97d5336 to ea53684 Compare April 29, 2020 11:11
@danxuliu

Copy link
Copy Markdown
Member

Yes, but the commit is covering two different cases: prevent reconnecting when the signaling is disconnected, and prevent reconnecting when the page is about to be unloaded.

As far as I understand it, prevent reconnecting when the signaling is disconnected is what is needed for the HPB clustering. That is OK, and it is not related to my comment. I only refered to prevent reconnecting when the page is about to be unloaded, as I think that could be handled easier just by checking the code from the close event, with no needs for beforeunload events, timers and so on.

@nickvergessen

Copy link
Copy Markdown
Member Author

Feel free to add/change it

@nickvergessen nickvergessen force-pushed the bugfix/noid/dont-reconnect-when-we-closed branch from ea53684 to 49b06ba Compare April 29, 2020 15:24
@nickvergessen

Copy link
Copy Markdown
Member Author

Updated and much less intrusive now!
Thanks for the pointer to the close code @danxuliu

@nickvergessen nickvergessen requested a review from danxuliu April 29, 2020 15:25
@danxuliu danxuliu force-pushed the bugfix/noid/dont-reconnect-when-we-closed branch from 49b06ba to 0235c9b Compare May 4, 2020 13:42
@nickvergessen

nickvergessen commented May 4, 2020

Copy link
Copy Markdown
Member Author

The commit Fix signaling toasts not being permanent should be dropped in favor of a rebase since #3449

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
…n top of error

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/dont-reconnect-when-we-closed branch from 0235c9b to 4981c33 Compare May 4, 2020 14:05
@nickvergessen

Copy link
Copy Markdown
Member Author

Rebased and dropped it

@danxuliu danxuliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works 👍

@nickvergessen nickvergessen merged commit bbb6d94 into master May 5, 2020
@nickvergessen nickvergessen deleted the bugfix/noid/dont-reconnect-when-we-closed branch May 5, 2020 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug feature: signaling 📶 Internal and external signaling backends

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants