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

Feature request: Non-async shutdown #274

Open
couling opened this issue Sep 1, 2022 · 5 comments
Open

Feature request: Non-async shutdown #274

couling opened this issue Sep 1, 2022 · 5 comments

Comments

@couling
Copy link

couling commented Sep 1, 2022

Problem description

The use of an async context manager as the only way into having a client can be very awkward to work with, especially where the client is likely to be long lived for performance reasons (see also #236).

Besides from wanting to hold connections open for a long time, there's a common convention of having close() methods not coroutines. There's an example of this model in core python where async streams have a def close() method and an accompanying async def wait_closed(): https://docs.python.org/3/library/asyncio-stream.html#asyncio.StreamWriter.close

So often enough I find myself having to write a def close() method that is forced to ultimately invoke aioboto3 client's __aexit__(). The only workaround I am left with is to write something like:

class Foo:
    def close(self):
        # Schedule it and hope!  
        asyncio.get_running_loop().create_task(self._async_close())

    async def _async_close(self):
        ...
  • There's no way to catch exceptions and process them on this call stack
  • There's no garentee the new task will even run. close() might have been invoked in a finally: block right before finishing a loop.run_until_complete(bar())

Questions?

  • What does aioboto3 actually do that's truly async on __aexit__()
  • Does it really need to do it, or is it just a question of sequencing. Eg could it be refactored into an async def wait_closed().
  • If exit could be a method instead of coroutine, might the __aenter__() work be refactored into something like a async def startup()

Feature request

In an ideal world it would be very helpful if:

  • Startup / shutdown could be invoked outside of the context manager.
  • Shutdown could be non-async

Since the current way in is only through the async context manager there's no reason why this suggestion could not remain backwards compatible. I'm not suggesting removing the existing behaviour of __aenter__ and __aexit__.

@terricain
Copy link
Owner

So, this was done long enough ago that I can't remember exactly why, but it was along the lines of there were some pretty extensive changes to how aiobotocore was initialised and this was the easiest way to make it work and ensure aiobotocore's contexts were closed too.

I fully agree, its a pain, even using asyncexitstack's dont help much. If you can get it working without breaking backwards compatibility and it not looking too nasty, by all means, raise a PR.

@josebsneto
Copy link

Has this problem been resolved?

@dacevedo12
Copy link
Contributor

somewhat related https://pypi.org/project/asyncio-atexit/

@couling
Copy link
Author

couling commented Mar 13, 2023

I'm starting to have second thoughts about how useful this might be.

More recent python versions (3.11) remove the loop parameter from a lot of async objects and the removal of implicit loop creation all point to a different way of working.

Prior to python 3.10/3.11, many people wrote a lot of the setup code to execute before the event loop started. But with no implicit loop creation and no way to pass in a loop parameter, much of the setup code for core python classes must happen in an async loop.

This is leading to a common pattern to have two methods def main() and async def async_main() with one used to setup an event loop and the other to use it for async startup and shutdown.

In good time I believe non-async shutdown will become an anti-pattern.

@bk5115545
Copy link

While I would never actually recommend it, per https://peps.python.org/pep-0492/#await-expression, __await__() is compatible with the generator protocol.

So from a sync context, the following is valid:

list(client.__aexit__().__await__())

Note: this does block and probably has some major problems that I'm missing but it I've used it in AWS Lambda to ensure successful shutdown of long-lived clients for years so far without issue.

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

5 participants