dabeaz / curio

Good Curio!
Other
4.01k stars 240 forks source link

TaskGroup and daemon task inconsistency #340

Closed kyuupichan closed 3 years ago

kyuupichan commented 3 years ago

TaskGroup treats daemonic tasks inconsistently between a) adding them in the constructor task list, and b) adding them by calling add_task().

Specifically, tasks passed to the constructor all go in the _tasks member set and none go in the _daemonic set regardless of their daemonic status. However add_task() makes a distinction.

To fix the inconsistency and reduce code duplication I suggest the constructor call add_task() in a loop, and that the semaphore is initialized to zero before that loop. This also has the benefit of replacing the assert with a RuntimeError. (Edit: actually this doesn't work because sema.release is async, which is unfortunate and perhaps a reason for the current duplication of code).

kyuupichan commented 3 years ago

Whilst we're on the topic, is it intended that using the TaskGroup as a context manager and exiting the context with an exception does not cancel daemonic tasks? aexit calls cancel_remaining() which leaves daemonic tasks alone. Exiting the context without an exception does cancel daemonic tasks.

This is contrary to the documentation which states:

It is important to emphasize that no tasks placed in a task group survive past the join() operation or exit from a context manager.

dabeaz commented 3 years ago

Let me take a look at this (and the docs). There was some refactoring of daemon tasks at some point.

kyuupichan commented 3 years ago

Thanks, the changes look great. And once again, thanks for curio, it's a pleasure to use.