Documentation remarks

[paulmelis]

Hi, just started trying out wsproto (nice concept) and I find the available documentation to be a bit suboptimal:

• The docs at https://wsproto.readthedocs.io/en/latest/basic-usage.html seem pretty out of date. E.g. under "Connecting to a WebSocket server" the event handling uses TextReceived and BinaryReceived, but those are now named TextMessage and BytesMessage?

• I built the docs locally (make html), but WSConnection shows no member information. Connection does have more interesting stuff documented, but that isn't listed in the generated docs (nor is it the end-user class to use).

• As WSConnection.events isn't document I looked at

  wsproto/example/synchronous_server.py

  Line 76 in a7abcc5

  except StopIteration:

  and it says there that WSConnection.events() raises StopIteration when the client disconnects unexpectedly. That can't be right in the general case? That comment seems misleading and is probably only valid for that specific example.

  Edit: ah, https://github.com/python-hyper/wsproto/blob/master/example/synchronous_client.py gives more clues:

  events is a generator that yields websocket event objects. [...] It will raise StopIteration when it runs out of events (i.e. needs more network data)

• Under Getting Started -> Connections (in my local doc build) it says (emphasis mine)

  Note
  If the connection drops, a standard Python socket.recv() will return zero.

  That should read "returns zero bytes"

[paulmelis]

The docs for class wsproto.events.CloseConnection read

wsproto automatically emits a CLOSE frame when it receives one, to complete the close-handshake.

This reads to me that the close frame still needs to be sent using the network layer used by the user. However, with the code below, which is straight from the synchronous_server.py example I get an exception from the wsconn.send() call. This is in the case the client side simply disconnects the connection (the relevant event output on the server side is code=1006/ABNORMAL_CLOSURE reason=None):

                elif isinstance(event, CloseConnection):
                    # Client connection closed
                    logger.info('Connection closed on %s: code=%d/%s reason=%s' % \
                        (sock, event.code.value, event.code.name, event.reason))
                    out_data = wsconn.send(event.response())

Traceback (most recent call last):
  File "./server.py", line 102, in <module>
    out_data = wsconn.send(event.response())
  File "/usr/lib/python3.7/site-packages/wsproto/__init__.py", line 38, in send
    data += self.connection.send(event)
  File "/usr/lib/python3.7/site-packages/wsproto/connection.py", line 81, in send
    "Connection cannot be closed in state %s" % self.state
wsproto.utilities.LocalProtocolError: Connection cannot be closed in state ConnectionState.CLOSED

So in this case, given that the connection appears to be already closed is there a need to send the response? And how to detect that situation?

[paulmelis]

Another thing to put in the docs: I couldn't figure out why there's receive events for both text and binary messages, but sending only has a single Message class. Only when looking into frame_protocol.py one learns that bytes, bytearray and memoryview objects are flagged as binary, with unicode data flagged as text.

[pgjones]

Hi @paulmelis could you open a pull request with these fixes in? I'd be happy to review and merge.

So in this case, given that the connection appears to be already closed is there a need to send the response? And how to detect that situation?

To detect this I check that connection.state == ConnectionState.REMOTE_CLOSING before sending the close. I think it makes sense for wsproto to complain loudly in this case as it has been asked to do something it cannot (send a close frame).

[mehaase]

I am going to work on updating https://github.com/HyperionGray/trio-websocket to wsproto=0.14.0, and while I do that I will update the docs as I go and submit a PR when I'm finished.

[mehaase]

@paulmelis I am incorporating your feedback.

and it says there that WSConnection.events() raises StopIteration when the client disconnects

Yes, the synchronous example is very awkward in this aspect. Almost all wsproto consumers will want to use for event in ws.events(): instead of next(event). I will fiddle with making that example more readable.

[Kriechi]

@mehaase @paulmelis I think this issue can be now closed, since we merged #119, right?

[paulmelis]

Looks good to me

[mehaase]

One more request for @pgjones or @Kriechi: The ReadTheDocs build hasn't run in about 4 months, I think because it is configured as a "GitHub service" and those are deprecated and don't run anymore. We should delete the RTD service and add a webhook instead[1]. I would offer to do this myself but I don't have admin rights to the RTD project.

[1] We also have a Travis CI service, and if I understand the deprecation process correctly, this has also superseded by webhooks and can be removed.

[Kriechi]

Thanks for pointing this out!
I fixed the RTD integration.
We already use a webhook for TravisCI - I will remove the old Github Service as it is now useless.