Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task management in Asphalt v5 #97

Open
davidbrochart opened this issue Nov 8, 2023 · 34 comments
Open

Task management in Asphalt v5 #97

davidbrochart opened this issue Nov 8, 2023 · 34 comments

Comments

@davidbrochart
Copy link
Contributor

@agronholm I don't know if you figured out everything regarding task management in Asphalt v5, otherwise I thought we could discuss it here.
My understanding is that with the move to AnyIO, launching tasks in a component is not as trivial. In Asphalt v4, one can just use asyncio to create a task in the start method, and stop it in the tear-down step. But in v5 with AnyIO, one cannot create a new task group and call start_soon in the component's start method, because start would not return until the tasks are done, instead of launching them in the background. Creating a "component-level" task group in which the start method would be spawned, and passing it e.g. as an attribute of the context could be a solution. One could use it to spawn background tasks, and cancelling this task group in the tear-down step would cancel these tasks.
I remember you also mentioned cancellation order. For instance, if component A uses a resource from component B, component A should be teared down first, otherwise it would use a resource that doesn't exist anymore for some time. But I'm also thinking that components A and B can depend on each other, right? Component A can provide a resource that component B needs, and component A can need a resource that component B provides. In this case there is no order that would guarantee a clean cancellation. But maybe this inter-dependency between components is wrong in the first place?
I'm sure I forgot other issues, happy to discuss them if you need help.

@agronholm
Copy link
Member

Creating a "component-level" task group in which the start method would be spawned, and passing it e.g. as an attribute of the context could be a solution. One could use it to spawn background tasks, and cancelling this task group in the tear-down step would cancel these tasks.

Suppose you had just one component that launches a background task. What would trigger the teardown of the top level context, and thus the cancellation of the task group?

For instance, if component A uses a resource from component B

In this example, are components A and B sibling components, or parent and child?

@davidbrochart
Copy link
Contributor Author

Suppose you had just one component that launches a background task. What would trigger the teardown of the top level context, and thus the cancellation of the task group?

I general, it is when the application is shutting down, right? So I guess it is the role of e.g. the sigterm_handler.

In this example, are components A and B sibling components, or parent and child?

Say they are sibling components. If component A is a child of component B, then I guess tearing down children first would be a solution, right?

@agronholm
Copy link
Member

I general, it is when the application is shutting down, right? So I guess it is the role of e.g. the sigterm_handler.

So you're testing your app, and you have a fixture where you set up your app:

@pytest.fixture
async def my_app():
    component = MyRootComponent()
    async with Context():
        await component.start()
        yield

How do you signal that you want the context to be shut down? The same applies to child components. Any component that leaves background tasks running after starting will have their own contexts that need to be shut down at some point.

I guess what I'm trying to ask is whether exiting a Context should start the teardown process and cancel all tasks in the group? Or should the context keep running, and require a call to .close() in order to start the teardown?

@davidbrochart
Copy link
Contributor Author

In v4, background tasks should explicitly be cancelled by the user in the teardown step, which is called by .close(), right? In v5, background tasks will automatically be cancelled with the task group cancellation, but this still happens in the teardown step, and thus it still requires a call to .close(). I don't really see why it should be different in v4 and v5, but I'm probably missing something.

@agronholm
Copy link
Member

Even in v4, exiting the Context automatically closes it. In fact, I was going to retire the close() method entirely, unless we decide that it should be repurposed. I at least never explicitly call close() in my Asphalt apps.

@davidbrochart
Copy link
Contributor Author

I also don't call close(), now I think I'm seeing your point. Exiting the Context will still automatically close it in v5, but we will use close() to cancel its task group. Is it what you're thinking about?

@agronholm
Copy link
Member

That was my line of thinking, yes. Whether this is a good idea, I don't know yet.

@agronholm
Copy link
Member

I think the next step is to do some Document Driven Development, that is, make a tree chart of components and then go over the initialization and shutdown processes, step by step.

@davidbrochart
Copy link
Contributor Author

Not sure if this is what you want, but here is an example:

root component
|-- component A
|   |-- component A1
|   |-- component A2
|-- component B

Initialization

The root component starts sibling components A and B, and component A starts sibling components A1 and A2. Let's say each component launches a task in the background, and exits their context.

Shutdown

Components are processed from children to parents, calling close() on their Context which causes their background tasks to be cancelled and eventually their task group context manager to exit. The application shuts down when all components' background tasks are done.

@agronholm
Copy link
Member

The root component starts sibling components A and B, and component A starts sibling components A1 and A2. Let's say each component launches a task in the background, and exits their context.

If the components exit their contexts after initialization, where are the background tasks managed then?

@davidbrochart
Copy link
Contributor Author

I was thinking that background tasks were launched in a task group that would be an attribute of the context, but exiting the context would not do anything to this task group yet. Calling close() on this (exited but existing) context would cancel the task group.

@agronholm
Copy link
Member

In order for a task group to function, it needs to be active in a running task, so there must be a hierarchy of active task groups. I don't see where this fits in in your solution.

@davidbrochart
Copy link
Contributor Author

I'm imagining that each component creates a task group where it starts its child components (it calls start_soon(component.start, ctx)), and this task group is passed to the child components' context as an attribute so that they can launch their tasks on it. So there will be a hierarchy of task groups. Am I missing something?

@agronholm
Copy link
Member

agronholm commented Nov 10, 2023

If a component creates its own task group, what task manages it once Component.start() returns?

@davidbrochart
Copy link
Contributor Author

A component creates a task group for its children. In the example above, component A creates a task group to start components A1 and A2. The root component creates a task group to start components A and B. And I guess the root component's start method is just awaited?

@agronholm
Copy link
Member

agronholm commented Nov 10, 2023

Are you saying that Component.start() does not return until the application is stopped? Or what do you mean by "the root component's start method is just awaited"?

@davidbrochart
Copy link
Contributor Author

Component.start() does return, what I mean is that there is probably no need to create a task group in which to launch the root component's start() since there is no other component, we can just do await root_component.start(). But every component, including the root component, creates a task group in which it launches it's child components' start().
Am I completely off-road or does this make sense?

@agronholm
Copy link
Member

Suppose you're starting the root component, and it creates a task group within start(). Where are the __aenter__() and __aexit__() for the task group called? Is this something you're thinking about?

async with Context():
    await root_component.start()

Does the context host the task group? If not, where is that done? What will cause the context to be shut down here? In v4, simply exiting the async with causes the teardown to start.

@davidbrochart
Copy link
Contributor Author

What about something like that?

async with create_task_group() as tg:
    async with Context(tg) as ctx:
        # now there is ctx.tg that users can use to launch background tasks
        await root_component.start(ctx)
    # teardown starts, cancel background tasks
    tg.cancel_scope.cancel()

@agronholm
Copy link
Member

This is no different from having an internal task group in the Context instance, and doesn't answer any of my questions.
In the above snippet, you commented teardown starts, but that's after the context's context manager has already exited! What would be the point of the use of Context as a context manager if exiting it doesn't execute the teardown process?

@davidbrochart
Copy link
Contributor Author

I'm not sure to understand, the task group is created and entered before the Context enters. But yes it could be done inside Context. The component's start() won't block after tasks have been launched, so the Context's context manager can execute the teardown process when exiting, just like in v4. Actually I think it should be the responsibility of the component to cancel its tasks in its teardown step.

@agronholm
Copy link
Member

Sorry about the lack of responses. Assuming your code above, how would the starting of the subcomponents of that component go? Would the root component's start() method have to explicitly do that? How are the subcomponents' task groups managed?

@agronholm
Copy link
Member

One idea I had was that the context would have a method to start a background task. Then, that task would be added to the context stack just like an added resource, so that it would be individually cancelled and awaited on during teardown. Alternatively, all the background tasks would be cancelled and awaited on before or after the resources would be torn down.

Assuming that we want the async with Context(): block to start the complete teardown of resources and tasks, something needs to be done on the application runner level to keep the application running. I seem to have had await sleep(float("inf")) there; that should work nicely.

To highlight the issues, here's a code-doodle of ContainerComponent.start():

    async def start(self, ctx: ComponentContext) -> None:
        """
        Create child components that have been configured but not yet created and then
        calls their :meth:`~Component.start` methods in separate tasks and waits until
        they have completed.

        """
        for alias in self.component_configs:
            if alias not in self.child_components:
                self.add_component(alias)

        async with create_task_group() as tg:
            for alias, component in self.child_components.items():
                tg.start_soon(component.start, ctx)

The current problem with this is that the same context is used for child components too, but that won't work when you want to shut down the child components hierarchically (child components before the parent).

@davidbrochart
Copy link
Contributor Author

I must admit I'm a bit lost about what we're trying to achieve now. I thought the primary issue was that AnyIO's tasks don't play well with Asphalt, because unlike asyncio's tasks we cannot just launch them in the component's start(), otherwise it would block and the component would never start:

class MyComponent(Component):
    async def start(self, ctx: Context) -> None:
        # with asyncio, doesn't block:
        asyncio.create_task(my_task())
        # with AnyIO, does block:
        async with create_task_group() as tg:
            tg.start_soon(my_task)

Anyway, we wouldn't want to leave the tasks running at teardown, so we want to cancel them, but unfortunately this doesn't work either (see this issue):

class MyComponent(Component):
    @context_teardown
    async def start(self, ctx: Context) -> AsyncGenerator[None, BaseException | None]:
        async with create_task_group() as tg:
            tg.start_soon(my_task)
            yield
            tg.cancel_scope.cancel()

In the end, don't we just want the latter to work? Since the issue seems to be that "a new task is spawned for the teardown operation" ("but AnyIO cancel scopes require that they are opened and closed in the same task"), is there a way for the teardown to be done in the same task?

@agronholm
Copy link
Member

I must admit I'm a bit lost about what we're trying to achieve now. I thought the primary issue was that AnyIO's tasks don't play well with Asphalt, because unlike asyncio's tasks we cannot just launch them in the component's start(), otherwise it would block and the component would never start:

We can, but the task group that's hosting those tasks must be constantly active, so it can react when a child task crashes.
This is to say, so long as we can access a running task group somewhere, it doesn't have to be running inside the start() function.

In the end, don't we just want the latter to work? Since the issue seems to be that "a new task is spawned for the teardown operation" ("but AnyIO cancel scopes require that they are opened and closed in the same task"), is there a way for the teardown to be done in the same task?

The problem with what you pasted is that the generator is adjourned at yield. What happens when a child task crashes? What task gets interrupted with CancelledError then?

@davidbrochart
Copy link
Contributor Author

The problem with what you pasted is that the generator is adjourned at yield. What happens when a child task crashes? What task gets interrupted with CancelledError then?

Yes you're right, currently nothing happens when a task crashes (and Python surprisingly takes 100% CPU):

async def my_task():
    1 / 0

@davidbrochart
Copy link
Contributor Author

I like your idea of treating background tasks as resources. But I was thinking that maybe a background task should instead be tied to a resource, meaning that for this resource to be used the associated task should be running. That would solve the issue of the order of cancellation of tasks.

@agronholm
Copy link
Member

Wouldn't this be automatically solved by always tearing down resources and tasks in the exact reverse order in which they were added to the context?

@davidbrochart
Copy link
Contributor Author

Maybe yes, you're right.

@agronholm
Copy link
Member

So the only remaining issue is how to manage the background tasks spawned from the start() methods. This would have to be in a task that is constantly waiting in an async with Context() block.

@davidbrochart
Copy link
Contributor Author

Great to see that you're making progress!

@davidbrochart
Copy link
Contributor Author

So should any error in background tasks trigger the teardown of the component, and all its resources?

@agronholm
Copy link
Member

Yes, the whole application should come crashing down if there's an unhandled error.

@davidbrochart
Copy link
Contributor Author

One idea I had was that the context would have a method to start a background task.

There should also be a context method to cancel the underlying task group's cancel_scope, right? In case the user wants to manually cancel their background tasks. Or a context property to get the cancel_scope?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants