asphalt-framework / asphalt

Asphalt application framework (core)
Apache License 2.0
256 stars 12 forks source link

Task management in Asphalt v5 #97

Open davidbrochart opened 10 months ago

davidbrochart commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

agronholm commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

davidbrochart commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Maybe yes, you're right.

agronholm commented 10 months ago

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 commented 10 months ago

Great to see that you're making progress!

davidbrochart commented 10 months ago

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

agronholm commented 10 months ago

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

davidbrochart commented 9 months ago

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?