send_message() race condition

[belm0]

This race condition causes send_message() to return even though message is not sent, and _writer_task() to block even though there is a pending message from wsproto.

1. send_message() "a" hands data to wsproto, sets _data_pending, blocks on _data_sent
2. wsproto queues data "a" for bytes_to_send
3. _writer_task() receives _data_pending event and calls bytes_to_send(), getting data "a"
4. send_message() "b" hands data to wsproto, sets _data_pending (already set), blocks on _data_sent
5. wsproto queues data "b" for bytes_to_send
6. _writer_task() sends message "a" to stream, clears _data_pending, sets _data_sent
7. send_message() "a" exits
8. send_message() "b" exits even though message has not been sent
9. _writer_task() blocks on _data_pending, even though wsproto has message "b" pending

send_message():
https://github.com/HyperionGray/trio-websocket/blob/b787bf1a8a026ef1d9ca995d044bc97d42e7f727/trio_websocket/__init__.py#L192-L195

_writer_task():
https://github.com/HyperionGray/trio-websocket/blob/b787bf1a8a026ef1d9ca995d044bc97d42e7f727/trio_websocket/__init__.py#L299-L306

As part of this fix, it would be great to set up pytest and have an explicit test for this case.

[belm0]

this needs some review of the API and design

my quick fix was to loop on bytes_to_send():

            while True:
                data = self._wsproto.bytes_to_send()
                if len(data) > 0:
                    await self._stream.send_all(data)
                else:
                    break

However, if there is a high enough rate of send_message() calls, we can never exit the loop and clear _data_sent.

Perhaps send_message() could be left synchronous (i.e. don't use _data_sent), giving up on back pressure. Otherwise, given that wsproto can aggregate messages, I'm not sure how to emit a data sent signal at send_message() granularity. (Granted, I don't know the wsproto API.)

[njsmith]

Whoops, I never did file this here, did I? It's mentioned here: python-trio/trio#637

I think the fix is to get rid of the writer task entirely, which would simplify things for other reasons too: #3

[mehaase]

An earlier version of this library had no writer task, and I thought I was simplifying things by adding one! It should be easy to revert.