graceful shutdown order and cleanup contexts about aiohttp HOT 7 CLOSED

I think in this case, you do want it to be on_cleanup. Otherwise, there could be a chance that a request handler for hello() might try to send a message after the producer has been stopped.

If the problem is that aiokafka is creating new tasks after startup that are meant to sit idle, then I think this is a duplicate of #8387. In this case, I'm starting to think that we should actually track the tasks created for response handlers, and only wait on those tasks to complete. Then make the documentation clear that background tasks that should be allowed to complete should be scheduled through aiojobs and waited on in the cleanup_ctx explicitly.

from aiohttp.

Aiohttp could track its own created tasks and only wait on those.

So, the original reason for this design is so we'd wait on all user-created tasks. For example, if you use asyncio.shield() to protect a database write, this will then be a task which can't be tracked by aiohttp. So, the idea was to wait on all these things. It seems the approach is conflicting with too many other libraries though, so I guess we'll just have to rely on documentation to encourage users to ensure such tasks are tracked by their own code.

from aiohttp.

This is working as intended. If you want to cancel the tasks first, then use on_shutdown (maybe we could even add a shutdown_ctx or something...).

In the typical case, such a scheduler may be spawning background tasks for DB writes or similar. Such tasks may all complete within a second or two and so it's preferable to give them a chance to complete, rather than cancelling them immediately. You might actually end up with 2 different schedulers for the different use cases, one on shutdown and one on cleanup.

The expected behaviour is that the second stage of all defined cleanup contexts is called before aiohttp waits for any pending/remaining unmonitored tasks to be completed during a graceful shutdown.

If you think about the meaning of 'cleanup', this typically refers to cleaning up resources, such as database connections. If we were to do this before waiting on tasks, then the tasks we're waiting on would likely break as the resources they need have just been removed.

from aiohttp.

Hi @Dreamsorcerer , thanks for the quick reply.
Perhaps the wording in my reproduction example was not the best, apologies!

While I do cancel the tasks in the reproduction example, in the case of aiokafka we actually call a Client.stop() method which in itself also performs graceful shutdown of all the running background tasks of the Kafka client. This means that they are not abruptly canceled in the cleanup context.

Perhaps your suggestion of adding a shutdown_ctx for startup/shutdown pairs similar to how cleanup_ctx exists for startup/cleanup pairs could be useful indeed and make life-cycle management more flexible.

For reference, the following is a basic example using a real aiokafka producer client:

import asyncio
from aiohttp import web
from aiokafka import AIOKafkaProducer

async def kafka_producer(app):
    app["kafka_producer"] = AIOKafkaProducer(
        bootstrap_servers="localhost:9092",
    )
    yield
    await app["kafka_producer"].stop()

async def hello(request):
    await request.app["kafka_producer"].send("some-topic", b"Hello, world")
    return web.Response(text="Hello, world")

app = web.Application()
app.add_routes([web.get("/", hello)])
app.cleanup_ctx.append(kafka_producer)  # or shutdown_ctx ?

web.run_app(app)

For this example to work correctly (no shutdown timeout), it would need to use the proposed shutdown_ctx because we need to ask the aiokafka client to perform a graceful shutdown before aiohttp does the wait-then-cleanup section.

My last concern would be then how we distinguish what goes into cleanup_ctx or shutdown_ctx?
The current documentation about shutdown/cleanup states the following:

• on_shutdown
  A Signal that is fired on application shutdown.
  Subscribers may use the signal for gracefully closing long running connections, e.g. websockets and data streaming.
• on_cleanup
  A Signal that is fired on application cleanup.
  Subscribers may use the signal for gracefully closing connections to database server etc.

The aiokafka client classifies more as a "database server" than "long running connections", but we have established that is better suited for its life-cycle to be managed using on_shutdown.

Thanks for your patience and looking forward on improving the usability of aiohttp in this regard!

from aiohttp.

If the problem is that aiokafka is creating new tasks after startup that are meant to sit idle, then I think this is a duplicate of #8387.

Yes, basically for the case of aiokafka is exactly that it creates its own tasks in the background after startup. But I think this problem can be generalised to anything that does the same. The issue you linked has the example for a postgres client. And yes I agree this issue is a duplicate of that one. Thanks for linking as I didn't see it before.

In this case, I'm starting to think that we should actually track the tasks created for response handlers, and only wait on those tasks to complete.

I was looking into the code of aiohttp too to see how to improve on this and I was reaching the same conclusion indeed. Aiohttp could track its own created tasks and only wait on those. This would fully solve my issue and #8387, and likely be more robust because aiohttp would not need to do assumptions on the tasks submitted to the event loop.

Given that this is indeed a duplicate, I'm going to close this issue in favour of #8387 and follow that issue instead.

In the meantime, while it is not resolved, our current workaround is to simply configure a way shorter shutdown_timeout, e.g. 5s, so the hang is not that long on termination.

from aiohttp.

I was looking into the code of aiohttp too to see how to improve on this and I was reaching the same conclusion indeed.

If you'd like to have a go at implementing this change, that'd be very helpful.

from aiohttp.

If you'd like to have a go at implementing this change, that'd be very helpful.

Thanks for the offer, but I think I'm not versed enough on aiohttp's codebase for such a change :(

I'll try to take a look but can't promise any progress, at least not any time soon 🙈

from aiohttp.