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

Breaks with nest-asyncio #3

Open
jacobtomlinson opened this issue Apr 30, 2023 · 11 comments · May be fixed by #6
Open

Breaks with nest-asyncio #3

jacobtomlinson opened this issue Apr 30, 2023 · 11 comments · May be fixed by #6

Comments

@jacobtomlinson
Copy link

jacobtomlinson commented Apr 30, 2023

I recently added nest-asyncio to a project that usesasyncio-atexit and it seems to break it. Do you have any thoughts on how I could use both together?

Minimal reproducible example

Before

# mre.py
import asyncio
import asyncio_atexit


async def finalizer():
    print("running finalizer")


async def main():
    asyncio_atexit.register(finalizer)


if __name__ == "__main__":
    print("before")
    asyncio.run(main())
    print("after")
$ python mre.py
before
running finalizer
after

After

# mre.py
import asyncio
import asyncio_atexit
+ import nest_asyncio

+ nest_asyncio.apply()

async def finalizer():
    print("running finalizer")


async def main():
    asyncio_atexit.register(finalizer)


if __name__ == "__main__":
    print("before")
    asyncio.run(main())
    print("after")
$ python mre.py
before
after
@jacobtomlinson jacobtomlinson changed the title Breaks with next-asyncio Breaks with nest-asyncio Apr 30, 2023
@minrk
Copy link
Owner

minrk commented May 4, 2023

I would consider this a bug in nest_asyncio.

This hook is based on patching loop.close, which is the only way to know an event loop is finished. asyncio.run calls loop.close when it's done, but nest_asynco patches asyncio.run to not call loop.close when it's done. That means asyncio.run with nest_asyncio doesn't clean up after itself in general, not just the hooks in this package.

That means the workaround ought to be to use the pre-asyncio.run style to do:

    print("before")
    loop = asyncio.new_event_loop()
    loop.run_until_complete(main())
    loop.close()
    print("after")

I'm not sure if there's a reason nest_asyncio doesn't close the loop like asyncio.run does (it would make sense if it re-uses event loops, unlike asyncio.run). If it doesn't re-use loops, this is likely a tiny fix in nest_asyncio.

@jacobtomlinson
Copy link
Author

Thanks @minrk. AFAIK it does reuse loops so the fix may be more involved.

Appreciate the workaround, I'll take a look.

@minrk
Copy link
Owner

minrk commented May 5, 2023

I opened erdewit/nest_asyncio#79 which should fix regular (not re-entrant) calls to asyncio.run.

Can I ask more about how you are using it and when you want to make sure cleanup is called? The big assumption here is that event_loop.close will be called, because otherwise we can't be sure it will be. But one thing we can do is also register loop.close with actual atexit, so that it will at least be called at process teardown.

@minrk minrk linked a pull request May 5, 2023 that will close this issue
@minrk
Copy link
Owner

minrk commented May 5, 2023

#6 adds an actual atexit hook to call loop.close if it hasn't been called yet. So if you don't call loop.close, you'll get a ResourceWarning and the loop will be closed. It's still a change with nest_asyncio, in that the close will be called at a different time (process teardown rather than at the end of asyncio.run), but at least it will be called. There should be no change if loop.close is called explicitly.

@jacobtomlinson
Copy link
Author

jacobtomlinson commented May 5, 2023

My use case (and I suspect many others) is to allow using asyncio.run from within a sync function that happens to be running inside some other event loop that you don't control.

Jupyter is the primary example here where notebook cells are technically executed within an event loop, but many users aren't aware of this and just use sync code.

import asyncio

## My library
# By default my library uses async/await but also exposes a sync API using asyncio.run
async def my_library_function():
    pass

def sync_api_for_my_library():
    return asyncio.run(my_library_function())  # This fails if not using nest-asyncio

## User code
# Many users are not aware or in the habit of using async/await in Jupyter
# so they just use regular sync functions and APIs
def jupyter_notebook_cell():
    sync_api_for_my_library()

## Jupyter notebook kernel
async def jupyter_kernel_runtime():
    jupyter_notebook_cell()

if __name__ == "__main__":
    asyncio.run(jupyter_kernel_runtime())

In an ideal world Jupyter users would use the asyncio API for my library. But if they don't they will run into errors unless we use something like nest-asyncio.

@jacobtomlinson
Copy link
Author

Thinking about it some more I don't really want the atexit to be called when the inner asyncio.run call completes, only when the outer one does.

If I change the outer loop startup to use the old style loop.run_until_complete() then the finalizer gets called.

# workaround-mre.py
import asyncio
import asyncio_atexit
import nest_asyncio

nest_asyncio.apply()


async def finalizer():
    print("running finalizer")


async def main():
    asyncio_atexit.register(finalizer)


if __name__ == "__main__":
    print("before")
    loop = asyncio.new_event_loop()
    loop.run_until_complete(main())
    loop.close()
    print("after")
$ python workaround-mre.py
before
running finalizer
after

But this would mean that folks using my library would need to ensure they start the loop in the same way, which I can't guarantee.

@minrk
Copy link
Owner

minrk commented May 5, 2023

Because using asyncio.run via nest asyncio is basically the same as await coro() in that it's the long-running kernel loop the atexit gets attached to, and it will be re-used in future asyncio.run calls, it's probably doing the right thing, and should get called when the event loop is closed at kernel shutdown (checks ipykernel to see if it actually calls loop.close... ipython/ipykernel#1116).

In gneral, asyncio_atexit relies on the event loop cleanly shutting down, just like atexit relies on the process cleanly shutting down (e.g. doesn't run if killed), so if there isn't a clean shutdown it may not get called. #6 should ensure close gets called, even without ipython/ipykernel#1116 (again, assuming the kernel actually does finish shutting down cleanly).

I've found running my own event loop in a thread for sync wrappers to be a simpler and more reliable approach to using nest_asyncio. But I know threads don't always work for people. And the advantage (and maybe disadvantage sometimes?) of nest_asyncio is that I think it actually cooperates with the parent loop, rather than blocking it, which a thread actually does.

nest_asyncio rejected my patch, but it wouldn't have affected the IPython case anyway, since it only touched the 'standard' not re-entrant calls to asyncio.run in your MWE, and not the case you actually care about.

@jacobtomlinson
Copy link
Author

Thanks @minrk for all the discussion here and for spending time on this.

Funnily enough I've done exactly that and changed my implementation to start a second loop in a new thread and use that to run my coroutines. This way I don't need nest-asyncio at all and asyncio-atexit works as expected.

Given how nest-asyncio monkey-patches asyncio it's probably best for me to not use it in my library as simply importing my library would have a potentially large and hidden consequence for my users. Running a new loop in a separate thread feels neater anyway.

I'm not too worried about the thread blocking the outer event loop because the user has called a sync function in an event loop anyway, so the choice to block the loop has already been made.

@jacobtomlinson
Copy link
Author

Technically this issue is still valid, nest-asyncio and asyncio-atexit are not compatible today. Given the discussions here and in patch-asyncio it sounds like they probably will never be. Not sure if you want to leave it open or maybe document this in the README and close it?

@minrk
Copy link
Owner

minrk commented May 8, 2023

Depends a little on what you mean. I think they are compatible, but there's a caveat worth documenting in that nest_asyncio changes when event loops are closed, which in turn changes when this hook may fire. As long as the eventloop is closed eventually, this should be okay. I'm not sure why #3 isn't working in your case, but it does seem like a reasonable addition, anyway.

Something like:

compatibility: asyncio-atexit hooks run when an event lop is closed, and nest_asyncio changes asyncio's behavior regarding closing event loops. With nest-asyncio, APIs like asyncio.run do not result in a closed event loop. Instead, the event loop is left open and reused in subsequent asyncio.run calls. This means the hooks registered with asyncio-atexit will not run until/unless the event loop is explicitly closed with event_loop.close(), which may not happen if nest_asyncio is not accounted for.

I personally think nest_asyncio makes a fundamental mistake in breaking the expectation that asyncio.run runs an event loop to completion, which it no longer does. It has instead made the choice to always leave the event loop open and never clean it up, which means it's on the user or application to call loop.close() when they are done (removing this need is one of the main reasons asyncio.run exists!). It also means that subsequent calls will get background tasks leaked from previously isolated runs. So all asyncio applications actually need to be aware that nest_asyncio might have been imported elsewhere, otherwise resources won't be cleaned up (not just this private-api patch, but asyncio itself).

@jacobtomlinson
Copy link
Author

Yeah I totally agree.

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

Successfully merging a pull request may close this issue.

2 participants