Repository: aaugustin/websockets (python)

Issue:
Error in data transfer due to 1006
I'm writing some stress tests for a web app and I expect that no matter what clients do the server should never experience an error. However, the test occasionally fails because the below shows in the log:

```text
ERROR    websockets.protocol:protocol.py:805 Error in data transfer
Traceback (most recent call last):
  File "venv/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 750, in transfer_data
    message = await self.read_message()
  File "venv/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 819, in read_message
    frame = await self.read_data_frame(max_size=self.max_size)
  File "venv/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 919, in read_data_frame
    await self.pong(frame.data)
  File "venv/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 689, in pong
    await self.ensure_open()
  File "venv/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 726, in ensure_open
    raise self.connection_closed_exc()
websockets.exceptions.ConnectionClosedError: code = 1006 (connection closed abnormally [internal]), no reason
```

As far as I can tell the exception never passes through the application thus providing the application no opportunity to catch it.

From a cursory reading it seems it should be safe to catch this since `self.state is State.CLOSED` and any later attempts to send or receive should raise the same exception to the application. However, I am not familiar enough with the library to know if it could be a symptom of a problem elsewhere.

What do you think?

It seems to be caused by connecting a client that does not receive any of the messages sent by the server eventually triggering a ping timeout. Not sure if it happens on client side or server side since in my testing so far both run in the same process.

Pull request:
Prevent crash of data transfer task
When the read buffer isn't empty, the following scenario is possible:

* TCP connection is closed (one way or another)
* transfer_data is still processing buffered data
* it encounters a ping and attempts to send a pong

transfer_data must never crash, so ignore the error in this case.

Fix #977

Patch:
diff --git a/src/websockets/legacy/protocol.py b/src/websockets/legacy/protocol.py
index 992d678..353317a 100644
--- a/src/websockets/legacy/protocol.py
+++ b/src/websockets/legacy/protocol.py
@@ -926,14 +926,17 @@ class WebSocketCommonProtocol(asyncio.Protocol):
                     # is empty and parse_close() synthetizes a 1005 close code.
                     await self.write_close_frame(frame.data)
                 except ConnectionClosed:
-                    # It doesn't really matter if the connection was closed
-                    # before we could send back a close frame.
+                    # Connection closed before we could echo the close frame.
                     pass
                 return None
 
             elif frame.opcode == OP_PING:
                 # Answer pings.
-                await self.pong(frame.data)
+                try:
+                    await self.pong(frame.data)
+                except ConnectionClosed:
+                    # Connection closed before we could respond to the ping.
+                    pass
 
             elif frame.opcode == OP_PONG:
                 if frame.data in self.pings:


Test patch:
diff --git a/tests/legacy/test_protocol.py b/tests/legacy/test_protocol.py
index 6f6e6f6..c1948c8 100644
--- a/tests/legacy/test_protocol.py
+++ b/tests/legacy/test_protocol.py
@@ -877,6 +877,16 @@ class CommonTests:
         self.run_loop_once()
         self.assertOneFrameSent(True, OP_PONG, b"test")
 
+    def test_answer_ping_does_not_crash_if_connection_closed(self):
+        self.make_drain_slow()
+        # Drop the connection right after receiving a ping frame,
+        # which prevents responding wwith a pong frame properly.
+        self.receive_frame(Frame(True, OP_PING, b"test"))
+        self.receive_eof()
+
+        with self.assertNoLogs():
+            self.loop.run_until_complete(self.protocol.close())
+
     def test_ignore_pong(self):
         self.receive_frame(Frame(True, OP_PONG, b"test"))
         self.run_loop_once()