the client connect(nursery) API is odd

[belm0]

Here too we should use the trio start() protocol. E.g.:

await connection = nursery.start(client.connect)
...

[belm0]

I suspect there no users yet so this isn't a concern, but it's possible to make this API change in a backwards compatible way:

async def connect(self, nursery=None, *, task_status=trio.TASK_STATUS_IGNORED):
    ...

[belm0]

I found that accepting a nursery as an argument also limits the implementation unfairly. Specifically, it precludes the connect() implementation from placing a try-catch around the _reader_task(). This is something I actually need to do for #17.

[mehaase]

I don't like this API either, but I'm new to Trio and not really sure what else to do.

If the client starts its own nursery, then AFAIK the caller needs to provide a handler coroutine that we will launch as a new task in our own nursery. In this hypothetical, the client code would need to look like this:

async def client():
    c = WebSocketClient('foo.com', 80, 'resource', use_ssl=True)
    # Rename connect() to run(), b/c it doesn't resume until after the 
    # connection closes:
    await c.run(handler)

async def handler(conn):
    # Do some stuff with connection. When I finish #3, this will be able
    # to use an async context manager.
    await conn.close()

Is this something we like? Is it "Trionic" (the Trio version of Pythonic)? cc: @njsmith

[mehaase]

John pointed out that Fuyukai/asyncwebsockets doesn't take a nursery as an argument. Indeed, it doesn't use a nursery at all. It doesn't need a background task because it doesn't read network data until you call next_event():

    async def next_event(self):
        """
        Gets the next event.
        """
        with trio.open_cancel_scope() as scope:
            self._cancel_scopes.add(scope)
            try:
                while True:
                    for event in self._connection.events():
                        if isinstance(event, events.DataReceived):
                            # check if we need to buffer
                            if event.message_finished:
                                return self._wrap_data(self._gather_buffers(event))
                            else:
                                self._buffer(event)
                                break  # exit for loop
                        else:
                            return event

                    data = await self._sock.receive_some(4096)
                    self._connection.receive_bytes(data)
            finally:
                self._cancel_scopes.remove(scope)

This next_event() API feels too low level. It can return any event type: text frame, binary, ping, pong, etc., and the caller has to figure out if and how to handle each event. Compare to the trio-websocket API, where a call to get_message() is guaranteed to return str or bytes.

I also think that this will break if two different tasks call next_event() at the same time. If there are no pending events, both tasks will call self._sock.receive_some() at the same time, which will throw an exception in the second task.

[belm0]

I agree that trio-websocket has a higher level API and we want it to handle ping and hide wsproto details. So the issue is that if the user is only blocking on next_event() sometimes, then we'd miss a ping.

Regarding client usage by simultaneous tasks, I don't think it should be supported. It's straightforward to wrap a single-task websocket client to support multiplexing, and I've done exactly that with asyncwebsockets. I'm not sure what the semantics of allowing multiple listeners would be. In my case responses need to get paired with requests based on a proprietary scheme, so custom multiplexing is needed regardless.

Regarding passing in nurseries, I've got over a month of using Trio full time now and am forming an opinion that it's an anti-pattern. The usual pattern when you have an object which needs to run something in the background is to give full control to the user about what scope that is run in, most importantly of which is the ability to wrap the task in a try-catch. This error handling part, along with scoped cancellation, are Trio's main selling points, and passing in a nursery foils them.

async def do_session():
    try:
        async with open_websocket(host, port) as ws:
            # At this point we know that there was successful ws handshake with peer.
            # We can check ws.port if it was automatically assigned.
            # ping requests will be handled automatically.
            # We can call ws.send() and ws.receive().  Neither support
            # simultaneous task access, so use a lock for send() and multiplexing
            # on receive() as necessary.
            # If we're defining an asynccontextmanager, we would
            # yield ws here.
        # this point is never reached since open_websocket always raises
    except ClosedConnection:
        # pass or something based on close reason
    except SomeHandlerError:
        # ...

# do_session would normally be run in parallel with other tasks via nursery.
#
# Another use case is to wrap the run in "with trio.move_on_after()", e.g. if
# session can't complete in 30 seconds, give up.

[njsmith]

My suggestion would be to have a "low level" API where you pass in a nursery, plus a "high level" API like async with open_websocket(url): ...

The high level API makes things convenient, since most users just want to open a websocket connection and don't care about the details; the low level API makes more exotic cases possible, like when you want to have an unpredictable number of connections with unpredictable lifetimes, and there's no clear mapping between connections and tasks. There's some more discussion of this (even using websocket as the example :-)) here: https://stackoverflow.com/questions/48282841/in-trio-how-can-i-have-a-background-task-that-lives-as-long-as-my-object-does

[mehaase]

Fixed in #27 and #28.