search

Kitware / wslink

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

Server keeps running forever and trying to send messages to improperly disconnected clients #118

Closed eino closed 2 years ago

eino commented 2 years ago

Describe the bug

Hi, We have met this issue where in some cases the asyncio server doesn't detect a client has disconnected (probably when they disconnect improperly). This causes the wslink server to never pass through the disconnection code, and thus continue running forever.

To Reproduce

Hard to reproduce systematically. I'm still trying to find a way to product the issue with no doubt, though it's presumably:

  1. Connect a client to the wslink server
  2. Improper disconnection (e.g. loss of network)
  3. In some cases the server keeps running forever, never going through the "client {0} disconnected"
  4. If a new client connects (thus producing operations to the link), following exceptions are logged
ConnectionResetError: Cannot write to closing transport
ERROR:asyncio:Task exception was never retrieved
future: <Task finished name='Task-5179' coro=<WslinkHandler.sendWrappedMessage() done, defined at /opt/app/.venv/lib/python3.9/site-packages/wslink/backends/aiohttp/__init__.py:542> exception=ConnectionResetError('Cannot write to closing transport')>
Traceback (most recent call last):
  File "/opt/app/.venv/lib/python3.9/site-packages/wslink/backends/aiohttp/__init__.py", line 596, in sendWrappedMessage
    await ws.send_str(json_header)
  File "/opt/app/.venv/lib/python3.9/site-packages/aiohttp/web_ws.py", line 308, in send_str
    await self._writer.send(data, binary=False, compress=compress)
  File "/opt/app/.venv/lib/python3.9/site-packages/aiohttp/http_websocket.py", line 690, in send
    await self._send_frame(message, WSMsgType.TEXT, compress)
  File "/opt/app/.venv/lib/python3.9/site-packages/aiohttp/http_websocket.py", line 653, in _send_frame
    self._write(header + message)

Expected behavior / Fix

It's probably linked to issue known in wslink for a couple of years: https://github.com/aio-libs/aiohttp/issues/5301 https://github.com/aio-libs/aiohttp/issues/5212

As the case may be not fixed in aiohttp, it could be handled in wslink.

I'm thinking at the points where wslink is writing to the socket (ws.send_str in sendWrappedMessage and sendWrappedError), catch any ConnectionResetError, and in this case, close the socket (await self.onClose(client_id), del self.connections[client_id]), and if there are no more clients, schedule shutdown.

Do you think it's a good approach? If it looks sound, I can start building up a PR. I'll try to find a reliable way to produce the issue as well.

wslink version: v1.8.2

jourdain commented 2 years ago

This seems to be a good approach especially if you kind of know how to reproduce/test it to validate your patch.

Please go ahead and make a PR. Thanks!

eino commented 2 years ago

The hardest is indeed to reproduce. So far my efforts have been unsuccessful (but I stumbled on an unrelated issue and opened a PR for it). Will keep you posted.

eino commented 2 years ago

Finally got my finger on it. If an exception is raised in current_ws.prepare (and probably could happend also if one is raised in onMessage, though I couldn't produce it), the client disconnection wasn't registered. Opened a PR for it.

jourdain commented 2 years ago

:tada: This issue has been resolved in version 1.8.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket: