python-trio / trio-websocket

WebSocket client and server implementation for Python Trio
MIT License
70 stars 26 forks source link

reader task leaks ClosedResourceError #105

Closed belm0 closed 5 years ago

belm0 commented 5 years ago

I'm able to trigger this exception by attempting to load the websocket URL in my Chrome browser.

Traceback (most recent call last):
  File "/.../lib/python3.7/site-packages/trio_websocket/_impl.py", line 936, in _reader_task
    data = await self._stream.receive_some(RECEIVE_BYTES)
  File "/.../lib/python3.7/site-packages/trio/_highlevel_socket.py", line 139, in receive_some
    return await self.socket.recv(max_bytes)
  File "/usr/lib/python3.7/contextlib.py", line 130, in __exit__
    self.gen.throw(type, value, traceback)
  File "/.../lib/python3.7/site-packages/trio/_highlevel_socket.py", line 29, in _translate_socket_errors_to_stream_errors
    ) from None
trio.ClosedResourceError: this socket was already closed
mehaase commented 5 years ago

Apologies for not replying yet. I will look into this next week.

mehaase commented 5 years ago

Sorry for ghosting on this ticket! I am unable to reproduce it:

  1. Run python examples/server.py 8000
  2. Open examples/client.html in a browser tab
  3. Send some messages

I tried this in Firefox and Chromium.

The line in question has try/except around it:

            try:
                data = await self._stream.receive_some(RECEIVE_BYTES)
            except (trio.BrokenResourceError, trio.ClosedResourceError):
                await self._abort_web_socket()
                break

Is it possible this was fixed in some other issue?

belm0 commented 5 years ago

to reproduce, simply attempt to load localhost:8000 in the browser

in 0.6.0, the trace is as I posted previously

in master, new trace:

Traceback (most recent call last):
  File "examples/server.py", line 68, in <module>
    trio.run(main, parse_args())
  File "/usr/local/lib/python3.7/site-packages/trio/_core/_run.py", line 1444, in run
    raise runner.main_task_outcome.error
  File "examples/server.py", line 49, in main
    await serve_websocket(handler, host, args.port, ssl_context)
  File "/Users/john/dev/trio/trio-websocket/trio_websocket/_impl.py", line 336, in serve_websocket
    await server.run(task_status=task_status)
  File "/Users/john/dev/trio/trio-websocket/trio_websocket/_impl.py", line 1285, in run
    await trio.sleep_forever()
  File "/usr/local/lib/python3.7/site-packages/trio/_core/_run.py", line 506, in __aexit__
    raise combined_error_from_nursery
  File "/usr/local/lib/python3.7/site-packages/trio/_highlevel_serve_listeners.py", line 129, in serve_listeners
    task_status.started(listeners)
  File "/usr/local/lib/python3.7/site-packages/trio/_core/_run.py", line 506, in __aexit__
    raise combined_error_from_nursery
  File "/usr/local/lib/python3.7/site-packages/trio/_highlevel_serve_listeners.py", line 27, in _run_handler
    await handler(stream)
  File "/Users/john/dev/trio/trio-websocket/trio_websocket/_impl.py", line 1313, in _handle_connection
    await connection.aclose()
  File "/usr/local/lib/python3.7/site-packages/trio/_core/_run.py", line 506, in __aexit__
    raise combined_error_from_nursery
  File "/Users/john/dev/trio/trio-websocket/trio_websocket/_impl.py", line 1119, in _reader_task
    self._wsproto.receive_data(data)
  File "/usr/local/lib/python3.7/site-packages/wsproto/__init__.py", line 43, in receive_data
    self.handshake.receive_data(data)
  File "/usr/local/lib/python3.7/site-packages/wsproto/handshake.py", line 155, in receive_data
    self._events.append(self._process_connection_request(event))
  File "/usr/local/lib/python3.7/site-packages/wsproto/handshake.py", line 201, in _process_connection_request
    "Missing header, 'Connection: Upgrade'", event_hint=RejectConnection()
wsproto.utilities.RemoteProtocolError: Missing header, 'Connection: Upgrade'
mehaase commented 5 years ago

Ahhh gotcha. This is really a test of connecting to a peer that isn't speaking WebSocket protocol. More broadly, it's a test of handling protocol errors.

I expect that this can be resolved by catching RemoteProtocolError in a few appropriate places and closing the connection.

mehaase commented 5 years ago

Here is a command line that will raise a similar exception in the reader task:

$ echo -en "GET / HTTP/1.1\r\n\r\n" | nc -v localhost 8000
Connection to localhost 8000 port [tcp/*] succeeded!