Users almost never discover the self._tasks set in BackgroundService, the most common pattern seems to be to just store the task as a new instance attribute, but then users forget to override cancel() and stop() to cancel and stop the new task.
Another common mistake is to forget calling super().__init__().
Also the internal task list is of type Task[Any], which completely subverts the type system, making user's code less safe.
All of this is error prone and confusing.
Proposed solution
Option 1
One way to improve this might be to remove the _tasks attribute and make BackgroundService more abstract, having no attributes at all, and adding some new abstract methods/properties:
@abstract
@ property
def tasks(self) -> Iterator[Task[Any]]:
...
@abstract
@ property
def forget_task(self, task: Task[Any]) -> bool: # True if it was removed
...
Then stop() and cancel() can use this interface to do the automatic stopping and cleanup, and users can have tasks as attributes or list or sets or dicts according to what's more convenient to them.
In this scheme it is still to be determine how we can force users to re-implement the new abstract methods in actor subclasses, as the Actor class will need to define them to manage the run() task.
Option 2
[!NOTE]
We should probably go with option 1, as this option is too error prone, and would probably lead to having 2 containers to store tasks, one in the BackgoundService class and one in the sub-class, specially for sub-classes that need special containers for tasks, like a dict, to quickly lookup a particular task (example).
Similar to option 1 but still keep an internal list in the BackgroundService, and have this instead:
@ property
def register_task_like(self, task_like: TaskLike) -> bool: # True if it was registered
...
@ property
def unregister_task_like(self, task_like: TaskLike) -> bool: # True if it was unregistered
...
So users still need to register and unregister any task they want the BackgroundService to manage. The only problem with this is users are still very likely to forget to register new tasks...
Related issues
The solution to this issue needs to have in mind the following related issues:
What's needed?
Users almost never discover the
self._tasksset inBackgroundService, the most common pattern seems to be to just store the task as a new instance attribute, but then users forget to overridecancel()andstop()to cancel and stop the new task.Another common mistake is to forget calling
super().__init__().Also the internal task list is of type
Task[Any], which completely subverts the type system, making user's code less safe.All of this is error prone and confusing.
Proposed solution
Option 1
One way to improve this might be to remove the
_tasksattribute and makeBackgroundServicemore abstract, having no attributes at all, and adding some new abstract methods/properties:Then
stop()andcancel()can use this interface to do the automatic stopping and cleanup, and users can have tasks as attributes or list or sets or dicts according to what's more convenient to them.In this scheme it is still to be determine how we can force users to re-implement the new abstract methods in actor subclasses, as the
Actorclass will need to define them to manage therun()task.Option 2
Similar to option 1 but still keep an internal list in the
BackgroundService, and have this instead:So users still need to register and unregister any task they want the
BackgroundServiceto manage. The only problem with this is users are still very likely to forget to register new tasks...Related issues
The solution to this issue needs to have in mind the following related issues:
9