Merge "Implement more robust connection handling for asynchronous LDAP calls" into stable/ussuri

2 files changed, 75 insertions, 38 deletions

diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py
index 2b6281d93..d4cdda845 100644
--- a/keystone/identity/backends/ldap/common.py
+++ b/keystone/identity/backends/ldap/common.py
@@ -14,7 +14,6 @@
 
 import abc
 import codecs
-import functools
 import os.path
 import re
 import sys
@@ -634,10 +633,36 @@ def _common_ldap_initialization(url, use_tls=False, tls_cacertfile=None,
                       tls_req_cert)
 
 
-class MsgId(list):
-    """Wrapper class to hold connection and msgid."""
+class AsynchronousMessage(object):
+    """A container for handling asynchronous LDAP responses.
 
-    pass
+    Some LDAP APIs, like `search_ext`, are asynchronous and return a message ID
+    when the server successfully initiates the operation. Clients can use this
+    message ID and the original connection to make the request to fetch the
+    results using `result3`.
+
+    This object holds the message ID, the original connection, and a callable
+    weak reference Finalizer that cleans up context managers specific to the
+    connection associated to the message ID.
+
+    :param message_id: The message identifier (str).
+    :param connection: The connection associated with the message identifier
+                       (ldappool.StateConnector).
+
+    The `clean` attribute is a callable that cleans up the context manager used
+    to create or return the connection object (weakref.finalize).
+
+    """
+
+    def __init__(self, message_id, connection, context_manager):
+        self.id = message_id
+        self.connection = connection
+        self.clean = weakref.finalize(
+            self, self._cleanup_connection_context_manager, context_manager
+        )
+
+    def _cleanup_connection_context_manager(self, context_manager):
+        context_manager.__exit__(None, None, None)
 
 
 def use_conn_pool(func):
@@ -791,15 +816,17 @@ class PooledLDAPHandler(LDAPHandler):
                    filterstr='(objectClass=*)', attrlist=None, attrsonly=0,
                    serverctrls=None, clientctrls=None,
                    timeout=-1, sizelimit=0):
-        """Return a ``MsgId`` instance, it asynchronous API.
+        """Return an AsynchronousMessage instance, it asynchronous API.
 
-        The ``MsgId`` instance can be safely used in a call to ``result3()``.
+        The AsynchronousMessage instance can be safely used in a call to
+        `result3()`.
 
-        To work with ``result3()`` API in predictable manner, the same LDAP
-        connection is needed which originally provided the ``msgid``. So, this
-        method wraps the existing connection and ``msgid`` in a new ``MsgId``
-        instance. The connection associated with ``search_ext`` is released
-        once last hard reference to the ``MsgId`` instance is freed.
+        To work with `result3()` API in predictable manner, the same LDAP
+        connection is needed which originally provided the `msgid`. So, this
+        method wraps the existing connection and `msgid` in a new
+        `AsynchronousMessage` instance. The connection associated with
+        `search_ext()` is released after `result3()` fetches the data
+        associated with `msgid`.
 
         """
         conn_ctxt = self._get_pool_connection()
@@ -812,30 +839,33 @@ class PooledLDAPHandler(LDAPHandler):
         except Exception:
             conn_ctxt.__exit__(*sys.exc_info())
             raise
-        res = MsgId((conn, msgid))
-        weakref.ref(res, functools.partial(conn_ctxt.__exit__,
-                                           None, None, None))
-        return res
+        return AsynchronousMessage(msgid, conn, conn_ctxt)
 
-    def result3(self, msgid, all=1, timeout=None,
+    def result3(self, message, all=1, timeout=None,
                 resp_ctrl_classes=None):
-        """Wait for and return the result.
+        """Wait for and return the result to an asynchronous message.
 
         This method returns the result of an operation previously initiated by
-        one of the LDAP asynchronous operation routines (eg search_ext()). It
-        returned an invocation identifier (a message id) upon successful
-        initiation of their operation.
+        one of the LDAP asynchronous operation routines (e.g., `search_ext()`).
+        The `search_ext()` method in python-ldap returns an invocation
+        identifier, or a message ID, upon successful initiation of the
+        operation by the LDAP server.
 
-        Input msgid is expected to be instance of class MsgId which has LDAP
-        session/connection used to execute search_ext and message idenfier.
+        The `message` is expected to be instance of class
+        `AsynchronousMessage`, which contains the message ID and the connection
+        used to make the original request.
 
-        The connection associated with search_ext is released once last hard
-        reference to MsgId object is freed. This will happen when function
-        which requested msgId and used it in result3 exits.
+        The connection and context manager associated with `search_ext()` are
+        cleaned up when message.clean() is called.
 
         """
-        conn, msg_id = msgid
-        return conn.result3(msg_id, all, timeout)
+        results = message.connection.result3(message.id, all, timeout)
+
+        # Now that we have the results from the LDAP server for the message, we
+        # don't need the the context manager used to create the connection.
+        message.clean()
+
+        return results
 
     @use_conn_pool
     def modify_s(self, conn, dn, modlist):
@@ -996,15 +1026,15 @@ class KeystoneLDAPHandler(LDAPHandler):
                 cookie='')
             page_ctrl_oid = ldap.controls.SimplePagedResultsControl.controlType
 
-        msgid = self.conn.search_ext(base,
-                                     scope,
-                                     filterstr,
-                                     attrlist,
-                                     serverctrls=[lc])
+        message = self.conn.search_ext(base,
+                                       scope,
+                                       filterstr,
+                                       attrlist,
+                                       serverctrls=[lc])
         # Endless loop request pages on ldap server until it has no data
         while True:
             # Request to the ldap server a page with 'page_size' entries
-            rtype, rdata, rmsgid, serverctrls = self.conn.result3(msgid)
+            rtype, rdata, rmsgid, serverctrls = self.conn.result3(message)
             # Receive the data
             res.extend(rdata)
             pctrls = [c for c in serverctrls
@@ -1020,11 +1050,11 @@ class KeystoneLDAPHandler(LDAPHandler):
                 if cookie:
                     # There is more data still on the server
                     # so we request another page
-                    msgid = self.conn.search_ext(base,
-                                                 scope,
-                                                 filterstr,
-                                                 attrlist,
-                                                 serverctrls=[lc])
+                    message = self.conn.search_ext(base,
+                                                   scope,
+                                                   filterstr,
+                                                   attrlist,
+                                                   serverctrls=[lc])
                 else:
                     # Exit condition no more data on server
                     break
diff --git a/releasenotes/notes/bug-1896125-b17a4d12730fe493.yaml b/releasenotes/notes/bug-1896125-b17a4d12730fe493.yaml
new file mode 100644
index 000000000..7a758b9ab
--- /dev/null
+++ b/releasenotes/notes/bug-1896125-b17a4d12730fe493.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    [`bug 1896125 <https://bugs.launchpad.net/keystone/+bug/1896125>`_]
+    Introduced more robust connection handling for asynchronous LDAP requests
+    to address memory leaks fetching data from LDAP backends with low page
+    sizes.