Repository navigation
Conversation
ConnectASGIApplication calls asyncio.sleep(0) after each request message and starts its disconnect watcher with asyncio.create_task, so under a trio server every request except a Connect unary RPC fails. Under trio, streaming and gRPC requests now run a separate path built on anyio, which is an optional extra, connectrpc[trio], and is imported only there. On asyncio the same statements run in the same order as before; sending a message and ending the response move into a class that both paths use. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
| @@ -63,6 +70,7 @@ dev = [ | |||
| "pyvoy==1.1.0", | |||
| "ruff==0.16.8", | |||
| "tombi==1.5.5", | |||
| "trio==0.34.0", | |||
There was a problem hiding this comment.
mostly a drive-by review, but to actually test the minimums here I don't think we can pin the versions in this group?
I think we could swap those below with just a "connectrpc[trio]" dep, which ought to use the declared range from that group?
There was a problem hiding this comment.
[claude code] You're right. With the pins, the lowest-direct jobs resolve anyio 4.14.1 and trio 0.34.0, so the minimums that the extra declares were never tested.
The new commit replaces both pins with connectrpc[trio] in the dev group. Under lowest-direct that resolves anyio 4.10.0 and trio 0.32.0, and the suite passes there on Python 3.10 and 3.14 (384 passed on each). The default resolution is unchanged: the lock still holds 4.14.1 and 0.34.0.
The same applies to typing_extensions: the dev group pins ==4.16.0, so the >=4.13.0 minimum resolves to 4.16.0 under lowest-direct. I left that alone in this PR.
There was a problem hiding this comment.
[claude code] Correction to my last paragraph: #350 already removed the typing_extensions pin on main. I measured on this branch, which predates it.
The dev group pinned anyio and trio, so the lowest-direct jobs resolved the pinned versions and never the minimums that the extra declares. The group now depends on connectrpc[trio], which resolves anyio 4.10.0 and trio 0.32.0 under lowest-direct and leaves the locked versions as they were. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
|
Thanks @iainmcgin really glad to support trio. Going to take a bit more time to fully mentally process this PR, sorry for the delay |
…nto server-trio
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
left a comment
There was a problem hiding this comment.
Thanks @iainmcgin - I have applied some big changes. Trying to minimize impact on existing asyncio is appreciated, but I think in the end it was hard to follow and feel confidence in the logic, especially with the behavior differences between the two paths. I went ahead and stuck to a single flow, with the small abstraction needed for the loops. I also just use trio directly since using anyio, only to support trio, means we can just use trio. It avoids the need for the extra. Can you confirm the new code looks fine, especially the trio touchpoints?
The flakes with conformance tests on hypercorn with trio matches our experience and is why we unfortunately don't run conformance tests with it. I added a pyvoy-based one instead which seems to be reliable (@iainmcgin hint hint ;-). I might not have run the full suite if not for @stefanvanburen's early Christmas present of way faster conformance tests.
To summarize the changes to asyncio, they are all related to cancellation, either from client or server. Now, stream requests propagate cancellation to the app server in addition to sending the client the canceled error as before, unary requests now send the client the canceled error as well as propagating it to app server as before (notice the two had opposite behavior), request streams get CancelledError instead of ConnectError, and any error in i.e. aclose when cleaning up doesn't overwrite the real error.
In the future I (or someone) will follow up to add a CI step that runs tests with trio not installed to confirm we don't accidentally import it.
| yield Hat(size=size.inches, color="black", name="echo") | ||
|
|
||
|
|
||
| class Wrapper: |
There was a problem hiding this comment.
This replaces the previous ad-hoc exchange tests which were quite giant and harder to reason about than this wrapper to me
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
|
Hi @iainmcgin - would you be able to take another look at the current form for the PR? We can try to review / merge it anyways but I think none of the maintainers here have much experience with trio. Thanks! |
…nto server-trio
ConnectASGIApplicationcallsasyncio.sleep(0)after each request message and starts its disconnect watcher withasyncio.create_task. Under a trio server, every request type except for a Connect unary RPC therefore fails withtrio.run received unrecognized yield message None.Under trio, streaming and gRPC requests now run a separate path built on anyio. The path is taken whenever no asyncio loop is running; trio is the only such loop that anyio supports. anyio is an optional extra,
connectrpc[trio](anyio>=4.10,trio>=0.32), and is imported only on that path; without it a request under trio raises anImportErrorthat names the extra. The dev group depends onconnectrpc[trio], so the CI jobs that resolve the lowest allowed versions test those minimums. On asyncio the same statements run in the same order as before. A single anyio path was not used because it would put an anyio task group under every server stream on asyncio.On the trio path a cancellation propagates to the scope that asked for it, where asyncio converts it to
ConnectError(CANCELED).on_endis toldCANCELED, closing the handler generator and running theon_endhooks are each shielded for up to 1 second, and nothing more is sent. A cancellation that arrives together with another error, such as a failure in the handler's cleanup, is handled the same way, and the error still propagates. The trio path also has two fixes that the asyncio path does not have yet: an error fromreceive()in the disconnect watcher ends the watcher only, and the request stream is closed when the request ends.Connect unary RPCs still run the existing code under trio, so a cancelled one calls
on_endwith no error, as on asyncio, and under trio the hook stops at its first await.Under hypercorn's trio worker, conformance over HTTP/1.1 goes from 1924 failures of 2716 to between 0 and 2 per run. hypercorn's asyncio worker fails 0 to 1 per run with the current code; in both cases the failures are connections that hypercorn closes early. hypercorn 0.18.0 does not complete the conformance suite's HTTP/2 cases on either worker, so HTTP/2 under trio is not measured.
The async client is unchanged; running it under trio needs a separate change and a pyqwest release with trio support.