kyuupichan / aiorpcX

Generic async RPC implementation, including JSON-RPC
MIT License
27 stars 23 forks source link

add tests: timeout_after does not compose with asyncio primitives #48

Closed SomberNight closed 7 months ago

SomberNight commented 7 months ago

These tests fail on older python (3.8, 3.9, 3.10), but pass on newer python (3.11, 3.12). The cause seems to be that TaskTimeout subclasses CancelledError.

With following patch, all tests pass even on older python: (note that TimeoutCancellationError also subclasses CancelledError though)

diff --git a/aiorpcx/curio.py b/aiorpcx/curio.py
index 7cb7569..7caf7f8 100755
--- a/aiorpcx/curio.py
+++ b/aiorpcx/curio.py
@@ -304,7 +304,7 @@ class TaskGroup:
         await self.join()

-class TaskTimeout(CancelledError):
+class TaskTimeout(Exception):

     def __init__(self, secs, *args):
         super().__init__(*args)

I could not find the exact change in cpython 3.11 that makes this not needed.

SomberNight commented 7 months ago

TaskTimeout subclasses CancelledError

Do you remember why this was done, and what do you think about changing it?

kyuupichan commented 7 months ago

It was done simply because that's what Curio does :). It is implemented by cancellation, both in this library and in Curio (and in asyncio too now, FWIW). Maybe that's not an argument.

I don't really intend to continue to maintain / work on aiorpcx going forwards, FYI. I'm happy to integrate patches and do releases but I don't really intend to spend any time on maintenance or improvements.

SomberNight commented 7 months ago

It is implemented by cancellation, both in this library and in Curio (and in asyncio too now, FWIW)

Re asyncio, the asyncio.timeout API (added in 3.11), which is very similar to timeout_after, raises TimeoutError(=asyncio.TimeoutError), which does not subclass CancelledError. I will think a bit more about the consequences, but I think it would be best if we just stopped inheriting.

I don't really intend to continue to maintain / work on aiorpcx going forwards, FYI. I'm happy to integrate patches and do releases but I don't really intend to spend any time on maintenance or improvements.

I see. Thanks for all the work done so far. I guess we will see if it would make sense to fork the library, perhaps when larger changes would be needed.

kyuupichan commented 7 months ago

What gave rise to this report? Does making it its own class fix the issues and pass the tests? It's not clear from the results what is going on.

Given the appearance of timeouts and task groups in asyncio (although with poor facilities / semantics IMO) it might be worth considering if this library will be of much use to you in the future.

SomberNight commented 7 months ago

In electrum, session.send_request() is called, but it is wrapped in what is effectively asyncio.wait_for. And the exception handling is outside that. https://github.com/spesmilo/electrum/blob/9b08eec4918c5c562350763ee052ec30ddec444b/electrum/interface.py#L165-L182

A server is sometimes not responding in time, so the requests are supposed to time out (by the session code, the time is exceeding sent_request_timeout): https://github.com/kyuupichan/aiorpcX/blob/a0be71c5671b07e02bedc51d7efaecda86f05128/aiorpcx/session.py#L511-L512

In the outer scope, I would expect TaskTimeout to get raised, however it is actually CancelledError that gets raised (depending on the python version...).

Given the appearance of timeouts and task groups in asyncio (although with poor facilities / semantics IMO) it might be worth considering if this library will be of much use to you in the future.

curio.py we will probably not need, but the other modules will still be useful. Note that the other modules themselves are using curio.py however. (which we might want to sever)

kyuupichan commented 7 months ago

OK I will merge this.

kyuupichan commented 7 months ago

One test needed a fix. I've made TaskTimeout its own exception as you indicated. Let me know if / when an updated release would be useful.

SomberNight commented 7 months ago

I've made TaskTimeout its own exception as you indicated.

Thanks.

One test needed a fix.

Ah, I see. Sorry I missed that. Some of the tests are broken/flaky on my machine. I've made some changes in https://github.com/kyuupichan/aiorpcX/pull/49 now.

Let me know if / when an updated release would be useful.

Indeed, it would be very welcome if you could tag and push a new release to pypi, ideally some time within the next week or two.

kyuupichan commented 7 months ago

Thanks. I've cleaned up the tests and run on all 3 platforms from 3.8 to 3.12; all tests seem good now. I'll do a release over the next few days.

kyuupichan commented 7 months ago

I've released 0.23: https://pypi.org/project/aiorpcX/