search

long2ice / asynch

An asyncio ClickHouse Python Driver with native (TCP) interface support.
https://github.com/long2ice/asynch
Apache License 2.0
185 stars 43 forks source link

Fix `Pool` dangling connections #109

Closed stankudrow closed 2 months ago

stankudrow commented 3 months ago

The MRs #106 and #107 were only foresteps to the real problem: poor connection management via the Pool class. My colleagues found that the version 0.2.4 leaves unclosed/dangling connections to the ClickHouse. Rolling back to the previous v0.2.3 made this problem disappear. Moreover, there is the discussion #108 highlighting the same problem.

This PR #109 addresses the dangling connection issue by injecting into the Pool class the interface for asynchronous context manager support. When entering a pool context, the minsize connections are established. Within the pool context one can acquire up to the maxsize connections, but not more or PoolError will be raised. Before leaving the pool context, all connections get closed.

Many thanks to the psycopg_pool open-source implementation and this article on psycopg3 pool design.

UPD

Sorry for overloading the PR with changes on Connection class, including tests on asynch.proto.Connection class and some minor grooming or refactoring. That is the result of API alignment in my opinion + bringing more order and unification in the API.

stankudrow commented 3 months ago

@long2ice , I need thorough reviews on this PR, it is about changes in architecture, could you ping/tag other maintainers?

@DFilyushin, please review this PR as well (also keeping on the discussion #108 ).

stankudrow commented 3 months ago

@DFilyushin , many thanks for approval.

stankudrow commented 3 months ago

@boolka , @gnomeby , @DaniilAnichin , @pufit , @KPull , @lxneng , could you possibly share your thoughts, leave your comments and review this PR as well?

gnomeby commented 3 months ago

OK. will see.

stankudrow commented 2 months ago

@gnomeby , @long2ice , could you have a thorough look at this work?

stankudrow commented 2 months ago

@boolka , could you review this PR with the approve of yours when an agreement with (maybe almost) all items on this work will be attained?

@gnomeby , the same request for a review, s'il vous plaît.

long2ice commented 2 months ago

Thanks!

gnomeby commented 2 months ago

Not working for me: ` 2024-08-26 12:16:04 asynch.proto.connection WARNING | Connection was closed, reconnecting.

2024-08-26 12:16:35 asynch.pool WARNING | Connection lost `