C extension reference leak testing

Add documentation how to run refleak tests with pydebug build of
CPython and pytest-refleak. Refleak testing runs each unit test multiple
times. Some small changes are required to clean up after each test cycle
and to avoid extra references.

* Only create logging structure for SlapdObject once. The logging
  framework creates reference cycles that cause false positives.
* Clean up and remove atexit handlers in SlapdObject.stop()
* Remove new entries at the end of each test, either with an
  explicit delete_s() or an write area, which is removed in tearDown().

https://github.com/python-ldap/python-ldap/pull/41
Signed-off-by: Christian Heimes <cheimes@redhat.com>

INSTALL

@@ -24,3 +24,37 @@ Quick build instructions:
     edit setup.cfg (see Build/ for platform-specific examples)
     python setup.py build
     python setup.py install
+
+--------------------
+Reference leak tests
+--------------------
+
+Reference leak tests require a pydebug build of CPython and pytest with
+pytest-leaks plugin. A pydebug build has a global reference counter, which
+keeps track of all reference increments and decrements. The leak plugin runs
+each tests multiple times and checks if the reference count increases.
+
+Download and compile pydebug build
+----------------------------------
+
+- curl -O https://www.python.org/ftp/python/3.6.3/Python-3.6.3.tar.xz
+- tar xJf Python-3.6.3.tar.xz
+- cd Python-3.6.3
+- ./configure --with-pydebug
+- make
+
+Create virtual env
+------------------
+
+- ./python -m venv /tmp/refleak
+- /tmp/refleak/bin/pip install pytest pytest-leaks
+
+Run refleak tests
+-----------------
+
+- cd path/to/python-ldap
+- /tmp/refleak/bin/pip install --upgrade .
+- /tmp/refleak/bin/pytest -v -R: Tests/t_*.py
+
+Run ``/tmp/refleak/bin/pip install --upgrade .`` every time a file outside
+of ``Tests/`` is modified.

Lib/slapdtest.py

@@ -117,6 +117,7 @@ class SlapdObject(object):
     else:
         SCHEMADIR = None
     PATH_LDAPADD = os.path.join(BINDIR, 'ldapadd')
+    PATH_LDAPDELETE = os.path.join(BINDIR, 'ldapdelete')
     PATH_LDAPMODIFY = os.path.join(BINDIR, 'ldapmodify')
     PATH_LDAPWHOAMI = os.path.join(BINDIR, 'ldapwhoami')
     PATH_SLAPD = os.environ.get('SLAPD', os.path.join(SBINDIR, 'slapd'))
@@ -125,8 +126,10 @@ class SlapdObject(object):
     # time in secs to wait before trying to access slapd via LDAP (again)
     _start_sleep = 1.5
 
+    # create loggers once, multiple calls mess up refleak tests
+    _log = combined_logger('python-ldap-test')
+
     def __init__(self):
-        self._log = combined_logger('python-ldap-test')
         self._proc = None
         self._port = self._avail_tcp_port()
         self.server_id = self._port % 4096
@@ -189,9 +192,11 @@ def _avail_tcp_port(self):
         find an available port for TCP connection
         """
         sock = socket.socket()
-        sock.bind((self.local_host, 0))
-        port = sock.getsockname()[1]
-        sock.close()
+        try:
+            sock.bind((self.local_host, 0))
+            port = sock.getsockname()[1]
+        finally:
+            sock.close()
         self._log.info('Found available port %d', port)
         return port
 
@@ -316,6 +321,15 @@ def stop(self):
             self._proc.terminate()
             self.wait()
         self._cleanup_rundir()
+        if hasattr(atexit, 'unregister'):
+            # Python 3
+            atexit.unregister(self.stop)
+        elif hasattr(atexit, '_exithandlers'):
+            # Python 2, can be None during process shutdown
+            try:
+                atexit._exithandlers.remove(self.stop)
+            except ValueError:
+                pass
 
     def restart(self):
         """
@@ -358,15 +372,22 @@ def _cli_popen(self, ldapcommand, extra_args=None, ldap_uri=None, stdin_data=Non
             '-H', ldap_uri or self.ldapi_uri,
         ] + self._cli_auth_args() + (extra_args or [])
         self._log.debug('Run command: %r', ' '.join(args))
-        proc = subprocess.Popen(args, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
+        proc = subprocess.Popen(
+            args, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE
+        )
         self._log.debug('stdin_data=%r', stdin_data)
         stdout_data, stderr_data = proc.communicate(stdin_data)
         if stdout_data is not None:
             self._log.debug('stdout_data=%r', stdout_data)
         if stderr_data is not None:
             self._log.debug('stderr_data=%r', stderr_data)
         if proc.wait() != 0:
-            raise RuntimeError('ldapadd process failed')
+            raise RuntimeError(
+                '{!r} process failed:\n{!r}\n{!r}'.format(
+                    args, stdout_data, stderr_data
+                )
+            )
         return stdout_data, stderr_data
 
     def ldapwhoami(self, extra_args=None):
@@ -389,6 +410,17 @@ def ldapmodify(self, ldif, extra_args=None):
         self._cli_popen(self.PATH_LDAPMODIFY, extra_args=extra_args,
                         stdin_data=ldif.encode('utf-8'))
 
+    def ldapdelete(self, dn, recursive=False, extra_args=None):
+        """
+        Runs ldapdelete on this slapd instance, deleting 'dn'
+        """
+        if extra_args is None:
+            extra_args = []
+        if recursive:
+            extra_args.append('-r')
+        extra_args.append(dn)
+        self._cli_popen(self.PATH_LDAPDELETE, extra_args=extra_args)
+
 
 class SlapdTestCase(unittest.TestCase):
     """

Tests/t_cext.py

@@ -51,6 +51,40 @@ def setUpClass(cls):
             ])
         )
 
+    def setUp(self):
+        super(TestLdapCExtension, self).setUp()
+        self._writesuffix = None
+
+    def tearDown(self):
+        # cleanup test subtree
+        if self._writesuffix is not None:
+            self.server.ldapdelete(self._writesuffix, recursive=True)
+        super(TestLdapCExtension, self).tearDown()
+
+    @property
+    def writesuffix(self):
+        """Initialize writesuffix on demand
+
+        Creates a clean subtree for tests that write to slapd. ldapdelete
+        is not able to delete a Root DSE, therefore we need a temporary
+        work space.
+
+        :return: DN
+        """
+        if self._writesuffix is not None:
+            return self._writesuffix
+        self._writesuffix = 'ou=write tests,%s' % self.server.suffix
+        # Add writeable subtree
+        self.server.ldapadd(
+            "\n".join([
+                'dn: ' + self._writesuffix,
+                'objectClass: organizationalUnit',
+                'ou:' + self._writesuffix.split(',')[0][3:],
+                ''
+            ])
+        )
+        return self._writesuffix
+
     def _open_conn(self, bind=True):
         """
         Starts a server, and returns a LDAPObject bound to it
@@ -285,7 +319,7 @@ def test_add(self):
         """
         l = self._open_conn()
         m = l.add_ext(
-            "cn=Foo," + self.server.suffix,
+            "cn=Foo," + self.writesuffix,
             [
                 ('objectClass', b'organizationalRole'),
                 ('cn', b'Foo'),
@@ -299,7 +333,7 @@ def test_add(self):
         self.assertEqual(msgid, m)
         self.assertEqual(ctrls, [])
         # search for it back
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=Foo)')
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=Foo)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
         # Expect to get the objects
         self.assertEqual(result, _ldap.RES_SEARCH_RESULT)
@@ -309,7 +343,7 @@ def test_add(self):
         self.assertEqual(
             pmsg[0],
             (
-                'cn=Foo,'+self.server.suffix,
+                'cn=Foo,'+self.writesuffix,
                 {
                     'objectClass': [b'organizationalRole'],
                     'cn': [b'Foo'],
@@ -324,7 +358,7 @@ def test_compare(self):
         """
         l = self._open_conn()
         # first, add an object with a field we can compare on
-        dn = "cn=CompareTest," + self.server.suffix
+        dn = "cn=CompareTest," + self.writesuffix
         m = l.add_ext(
             dn,
             [
@@ -378,7 +412,7 @@ def test_delete_no_such_object(self):
     def test_delete(self):
         l = self._open_conn()
         # first, add an object we will delete
-        dn = "cn=Deleteme,"+self.server.suffix
+        dn = "cn=Deleteme,"+self.writesuffix
         m = l.add_ext(
             dn,
             [
@@ -402,7 +436,7 @@ def test_modify_no_such_object(self):
 
         # try deleting an object that doesn't exist
         m = l.modify_ext(
-            "cn=DoesNotExist,"+self.server.suffix,
+            "cn=DoesNotExist,"+self.writesuffix,
             [
                 (_ldap.MOD_ADD, 'description', [b'blah']),
             ]
@@ -439,7 +473,7 @@ def test_modify(self):
         """
         l = self._open_conn()
         # first, add an object we will delete
-        dn = "cn=AddToMe,"+self.server.suffix
+        dn = "cn=AddToMe,"+self.writesuffix
         m = l.add_ext(
             dn,
             [
@@ -465,7 +499,7 @@ def test_modify(self):
         self.assertEqual(msgid, m)
         self.assertEqual(ctrls, [])
         # search for it back
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=AddToMe)')
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=AddToMe)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
         # Expect to get the objects
         self.assertEqual(result, _ldap.RES_SEARCH_RESULT)
@@ -479,7 +513,7 @@ def test_modify(self):
 
     def test_rename(self):
         l = self._open_conn()
-        dn = "cn=RenameMe,"+self.server.suffix
+        dn = "cn=RenameMe,"+self.writesuffix
         m = l.add_ext(
             dn,
             [
@@ -500,16 +534,16 @@ def test_rename(self):
         self.assertEqual(ctrls, [])
 
         # make sure the old one is gone
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=RenameMe)')
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=RenameMe)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
         self.assertEqual(result, _ldap.RES_SEARCH_RESULT)
         self.assertEqual(len(pmsg), 0) # expect no results
         self.assertEqual(msgid, m)
         self.assertEqual(ctrls, [])
 
         # check that the new one looks right
-        dn2 = "cn=IAmRenamed,"+self.server.suffix
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)')
+        dn2 = "cn=IAmRenamed,"+self.writesuffix
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
         self.assertEqual(result, _ldap.RES_SEARCH_RESULT)
         self.assertEqual(msgid, m)
@@ -519,7 +553,7 @@ def test_rename(self):
         self.assertEqual(pmsg[0][1]['cn'], [b'IAmRenamed'])
 
         # create the container
-        containerDn = "ou=RenameContainer,"+self.server.suffix
+        containerDn = "ou=RenameContainer,"+self.writesuffix
         m = l.add_ext(
             containerDn,
             [
@@ -542,18 +576,18 @@ def test_rename(self):
         self.assertEqual(ctrls, [])
 
         # make sure dn2 is gone
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)')
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
         self.assertEqual(result, _ldap.RES_SEARCH_RESULT)
         self.assertEqual(len(pmsg), 0) # expect no results
         self.assertEqual(msgid, m)
         self.assertEqual(ctrls, [])
 
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(objectClass=*)')
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(objectClass=*)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
 
         # make sure dn3 is there
-        m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamedAgain)')
+        m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamedAgain)')
         result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout)
         self.assertEqual(result, _ldap.RES_SEARCH_RESULT)
         self.assertEqual(msgid, m)
@@ -595,7 +629,7 @@ def test_whoami_after_unbind(self):
     def test_passwd(self):
         l = self._open_conn()
         # first, create a user to change password on
-        dn = "cn=PasswordTest," + self.server.suffix
+        dn = "cn=PasswordTest," + self.writesuffix
         m = l.add_ext(
             dn,
             [

Tests/t_edit.py

@@ -83,6 +83,12 @@ def test_add_object(self):
             ("cn=Added,ou=Container," + base,
                 {'cn': [b'Added'], 'objectClass': [b'organizationalRole']}),
         ])
+        # Delete object
+        self.ldap.delete_s(dn)
+        result = self.ldap.search_s(
+            base, ldap.SCOPE_SUBTREE, '(cn=Added)', ['*']
+        )
+        self.assertEqual(result, [])
 
 
 if __name__ == '__main__':