Update asyncio external_api and expand tests #16
Conversation
|
I'll hold on merging this until #15 is merged and I can add documentation. |
|
Exception name should be changed because the service doesn't have to have been cancelled for this to raise. Maybe |
|
@pipermerriam Feel free to merge #15 if you want. I didn't want to merge it without a ✔️ 😅 |
|
@carver I would recommend taking a look at this. This is my best idea thus far for how to let services expose external APIs in a way that ensures that they are only operational during the time the service is running and that the called isn't required to wrap them in any special way. The caller should wrap them in a |
pipermerriam
force-pushed
the
piper/cleanup-asyncio-external-api-decorator
branch
from
77a8d36
to
815eca0
Compare
There was a problem hiding this comment.
Same question that @ralexstokes had: how can we run cleanup tasks on shutdown?
async_service/asyncio.py
Outdated
| @@ -272,43 +273,77 @@ def run_child_service( | |||
| return child_manager | |||
|
|
|||
|
|
|||
| TReturn = TypeVar("TReturn") | |||
| TFunc = TypeVar("TFunc", bound=Callable[..., Coroutine[Any, Any, TReturn]]) | |||
| def cleanup_tasks(*tasks: "asyncio.Future[Any]") -> AsyncContextManager[None]: | |||
There was a problem hiding this comment.
Is this wrapper around _cleanup_tasks() just dealing with some mypy shenanigans?
There was a problem hiding this comment.
AIUI (and it took me a really long time before I did), this is a trick to "consume" the first task and have _cleanup_tasks() called with only the remaining ones. Otherwise, on line 294 we'd end up always calling it with the same sequence of tasks. It is a clever trick but doesn't come naturally to me (and I guess to you either?), so I can't help but wonder if it wouldn't be better to write it in a less clever but more obvious way?
There was a problem hiding this comment.
I think it was actually both of these things. Previously, I needed a type: ignore on the _cleanup_tasks implementation (for some reason that is no longer the case either because mypy got smarter or something else). That went away at some point but, @gsalgado is also correct that this was a way to consume the first element of the list of tasks without needing to do any length checking or error handling because it allows it to be handled at the language level.
I'll look at how to clean this up, either by doing it less cleverly or potentially by adding some comments to explain it clearly.
docs/async_service.rst
Outdated
| @@ -172,6 +172,11 @@ If a task raises an exception it will trigger cancellation of the service. | |||
| Upon exiting, all errors that were encountered while running the service will | |||
| be re-raised. | |||
|
|
|||
| For slighly nicer logging output you can provide a ``name`` as a keyword | |||
| argument to `~async_service.abc._InternalManagerAPI.run_task` which will be | |||
There was a problem hiding this comment.
Hm, it looks like this is a private API, but we're documenting it publicly. Should it become a public API?
There was a problem hiding this comment.
Agreed. I think _InternalManagerAPI should be renamed and drop the underscore. As I understand it, we want to make sure that external consumers are only exposed to ManagerAPI which is fine. We can ensure to use ManagerAPI as the return type to hide the fact that we are returning an _InternalManagerAPI. So, if someone would want to call APIs on it they would be forced to cast() it down to the underlying _InternalManagerAPI.
But _InternalManagerAPI is still used from within the service so to the internals of the service there's nothing private about _InternalManagerAPI. I guess that's true for any class, there's always someone consuming a class which means it can not be private on the class level.
I think _InternalManagerAPI should just become InternalManagerAPI
docs/async_service.rst
Outdated
| self.manager.run_task(self.do_long_running_thing, daemon=True) | ||
| self.manager.run_daemon_task(self.do_long_running_thing) |
There was a problem hiding this comment.
Is one of them preferred? Should we discourage use of one as deprecated?
There was a problem hiding this comment.
Anyone have thoughts on just removing the daemon=True option so that you just either use run_task or run_daemon_task based on your needs?
There was a problem hiding this comment.
👍 for removing the boolean daemon flag option
Here are my thoughts. Let me know what you two think. |
pipermerriam
force-pushed
the
piper/cleanup-asyncio-external-api-decorator
branch
from
16d0708
to
0aa9220
Compare
pipermerriam
deleted the
piper/cleanup-asyncio-external-api-decorator
branch
What was wrong?
The
async_service.asyncio.external_apiwas not type hinted correctly and was lacking tests.How was it fixed?
Fixed the type hints as well as including typing tests to confirm these type hints are correct as well as expanding the test suite for this decorator.
Cute Animal Picture