Skip to content

Add connection_holder_class to Pool for custom connection handling #1251

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KorsaR-ZN
Copy link

@KorsaR-ZN KorsaR-ZN commented Apr 8, 2025

This PR introduces the connection_holder_class parameter to the Pool class, enabling developers to plug in custom logic for managing individual connections — including acquisition, release, and lifecycle control. This added flexibility is particularly useful for advanced or non-standard use cases where the default connection handling may be insufficient.

The change also lays the groundwork for addressing issue #989 by making it possible to implement custom connection management strategies tailored to specific requirements.

KorsaR-ZN added a commit to KorsaR-ZN/asyncpg that referenced this pull request Apr 8, 2025
@KorsaR-ZN KorsaR-ZN force-pushed the feature/pool-connection-management branch from 4a5d365 to fbd1ef2 Compare April 8, 2025 16:49
…agicStack#1251

Introduces a `connection_holder_class` parameter to the `Pool` class, enabling the use of custom classes for managing individual connections. This provides more flexibility for handling connection lifecycle, including acquisition and release logic, tailored to specific use cases.
@KorsaR-ZN KorsaR-ZN force-pushed the feature/pool-connection-management branch from fbd1ef2 to daa3d96 Compare April 8, 2025 16:53
@KorsaR-ZN
Copy link
Author

If you consider the behavior described in #989 to be a bug in connection management, I also have a modified version of PoolConnectionHolder that addresses it.

This version ensures that connections are not closed below min_size, even in cases where the database drops the connection. I'm happy to submit it as a separate PR if there's interest.

@elprans
Copy link
Member

elprans commented Apr 8, 2025

If you consider the behavior described in #989 to be a bug in connection management, I also have a modified version of PoolConnectionHolder that addresses it.

I think #989 is arguably a bug.

@KorsaR-ZN
Copy link
Author

KorsaR-ZN commented Apr 9, 2025

If you consider the behavior described in #989 to be a bug in connection management, I also have a modified version of PoolConnectionHolder that addresses it.

I think #989 is arguably a bug.

Great, I also consider the current behavior to be a bug. In that case, I suggest we fix it as part of this PR — I'll update it a bit later to include the improved PoolConnectionHolder

@elprans
Copy link
Member

elprans commented Apr 9, 2025

Great, I also consider the current behavior to be a bug. In that case, I suggest we fix it as part of this PR — I'll update it a bit later to include the improved PoolConnectionHolder

I would prefer if we separated the fix into a standalone PR.

PoolConnectionHolder is really a fairly low-level implementation detail and I'm a bit wary of turning it into a public API. People already tend to misuse private methods with custom connection/pool classes and things break when we change things (as simple as adding a new argument to a private method). It might be more practical to allow more configurability to Pool via a policy callback or something like that rather than a full subclass.

@KorsaR-ZN
Copy link
Author

KorsaR-ZN commented Apr 9, 2025

@elprans, I understand your concerns about making PoolConnectionHolder part of the public API, as it is quite low-level. However, it seems that PoolConnectionHolder is essentially the connection lifecycle management policy within the pool. We might just need to simplify it a bit and make it more suitable for public use, which is what I originally planned to do.

Regarding the idea of adding a separate policy, callback, or something similar, I’ll consider it, but it seems that this would only add extra complexity due to the additional layer of abstraction.

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 this pull request may close these issues.

2 participants