Why connection.close() can raise an exception?

[dolfinus]

I'm using AIOConnectionPool and try to check its behavior if LDAP server become unavailable.

Initial setup - I've started LDAP server on localhost, and try to search for a specific user:

Run local LDAP server

Using https://github.com/docker-ThoTeam/slapd-server-mock:

docker run --rm -v ./data.ldif.TEMPLATE:/bootstrap/data.ldif.TEMPLATE -p 389:389 thoteam/slapd-server-mock

>>> import logging
>>> from bonsai import LDAPClient, LDAPError, LDAPSearchScope
>>> from bonsai.asyncio import AIOConnectionPool

>>> log = logging.getLogger()

>>> client = LDAPClient("ldap://localhost")
>>> pool = AIOConnectionPool(client)

>>> async with pool.spawn() as connection:
>>>    results = await connection.search(base="ou=people,dc=ldapmock,dc=local", filter_exp="uid=developer1", scope=LDAPSearchScope.SUBTREE)

>>> results
[{'dn': <LDAPDN uid=developer1,ou=people,dc=ldapmock,dc=local>, 'objectClass': ['inetOrgPerson', 'organizationalPerson', 'person', 'top'], 'cn': ['developer One'], 'sn': ['One'], 'givenName': ['developer'], 'mail': ['[email protected]'], 'uid': ['developer1']}]

>>> connection
<bonsai.asyncio.aioconnection.AIOLDAPConnection object at 0x7f922fbd3770>
>>> connection.closed
False

Pool now has an opened connection. Stop LDAP server, and try to search for user again

try:
    async with pool.spawn() as connection:
        try:
            results = await connection.search(base="ou=people,dc=ldapmock,dc=local", filter_exp="uid=developer1", scope=LDAPSearchScope.SUBTREE)
        except LDAPError:
            log.exception("Exception during search")
            connection.close()
except LDAPError:
    log.error("Exception during spawn")
    raise

Calling connection.close() allows to skip returning connection back to pool, so next access to the same pool will initialize a new connection: https://github.com/noirello/bonsai/blob/74ce8f8548a04ab763a21ffaf4bef3ef529ced18/docs/advanced.rst#connection-timeouts

But connection.close() raises an exception, so connection is not being closed:

Exception during search
Traceback (most recent call last):
  File "<console>", line 4, in <module>
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 59, in _poll
    raise exc
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 54, in _poll
    return await asyncio.wait_for(fut, timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/tasks.py", line 452, in wait_for
    return await fut
           ^^^^^^^^^
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 40, in _ready
    res = super().get_result(msg_id)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
bonsai.errors.ConnectionError: Can't contact LDAP server. (0xFFFF [-1])
Exception during spawn
Traceback (most recent call last):
  File "<console>", line 4, in <module>
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 59, in _poll
    raise exc
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 54, in _poll
    return await asyncio.wait_for(fut, timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/tasks.py", line 452, in wait_for
    return await fut
           ^^^^^^^^^
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 40, in _ready
    res = super().get_result(msg_id)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
bonsai.errors.ConnectionError: Can't contact LDAP server. (0xFFFF [-1])

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.11/concurrent/futures/_base.py", line 456, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "<console>", line 7, in <module>
bonsai.errors.ConnectionError: Can't contact LDAP server. (0xFFFF [-1])

>>> connection
<bonsai.asyncio.aioconnection.AIOLDAPConnection object at 0x7f922fbd3770>
>>> connection.closed
False

Because connection.close() raised an exception, it looks like it haven't changed connection.closed flag to True. This connection is broken now, it cannot be used for making any requests.

For example, start LDAP server again and use the same pool. Because connection.closed is not True, it is still in the pool and being reused:

try:
    async with pool.spawn() as connection:
        try:
            results = await connection.search(base="ou=people,dc=ldapmock,dc=local", filter_exp="uid=developer1", scope=LDAPSearchScope.SUBTREE)
        except LDAPError:
            log.exception("Exception during search")
            connection.close()
except LDAPError:
    log.error("Exception during spawn")
    raise

Exception during search
Traceback (most recent call last):
  File "<console>", line 4, in <module>
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 59, in _poll
    raise exc
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 54, in _poll
    return await asyncio.wait_for(fut, timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/tasks.py", line 452, in wait_for
    return await fut
           ^^^^^^^^^
  File "/home/maxim/Repo/horizon/.venv/lib/python3.11/site-packages/bonsai/asyncio/aioconnection.py", line 40, in _ready
    res = super().get_result(msg_id)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
bonsai.errors.LDAPError: Success. (0x0000 [0])

Well, something is definitely went wrong. Fortunately, connection now become closed, so next requests made using the same pool working as expected:

>>> connection
<bonsai.asyncio.aioconnection.AIOLDAPConnection object at 0x7f922fbd3770>
>>> connection.closed
True

>>> try:
...     async with pool.spawn() as connection:
...         try:
...             results = await connection.search(base="ou=people,dc=ldapmock,dc=local", filter_exp="uid=developer1", scope=LDAPSearchScope.SUBTREE)
...         except LDAPError:
..             log.exception("Exception during search")
...             connection.close()
... except LDAPError:
...     log.error("Exception during spawn")
...     raise

>>> results
[{'dn': <LDAPDN uid=developer1,ou=people,dc=ldapmock,dc=local>, 'objectClass': ['inetOrgPerson', 'organizationalPerson', 'person', 'top'], 'cn': ['developer One'], 'sn': ['One'], 'givenName': ['developer'], 'mail': ['[email protected]'], 'uid': ['developer1']}]

>>> connection
<bonsai.asyncio.aioconnection.AIOLDAPConnection object at 0x7f922effc670>
>>> connection.closed
False

Few more things bother me:

1. connection.close() is synchronous in both LDAPConnection and AIOLDAPConnection, but it looks like it try to send network request to LDAP server for properly unbind/disconnect instead of just dropping connection. Current implementation of AIOConnectionPool.close() is asynchronous, and it calls this synchronous method, so it can block event loop: https://github.com/noirello/bonsai/blob/74ce8f8548a04ab763a21ffaf4bef3ef529ced18/src/bonsai/asyncio/aiopool.py#L82 https://github.com/noirello/bonsai/blob/74ce8f8548a04ab763a21ffaf4bef3ef529ced18/src/bonsai/pool.py#L115

2. Current Pool.close() implementation calls conn.close() for all connections using a cycle. So if some connection raises an exception during close, other connections in the pool may left unclosed. The only workaround I could find is:

for _ in range(pool.shared_connections):
    with contextlib.suppress(LDAPError):
        pool.close()

Which does not look right.

Proposed solution:

1. Change connection.close() implementation to never raise any exceptions, if that even possible.
2. If AIOLDAPConnection.close() requires some request to be made to LDAP server (unbind, explicit connection close), make it async, and use await in AIOConnectionPool.close()
3. If establishing the connection is failed with any LDAPError (not just ConnectionError, but also AuthenticationError, InvalidDN, TimeoutError and so on), immediately mark connection as closed without requiring to call connection.close() explicitly. This connection cannot be used anymore.
4. Change ConnectionPool.close() implementation to pop connection from set, e.g.:

def close(self) -> None:
    while self._idles:
        with suppress(Exception):
            conn = self._idles.pop()
            conn.close()
    while self._used:
        with suppress(Exception):
            conn = self._used.pop()
            conn.close()
    self._closed = True

To avoid partial cleanup of the pool if some connection cannot be closed.