Skip to content

Add trio implementation #1628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add trio implementation #1628

wants to merge 3 commits into from

Conversation

aaugustin
Copy link
Member

No description provided.

@agronholm
Copy link

So, while working on my own version, I started wondering if the Assembler class was actually something that should be left as an implementation detail rather than part of the API? I didn't really understand why it was a separate thing from the connection classes.

Another big change I had to make was the introduction of pytest as a test dependency, as testing with plain unittest would require a custom harness which I feel is pointless when AnyIO's pytest plugin contains all the necessary machinery to support async tests.

@aaugustin aaugustin marked this pull request as draft May 22, 2025 17:06
@aaugustin
Copy link
Member Author

@agronholm The Assembler is an implementation detail: it isn't documented in the API. It could live in the connection module. It happens to be an independent piece with medium complexity, which justifies giving it its own module. That keeps the connection module simpler. This is a personal preference in how I organize the code with no user-visible consequences.

@aaugustin
Copy link
Member Author

aaugustin commented May 22, 2025

websockets got its own async test harness before there was one in the stdlib. The legacy implementation still uses it. I switched to unittest.IsolatedAsyncioTestCase in the new asyncio implementation.

I chose to stick to vanilla choices everywhere. websockets has no required dependencies, even for development. I am clearly biased by:

  • my background in Django, which didn't have any dependencies for an extremely long time;
  • my desire to avoid debugging dependencies :-) I got my share of debugging with asyncio ;-)

@agronholm
Copy link

OK, so is the introduction of pytest as a test dependency going to be a problem? I mean, I have plenty of experience in the best practices front so I can do all the heavy lifting in that regard (packaging, CI etc.).

@agronholm
Copy link

my background in Django, which didn't have any dependencies for an extremely long time;

Incidentally, this is the primary reason why I steer clear of Django.

@aaugustin
Copy link
Member Author

Mastery of pytest isn't the problem. I used it in a professional context before. I know how to configure it. Actually, I spent a lot more time deep in the guts of the fixtures system than I ever wanted...

It's simply not suitable considering my goals and constraints for this project. Keeping complexity and the amount of third-party code that I may have to understand or debug to the bare minimum ranks very high on the list.

You're welcome to use it as a test runner though.

@agronholm
Copy link

Sure, it's your project, your rules. But I've just been thinking how much of the maintenance burden you could shed by relying on external projects rather than inventing everything yourself. Just my two cents.

@aaugustin aaugustin force-pushed the trio branch 4 times, most recently from 62e304c to 09b9947 Compare May 22, 2025 20:37
@agronholm
Copy link

So are you going to continue with your work of adding a Trio-specific back-end? The pushes seem to indicate as much. Would you not rather let me finish my work on the AnyIO back-end?

@aaugustin aaugustin force-pushed the trio branch 2 times, most recently from 1051bfc to 30fdd95 Compare May 22, 2025 21:14
@aaugustin
Copy link
Member Author

Short, directional answer because it's 11:30pm here:

  • I don't see myself reviewing a PR adding an entire implementation. There's a lot of details to get right; I find it quite hard to get them right myself; checking that someone else got them right feels impossible to me. We didn't talk before you started this work and I don't have a plan to integrate it. Of course you can publish websockets-anyio or fork.
  • Since the asyncio backend is already there and there's no good reason to remove it, at this point, it's more straightforward to add just trio than to add anyio + trio. I don't see a benefit in adding anyio and I see a downside - more moving parts going foward.

@agronholm
Copy link

Since the asyncio backend is already there and there's no good reason to remove it, at this point, it's more straightforward to add just trio than to add anyio + trio. I don't see a benefit in adding anyio and I see a downside - more moving parts going foward.

I don't understand. Why even suggest adding anyio AND trio back-ends when the former caters for both Trio and asyncio users? The biggest reason would be fewer parts to maintain, not more!

@aaugustin
Copy link
Member Author

Indeed, if I finish the trio implementation within a reasonable timeframe, there's little point in an anyio implementation.

As all things open-source, completion is highly dependent on everything else in my life; it isn't done until it's done :-)


I see your frustration that I'm not taking advantage of anyio when you built it exactly for people like me. It's fine to disagree. However, I tried to understand where our views diverge to make sure I'm not missing something. Going straight to the point:

1/ I don't have a significant cost of maintenance for code that I control. I invest upfront in getting it right. The cost of maintenance is driven by changes in code outside of my control, whether intentional or not. asyncio is outside of my control and has been painful in this regard. trio and anyio are outside of my control; they may be less painful as asyncio but still it's more code outside of my control. (Of course anyio is under your control so you have the opposite perspective here.)

2/ This is a hobby project for me. I spend time writing code in websockets because I like coding and I don't get the opportunity to write meaningful code in my current job. Some people do crosswords; I do open-source. It's time well spent, even if others have written similar code before. I wrote an unconventional HTTP parser; it does exactly what I want and never caused problems; I found that interesting. Conversely, I get less joy of creation from diagnosing what changed in someone else's code and suddenly caused websockets to misbehave or from reviewing someone else's contribution so I'm not looking to spend more time on these activities.

@agronholm
Copy link

Indeed, if I finish the trio implementation within a reasonable timeframe, there's little point in an anyio implementation.

As all things open-source, completion is highly dependent on everything else in my life; it isn't done until it's done :-)

I see your frustration that I'm not taking advantage of anyio when you built it exactly for people like me. It's fine to disagree. However, I tried to understand where our views diverge to make sure I'm not missing something. Going straight to the point:

1/ I don't have a significant cost of maintenance for code that I control. I invest upfront in getting it right. The cost of maintenance is driven by changes in code outside of my control, whether intentional or not. asyncio is outside of my control and has been painful in this regard. trio and anyio are outside of my control; they may be less painful as asyncio but still it's more code outside of my control. (Of course anyio is under your control so you have the opposite perspective here.)

Funnily enough, you're making my point for me. If you proceed with your current plan of adding a Trio backend, you will end up with two async back-ends, either one of which may break when the upstream makes changes. But if there was only the AnyIO back-end, then websockets would "just work" on both asyncio and Trio (and any other event loop it will support in the future; there have been talks of that), and you'd only have to worry about breakages occurring in AnyIO (which are quite rare these days), as it will shield you against most upstream changes.

@aaugustin
Copy link
Member Author

aaugustin commented May 23, 2025

Funnily enough, you're making my point for me.

This sounds like you're talking down to me. Please, let's have a constructive conversation or leave it there.

I'm clear on where I disagree with you. I'm trying — and, for now, failing — to land this conversation on a respectful agreement to disagree.

You're saying that using anyio will result in a lower maintenance cost. I don't buy it. I don't think there's a clear cut winner between:

  1. Depending directly on asyncio and trio
  • allows providing native APIs for both -> better UX IMO
  • vulnerable to changes in asyncio and trio but debugging involves only one layer (debugging usually means reading the source)
  • when appropriate, I can push back with "it's not me; it's asyncio / it's trio" as the user is actively choosing asyncio or trio
  1. Depending directly on anyio and indirectly on asyncio and trio
  • allows providing a unified API for both -> some might prefer this
  • users are exposed structured concurrency (when many are just learning asyncio) -> leads to questions in the issue tracker
  • vulnerable to changes in anyio and asyncio and trio, although anyio might shield some of them, but debugging involves two layers

(This isn't a full decision matrix; that's not my point; my point is that you have pros and cons on both sides.)

It's similar building a Swift and Kotlin apps or a React Native app: both options are valid, depending on your preferences and constraints.


Based on how this conversation went, it doesn't look like there's a path for smooth collaboration. Therefore, I won't consider a PR adding an anyio backend to websockets.

@agronholm
Copy link

agronholm commented May 23, 2025

Alright, thank you for the elaborate response. At least I tried. And sorry if I sounded like I was talking down to you – it was never my intention.

@aaugustin
Copy link
Member Author

aaugustin commented May 23, 2025

No worries. Feels like we fell into a typical trap of open-source collaboration :-(

If you have a WIP version of your anyio implementation somewhere, I'm genuinely interested in reading it because it will help me get a better grasp of anyio and structured concurrency (which I have no experience with).

@agronholm
Copy link

If you have a WIP version of your anyio implementation somewhere, I'm genuinely interested in reading it because it will help me get a better grasp of anyio and structured concurrency (which I have no experience with).

Here it is: https://github.com/agronholm/websockets/tree/anyio

@aaugustin aaugustin force-pushed the trio branch 4 times, most recently from 960016d to 894c1ac Compare May 26, 2025 19:14
@Zac-HD
Copy link

Zac-HD commented May 27, 2025

👋 hey all - I really appreciate all the work you've both been doing on this, and thought it might be helpful to share a user story / why I'm excited.

  • Since the asyncio backend is already there and there's no good reason to remove it, at this point, it's more straightforward to add just trio than to add anyio + trio. I don't see a benefit in adding anyio and I see a downside - more moving parts going foward.

As a Trio user, the support for my app as a direct user is equally good whether we add websockets.trio or websockets.anyio - but the latter makes it easy for intermediate libraries to support Trio without adding an additional implementation. Low-level libraries (e.g. hypercorn, maybe websockets?) often take the code duplication, but further up the stack it's a lot easier to argue that "you can just use anyio and get support for both, for free" than to ask for a separate code path. It's that widespread interoperability between libraries that really excites me.

@aaugustin
Copy link
Member Author

That's a fair point. I didn't have it in mind because I haven't come across "intermediate libraries" having this use case until now. Do you have examples? I'd like to check them out.

To the best of my current knowledge, the main use case for embedding websockets is application servers (usually providing HTTP + WebSocket). The Sans-I/O layer is best for them. The other use case is projects like HomeAssistant; they're "end users" who can pick one I/O framework and stick to it. My best guess for "intermediate libraries" would be libs implementing a protocol running on top of WebSocket. I've come across some industrial protocols like this before; I don't remember the details.

Right now I'm in the process of learning structured concurrency and the constraints it puts on API design, as well as a bunch of subtle differences between asyncio and trio. I think that's part of the learning journey anyway if anyio ends up being somewhere in the final picture!

@aaugustin
Copy link
Member Author

BTW I have a working client now, with some truly outstanding code inside, but it works — like I said I'm learning about structured concurrency ;-)

    # async with connect(...) as ...: ...

    async def __aenter__(self) -> ClientConnection:
        self.nursery_manager = trio.open_nursery()
        self.nursery = await self.nursery_manager.__aenter__()
        return await self

    async def __aexit__(
        self,
        exc_type: type[BaseException] | None,
        exc_value: BaseException | None,
        traceback: TracebackType | None,
    ) -> None:
        try:
            await self.connection.close()
        finally:
            await self.nursery_manager.__aexit__(exc_type, exc_value, traceback)

@aaugustin
Copy link
Member Author

One one hand, this is clearly voiding the warranty :-) On the other hand, I don't see a better way to provide the API that I want to provide (async with connect(...) as connection: ... + async for connection in connect(): ...). Surely you have more experience; advice or warnings welcome!

@agronholm
Copy link

Yeah, the await connect(...) method is a no-go with structured concurrency, as the background task for keepalives requires management.

@aaugustin
Copy link
Member Author

It can work if the user takes responsibility:

async with trio.open_nursery() as nursery:
    connection = await connect(..., nursery=nursery)

Currently I'm +0 on including that in the API but open to the idea of killing it if you tell me that it's a clear no-go in the structured concurrency world.

@Zac-HD
Copy link

Zac-HD commented May 28, 2025

More idiomatic, if I understand your intended semantics:

async with trio.open_nursery() as nursery:
    connection = await nursery.start(connect, ...)
    ...  # do stuff here
    nursery.cancel_scope.cancel(reason="no automatic way for the connection to close?")

nursery.start is conceptually pretty similar to a context manager - task_status.started(x) ~ yield x. My main concern looking at either is that the connection seems likely to stay alive indefinitely by default; using a context-manager-per-connection which has an internal nursery would be the usual pattern for that.

@aaugustin
Copy link
Member Author

aaugustin commented May 28, 2025

using a context-manager-per-connection which has an internal nursery would be the usual pattern for that.

Yes, that's the recommend way to connect. The code snippet that I shared above implements it (in a class that also implements __aiter__ for automatic reconnection; I end up with this complexity because I overload a single API with multiple usage patterns).

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

Thank you for sharing the idiomatic version.

As you point out, the problem is to determine when to close connections. websockets always encouraged using a context manager, as this is obviously the best way. However, as a consequence of starting as an asyncio library (a tulip library, even!) and mirroring asyncio's API design, just starting a connection is allowed.

The more I think about it, the more I'm leaning against providing that option. I went for a trio-specific implementation because I wanted to feel like a trio library. It looks like trio wouldn't provide that functionality. There's quite a few things that you can do with asyncio (e.g., futures) and don't exist in trio; this appears to be one of them.

So I shouldn't include it.

Then maybe I can get rid of my sketchy cutting of a context manager in two halves (which has a bug, BTW, __aexit__ isn't called if __aenter__ crashes at the wrong place).

@agronholm
Copy link

Then maybe I can get rid of my sketchy cutting of a context manager in two halves (which has a bug, BTW, aexit isn't called if aenter crashes at the wrong place).

This is a common mistake which I've also made a few times which is why I added context manager mix-ins to AnyIO (to be released with v4.10).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants