search

Kitware / wslink

Python/JavaScript library for communicating over WebSocket
https://kitware.github.io/wslink/
BSD 3-Clause "New" or "Revised" License
83 stars 27 forks source link

enable heartbeat to shutdown clients on network interrupt #79

Closed knopkem closed 2 years ago

knopkem commented 2 years ago

When using vtk.js remoteView together with wslink and vtkWeb we run into websocket timeouts whenever a user doesn't interact for more than 60s. While not exactly sure who is responsible for interrupting the connection (apache, wslink) another side-effect of those interrupted sessions is that wslink does not shutdown these correctly. Looking at the code wslink async awaits on new messages but in case of (forced) connection drop it basically hangs forever waiting for messages on a dead client. While it would be possible to add ws ping to our client code it would only partially solve the issue of leaked sessions, it would keep connections alive but any network interruption would still lead to the issue described. So the imho better approach would be to use the built in 'heartbeat' functionality of aiohttp which by default is deactivated. The PR uses a (random) fixed number of 30s, which I guess could also come from a config but it wouldn't be needed as long as the heartbeat interval is below 60s.

jourdain commented 2 years ago

While this is an elegant solution, the issues you are facing should not be happening. I still think your fix is great and should go in, but I want to mention how the 2 problem you are facing should work without that change.

1) WS disconnection: Usually this is related to a proxy (apache/nginx) configuration that cut the client/server connection.

2) Connection cleanup: wslink as a built-in reaper that will stop the server process if we don't have any remaining ws client connected after a given timeout (default: 300s). I'm wondering if your apache is keeping the ws connection even after the client got disconnected or if you did not reached the default timeout of 300s.

knopkem commented 2 years ago

That are good hints, indeed apache and wstunnel seem to come with timeouts, will play with this. The reaper definitely didn't kick in for us, which could be as you said that apache keeps connections alive.

jourdain commented 2 years ago

For the timeout did you reduced the value as 300s is pretty long?

jourdain commented 2 years ago

Sorry I just realized the commit message is not following semantic-release. Can you rework the message to have fix(aiohttp): register 30s heartbeat on ws

knopkem commented 2 years ago

ok, done

jourdain commented 2 years ago

:tada: This PR is included in version 1.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: