encode / databases

Async database support for Python. 🗄
https://www.encode.io/databases/
BSD 3-Clause "New" or "Revised" License
3.8k stars 260 forks source link

Potential Inefficiency in Connection Management Leading to "Too Many Connections" Error #593

Closed AbdullahSaquib closed 1 month ago

AbdullahSaquib commented 2 months ago

In a highly refactored codebase using the Database class from databases/core.py, I've encountered a "too many connections" error. While investigating, I found comment on Stack Overflow suggesting that the library's connection management might be inefficient. Reviewing the code, I suspect the way connections are managed per task might be a contributing factor.

The current implementation uses a _connection_map keyed by asyncio.current_task(). This means each coroutine, including nested ones, gets its own connection. This can potentially result in a large number of connections being opened in complex applications.

To potentially address this, can we use contextvars to manage a single connection among parent and nested tasks. By storing the connection in a context variable, nested coroutines can reuse their parent task's connection, reducing the need to open multiple connections.

zevisert commented 1 month ago

FWIW: Coroutines are not always tasks. Databases only creates a new connection in separate asyncio.Tasks, not for each coroutine. Tasks enable concurrency but they are not threadsafe, so it's easier on users of this library to create new connections to keep concurrent code isolated. If you're willing to handle the complexity of sharing a connection between tasks, you can do so by passing a reference to a databases.core.Connection returned from databases.Database(...).connection() around.

I've been thinking a lot about how Databases works, and I concede it's still not ideal for me in all scenarios either. I wrote some of the logic you're referring to in PR #546 - I'd encourage you to read the discussion there and let me know it it influences your thoughts.

AbdullahSaquib commented 1 month ago

Thanks, @zevisert, for your helpful reply. It seems that my initial suspicions weren't correct. I also went through the PR you mentioned and found it interesting that ContextVar was initially considered for managing connections.