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

Process hangs until SQLiteBackend instance is closed #173

Closed
aaraney opened this issue Sep 19, 2023 · 13 comments
Closed

Process hangs until SQLiteBackend instance is closed #173

aaraney opened this issue Sep 19, 2023 · 13 comments
Labels
bug Something isn't working
Milestone

Comments

@aaraney
Copy link
Contributor

aaraney commented Sep 19, 2023

The problem

Process will hang until instance of SQLiteBackend is closed. More specifically, the process will hang until the SQLiteBackend's responses: SQLitePickleCache and redirects: SQLiteCache instance variables are closed. Note, Ive not tested this on other backends.

Expected behavior

Process will exit without explicit close of SQLiteBackend.

Steps to reproduce the behavior

import asyncio
from aiohttp_client_cache import CachedSession, SQLiteBackend

async def hangs_forever():
    cache = SQLiteBackend("test_cache")
    async with CachedSession(cache=cache) as session:
        await session.get("http://httpbin.org/delay/1")
    # await cache.close()


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

Workarounds

The only workaround ive found thus far is to explicitly close the SQLiteBackend cache instance (e.g. await cache.close()).

Environment

  • aiohttp-client-cache version: [e.g. 0.8.2]
  • Python version: [e.g. 3.9]
  • Platform: [e.g. macOS Ventura 13.4.1 arm64]
@aaraney aaraney added the bug Something isn't working label Sep 19, 2023
@aaraney
Copy link
Contributor Author

aaraney commented Sep 19, 2023

This looks like an issue stemming from aiosqlite.Connection which inherits from threading.Thread.

@JWCook JWCook added this to the v0.9 milestone Sep 19, 2023
@JWCook
Copy link
Member

JWCook commented Sep 19, 2023

Could you give the changes in main a try for me? I believe that will fix the issue. I'll get a patch release out for that soon.

@aaraney
Copy link
Contributor Author

aaraney commented Sep 19, 2023

@JWCook, certainly! Just tested at ae63533 and the process exited as expected.

@aaraney aaraney closed this as completed Sep 19, 2023
@aaraney
Copy link
Contributor Author

aaraney commented Sep 19, 2023

For those interested, the fix was introduced in a33331f.

@aaraney
Copy link
Contributor Author

aaraney commented Sep 19, 2023

Fixed in 0.9.0

aaraney added a commit to aaraney/hydrotools that referenced this issue Sep 19, 2023
See NOAA-OWP#103 for background as to why aiohttp was previously pinned. In
short, the issues that led to pinning aiohttp have be resolved upstream.

aiohttp_client_cache >= 0.9.0 needed b.c.
requests-cache/aiohttp-client-cache#173
@aaraney
Copy link
Contributor Author

aaraney commented Sep 21, 2023

Related to #147.

@aaraney
Copy link
Contributor Author

aaraney commented Sep 21, 2023

I am reopening this because it seems that the switch to aiosqlite has altered the supported semantics for using aiohttp_client_cache.CachedSession. The following code hangs:

import asyncio
from aiohttp_client_cache import SQLiteBackend, CachedSession


async def main():
    cache = SQLiteBackend()
    client = CachedSession(cache=cache)

    req = await client.get("http://httpbin.org/get")
    resp = await req.text()
    req.close()
    print(resp)

    # does not hang if properly close (identical behavior to async context manager)
    # await client.close()


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

TLDR; the above code hangs because client.close() is never called and awaited.

It is required that client.close() is called and awaited because SQLiteBackend creates and uses a aiosqlite.Connection that runs in its own thread. The aiosqlite.Connection acts like an event loop (see its run() method). It accepts work (coroutines) over a queue and then reschedules coroutines on the main thread's event loop. This is the problem, in the above code, the event loop runs for a shorter amount of time than the aiosqlite.Connection thread (as soon as main() returns, the event loop starts its shutdown). This makes it impossible to close unless done implicitly or explicitly at the call site. In some cases adding a finalizer (__del__) to SQLiteBackend that schedules a close() task on the main event loop to could remedy this, but there is no guarantee that the event loop will be open long enough for that task to be scheduled and run.

The unfortunate side-affect of this behavior is that all code that uses a SQLiteBackend passes the responsibility of handling the lifetime of the aiosqlite.Connection's thread to the end user. Meaning users of that code need to either explicitly call a shutdown method (i.e. close()) or use the object via a context manager. For example:

import asyncio
from aiohttp_client_cache import SQLiteBackend, CachedSession


class Client:
    def __init__(self):
        self._session = CachedSession(cache=SQLiteBackend())

    async def get(self) -> str:
        async with self._session.get("http://httpbin.org/delay/1") as req:
            return await req.text()

    async def close(self) -> None:
        # failing to call close will result in the program hanging
        await self._session.close()

    async def __aenter__(self) -> "Client":
        return self

    async def __aexit__(self, exc_type, exc_val, exc_tb) -> None:
        await self.close()


async def main():
    client = Client()
    res = await client.get()
    print(res)
    # this hangs because client.close() is never called


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

@aaraney aaraney reopened this Sep 21, 2023
@JWCook
Copy link
Member

JWCook commented Oct 3, 2023

Are you still seeing this behavior in the latest release (0.9.1)? SQLite connections are now closed automatically on contextmanager exit, and your code snippet appears to work fine for me locally and in CI. Note that this behavior can also be changed with the autoclose parameter (see #162).

@netomi
Copy link
Contributor

netomi commented Oct 3, 2023

If the CachedSession is not properly closed this can happen, however, close is not called magically without using a contextmanager.

You have to do:

  async with CachedSession(cache=SQLiteBackend()) as client:
      # use client

after the with block finished, the session and the associated cache will be properly closed and no dangling sqlite connection will be running.

@JWCook
Copy link
Member

JWCook commented Oct 3, 2023

Okay, I understand now. Yeah, getting aiosqlite to close properly on its own without using a contextmanager is a bit tricky. Looks like there are a couple relevant open issues on the aiosqlite repo, but there are some (less than ideal) ways I could hack around this in the mean time.

I'll make a separate issue for this (#187).

@aaraney
Copy link
Contributor Author

aaraney commented Oct 3, 2023

@JWCook thanks for continuing to look into this and try and find a viable way to move forward. I am more than happy to collaborate on this issue, meaning submitting a PR, working through proposed solutions, or just testing it. I've been using your package for a long time and would like to continue to do so. You've always been quick to respond and great to communicate with!

@JWCook
Copy link
Member

JWCook commented Oct 3, 2023

Thanks, I'm glad you've found it useful! I'll try to get at least a short-term fix for this released within the next couple weeks.

One thing that would help would be testing out the changes in #189, and let me know if you have any thoughts about that approach.

@aaraney
Copy link
Contributor Author

aaraney commented Oct 3, 2023

@JWCook for sure! I'll take a look at that over the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants