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

Added alert when websocket closes and trying to trigger it faster #2131

Merged
merged 7 commits into from
Feb 8, 2020

Conversation

foxman69
Copy link
Contributor

The webpage does not recognize websocket disconnections, this can cause major problems when still viewing the webpage while it happens (Any action like turning on the switch will seem working on the webpage but actually nothing happens)

I've added an alert when websocket.onclose happens and to make it trigger faster (if no websocket action happens it can take forever) I've added also a ping pong function to make some noise on the websocket.

@foxman69 foxman69 requested a review from mcspr January 30, 2020 21:30
code/espurna/ws.ino Outdated Show resolved Hide resolved
code/html/custom.js Outdated Show resolved Hide resolved
code/html/custom.js Outdated Show resolved Hide resolved
code/html/custom.js Outdated Show resolved Hide resolved
@foxman69 foxman69 requested a review from mcspr February 3, 2020 19:55
@mcspr
Copy link
Collaborator

mcspr commented Feb 4, 2020

btw what happens on ota, when device reboots?
if we receiving pong in response to ping, should we use that as basis for protocol-level keepalive timeout?

@foxman69
Copy link
Contributor Author

foxman69 commented Feb 4, 2020

As far as i seen during reboot and ota it works just fine, it takes a bit of time since the websocket closed till the onclose gets triggered, the ping pong function is just to make it happen a bit faster but maybe it need to get tested more deeply to make sure it does not interrupt with anything.
I've checked it on a few devices (Sonoff and Tuya) and it didn't cause any problems.

As far as i read online, this ping pong is the basics of keepalive in websockets, if you know a better way of doing it please suggest:)

@mcspr
Copy link
Collaborator

mcspr commented Feb 5, 2020

So far I only found info for proxy use-cases, where intermediate closes the connection when we dont send anything for 55-60 seconds.

If the goal is to avoid that, sure that is fine. What I was asking about is whether we should have something like this:

  • have setInterval run ping() function
  • ping() sets setTimeout()
  • when processData receives "pong", we abort the setTimeout()
  • when we don't receive "pong", we treat the connection as closed

@foxman69
Copy link
Contributor Author

foxman69 commented Feb 5, 2020

I'm not sure that putting a timeout on pong response is a good thing to do, because there are scenarios where the communication on the websocket is slow but the websocket itself is still connected.
I'm afraid doing so can cause a lot of false positives and disrupt the regular surf experience in the webui.

When i tested it, i tried 3 scenarios

  1. Connect to the device while my phone is on wifi and then turn the wifi of (on my phone) and see what happens. (Result: the onclose get called immediately as it should and after the refresh it connects back regulary)
  2. Doing the same as 1 but moving between two different APs (Result: same as 1)
  3. Connect to the device and then plug it off the power while still viewing the webui (Result: Takes about 15 seconds in average till the onclose function gets called)

IMO the current implementation is fine because it don't cause any unneeded disruption to the user

Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

OK, we'll see then :)

otoh, only issue I can see with missing messages is webserver wsSend_P() may stumble upon the queuing issue. I will fix that some time in the future though.
edit: and in case tcp connection is slow and we see such long delays, it may do weird things it is true.

@mcspr mcspr merged commit bc1fb0b into xoseperez:dev Feb 8, 2020
@foxman69 foxman69 deleted the dev10 branch June 9, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants