Reconnect on lost websocket connection #39

farao · 2020-05-21T12:52:11Z

When the websocket connection failed after having been established before,
the complete rtc session is tried to reinitialize.

This needs an update on the palava client first: palavatv/palava-client#16

farao · 2020-05-21T13:17:46Z

Two things to discuss:

is it possible to get rid of the race condition that between the if(navigator.online) and the add event listener the status could go from offline to online without the reconnect happening? maybe with a promise or so?
the check for navigator.online is bad for trying out locally I guess - but there the connection should not get lost in the first place, right?

janlelis · 2020-05-22T07:49:05Z

src/views/Room.vue

@@ -48,6 +48,7 @@ export default {
      peers: [],
      localPeer: null,
      infoPage: null,
+      signalingConnectedBefore = false,


Syntax error :/

Suggested change

signalingConnectedBefore = false,

signalingConnectedBefore: false,

I am not that familiar with javascript. But could we apply some sanitizer/compiler to our javascript files. Such that these kind of syntax errors could be detected in CI? I could setup github actions for this repository.

@erikzenker It already happens and it would be super duper to have such a github action. The command is $ yarn lint

janlelis · 2020-05-22T07:50:40Z

src/views/Room.vue

@@ -202,6 +203,18 @@ export default {
        },
      })
    },
+    reconnectRtc() {
+      if (this.signalingConnectedBefore) {


This flag is never flipped

janlelis · 2020-05-22T07:51:40Z

src/views/Room.vue

+    reconnectRtc() {
+      if (this.signalingConnectedBefore) {
+        // TODO: show "Palava server not reachable" or "Network not reachable" overlay
+        if (navigator.online) {


Haven't known about navigator.online before - good idea!

janlelis · 2020-05-22T07:56:10Z

src/views/Room.vue

+      if (this.signalingConnectedBefore) {
+        // TODO: show "Palava server not reachable" or "Network not reachable" overlay
+        if (navigator.online) {
+          rtc.reconnect()


Although we are guarding against being offline, the network can be bad (or the server could be down), so there should probably be a timeout functionality here. Maybe also a retry count (or an "endless" interval with larger timeout)

And what happens after the retry count? The person gets kicked out? I think as long as somebody wants to participate in the discussion, it should be continously tried to reconnect?

Fair point. One way would be to then show a network error page with a "reconnect" button. But the endless-interval with reasonable timeout in between tries sounds fine too me too.

janlelis · 2020-05-22T08:21:36Z

1. is it possible to get rid of the race condition that between the if(navigator.online) and the add event listener the status could go from offline to online without the reconnect happening? maybe with a promise or so?

Can you please provide an example of the race condition, it's not yet clear to me

janlelis · 2020-05-22T08:23:11Z

2\. the check for navigator.online is bad for trying out locally I guess - but there the connection should not get lost in the first place, right?

The Chrome devtools have an "offline" flag in the network tab - this should trigger this event (maybe not, but I think it does)

janlelis · 2020-05-22T08:25:36Z

src/views/Room.vue

+        if (navigator.online) {
+          rtc.reconnect()
+        } else {
+          window.addEventListener('online',  function() {rtc.reconnect()});


The event listener needs to be unregistered, when fired, or they will "stack up" with every new error

This whole online/offline state thing could also be considered an extra feature (and treated separately from connection errors)

On some point we could think of introducing some kind of state machine. Otherwise it will get hard to keep the overview.

I think as long as the websocket connection stands, everything is ok - and when it fails, this is only a safeguard not to try it again until it could actually work :-) I don't see a need to treat this separately.

I like the idea of having a state-machine (probably even the client library) - which brings together signaling, peer connection, and network/online status

farao · 2020-05-22T10:02:45Z

1. is it possible to get rid of the race condition that between the if(navigator.online) and the add event listener the status could go from offline to online without the reconnect happening? maybe with a promise or so?

Can you please provide an example of the race condition, it's not yet clear to me

Not quite sure, if javascript prevents this to happen, but what I imagine could happen:

"if (navigator.online) {" => false (offline)
navigator.online becomes true (online)
online event listener is registered but will not be called since the status changed already to online between the if and registering the listener

How about this as a solution?:

register the online listener
if navigator.online, send the online event manually (does this work?)

janlelis · 2020-05-22T14:05:39Z

Rebased + pushed to staging!

janlelis · 2020-05-22T14:16:18Z

Not quite sure, if javascript prevents this to happen, but what I imagine could happen:

I think we are safe here

farao · 2020-05-22T15:09:44Z

Not quite sure, if javascript prevents this to happen, but what I imagine could happen:

I think we are safe here

Unfortunately no:

<html>
<script>
    function sleep(ms) {
        return new Promise(resolve => setTimeout(resolve, ms));
    }
    async function test() {
        if(navigator.online) console.log("online")
        await sleep(20000)
        console.log("register listener");
        window.addEventListener("online", function() { console.log("went online"); });
    }
    console.log("start");
    test()
</script>
</html>

Disconnect from the internet
After seeing start, enable your internet again (quick! :D)
Note that "online" has not been printed
"went online" also never gets printed even though you went online again in the meantime

farao · 2020-05-22T15:10:20Z

Is it possible to emit the online event on window manually?

farao · 2020-05-22T15:24:42Z

Fixed it :-)

farao · 2020-05-22T23:54:02Z

Ok to merge or shall we do something about my original concern nr. 2 (local testing)?
And what about the TODO line (showing some information that the network (or more concretely the palava server) is not reachable?

erikzenker · 2020-05-23T07:37:41Z

Ok to merge or shall we do something about my original concern nr. 2 (local testing)?

I think going forward we should invest some energy into automatic testing. Javascript should have
decent mocking capabilities so I guess it will not get to complex.

And what about the TODO line (showing some information that the network (or more concretely the palava server) is not reachable?

What about creating a new issue and doing it in a followup? By that you keep the focus in this PR on the reconnect feature.

janlelis · 2020-05-23T07:38:48Z

@farao I would say we always put the latest version of this PR on staging for simpler real-worold testing.

Original concern nr.2: The chrome devtools let you switch your browser to offline under the network tab. They seem to have removed the handy checkbox, but you can still achieve this by creating a "throttle" configuration which sends 0 kbits. I have verified that this flips the onLine state (on Chrome)
I have created an extra issue for displaying network problems to the user in Display "reconnecting" indicator in UX when network not reachable #44
I still think relying soley on the onLine state is not enough, since (a) browser implementation varies https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine/onLine and (b) the network is generally unreliable so that it is not a super rare edge case that the browser is connected to the internet, but the palava server cannot be reached and (c) the palva server can be down. Having said this, I love this idea of using the online state and it should remain the main mechanism.
The secondary mechanism should be an interval based one: If browser state is onLine and connection did not work, retry every X seconds (maybe even with increasing intervals). The current version would continuously attempt to connect

janlelis · 2020-05-23T07:44:25Z

I think going forward we should invest some energy into automatic testing. Javascript should have
decent mocking capabilities so I guess it will not get to complex.

Absolutely, especially end-to-end (browser) tests (opened an issue for that #45). Also, there is already some boilerplate test setup in the project here: https://github.com/palavatv/palava-web/blob/master/tests/e2e/specs/test.js

farao · 2020-05-23T12:54:09Z

* Original concern nr.2: The chrome devtools let you switch your browser to offline under the network tab. They seem to have removed the handy checkbox, but you can still achieve this by creating a "throttle" configuration which sends 0 kbits. I have verified that this flips the `onLine` state (on Chrome)

Ok, so we just leave it this way I guess!

* I have created an extra issue for displaying network problems to the user in #44

👌

* I still think relying soley on the `onLine` state is not enough, since (a) browser implementation varies https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine/onLine and (b) the network is generally unreliable so that it is not a super rare edge case that the browser is connected to the internet, but the palava server cannot be reached and (c) the palva server can be down. Having said this, I love this idea of using the online state and it should remain the main mechanism.

Super strange that the browsers implement this so differently, but apparently they at least have in common that offline always means offline. So I would think this is good enough, in chrome we will then simply keep on trying to reestablish the websocket connection.

* The secondary mechanism should be an interval based one: If browser state is `onLine` and connection did not work, retry every X seconds (maybe even with increasing intervals). The current version would continuously attempt to connect

Yes, this makes sense!

janlelis · 2020-05-24T15:08:58Z

Looks good! Some smallish things before we can finally merge:

Please rebase to latest master
Typo repeatet -> repeated
Could you move the actual timeout value to config.js?

When the websocket connection failed after having been established before, the complete rtc session is tried to reinitialize.

janlelis · 2020-05-24T18:34:54Z

🎉 --> staging

farao force-pushed the reconnect branch from 85d9f94 to d561e19 Compare May 21, 2020 13:10

janlelis reviewed May 22, 2020

View reviewed changes

janlelis force-pushed the reconnect branch from 91478b0 to 3dd348c Compare May 22, 2020 14:02

farao and others added 6 commits May 24, 2020 18:45

Reconnect on lost websocket connection

a83a879

When the websocket connection failed after having been established before, the complete rtc session is tried to reinitialize.

Fix reconnect issues

02328fd

JavaScript fixes / typos

cb8a2c2

Fix race condition when reconnecting

dd77f80

Add a timeout of 1s on repeatet reconnects

11cd96a

Small reconnect improvements

bb5ed7e

farao force-pushed the reconnect branch from f938ee3 to bb5ed7e Compare May 24, 2020 17:14

janlelis merged commit f9a64e6 into master May 24, 2020

janlelis deleted the reconnect branch June 1, 2020 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconnect on lost websocket connection #39

Reconnect on lost websocket connection #39

farao commented May 21, 2020

farao commented May 21, 2020

janlelis May 22, 2020 •

edited

Loading

erikzenker May 22, 2020

janlelis May 22, 2020 •

edited

Loading

janlelis May 22, 2020

janlelis May 22, 2020

janlelis May 22, 2020

farao May 22, 2020

janlelis May 22, 2020

janlelis commented May 22, 2020 •

edited

Loading

janlelis commented May 22, 2020

janlelis May 22, 2020

janlelis May 22, 2020

erikzenker May 22, 2020

farao May 22, 2020

janlelis May 22, 2020

farao commented May 22, 2020

janlelis commented May 22, 2020

janlelis commented May 22, 2020

farao commented May 22, 2020

farao commented May 22, 2020

farao commented May 22, 2020

farao commented May 22, 2020

erikzenker commented May 23, 2020 •

edited

Loading

janlelis commented May 23, 2020

janlelis commented May 23, 2020 •

edited

Loading

farao commented May 23, 2020

janlelis commented May 24, 2020 •

edited

Loading

janlelis commented May 24, 2020

	signalingConnectedBefore = false,
	signalingConnectedBefore: false,

Reconnect on lost websocket connection #39

Reconnect on lost websocket connection #39

Conversation

farao commented May 21, 2020

farao commented May 21, 2020

janlelis May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janlelis May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janlelis commented May 22, 2020 • edited Loading

janlelis commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farao commented May 22, 2020

janlelis commented May 22, 2020

janlelis commented May 22, 2020

farao commented May 22, 2020

farao commented May 22, 2020

farao commented May 22, 2020

farao commented May 22, 2020

erikzenker commented May 23, 2020 • edited Loading

janlelis commented May 23, 2020

janlelis commented May 23, 2020 • edited Loading

farao commented May 23, 2020

janlelis commented May 24, 2020 • edited Loading

janlelis commented May 24, 2020

janlelis May 22, 2020 •

edited

Loading

janlelis May 22, 2020 •

edited

Loading

janlelis commented May 22, 2020 •

edited

Loading

erikzenker commented May 23, 2020 •

edited

Loading

janlelis commented May 23, 2020 •

edited

Loading

janlelis commented May 24, 2020 •

edited

Loading