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

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

Closed
AbdullahSaquib opened this issue Jun 18, 2024 · 2 comments

Comments

@AbdullahSaquib
Copy link

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
Copy link
Contributor

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
Copy link
Author

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.

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

2 participants