[python-ldap] Patch for ReconnectLDAPObject

[Jonathan Giannuzzi]

Dylan Jay wrote:
> The details of how reproduce this are in the post by Maurits van Rees
> 
> https://bugs.launchpad.net/ldapuserfolder/+bug/650371

The last comment (#6) shows that after a failed reconnect, the next call to reconnect() will get stuck in an infinite loop.
That is because the _pending_reconnect flag is not reset when the reconnection eventually fails.
The fix for that is pretty easy:

--- a/ldap/ldapobject.py	2013-07-13 00:31:13.000000000 +0200
+++ b/ldap/ldapobject.py	2013-07-13 00:54:10.000000000 +0200
@@ -792,6 +792,7 @@
           ))
         reconnect_counter = reconnect_counter-1
         if not reconnect_counter:
+          self._pending_reconnect = 0
           raise
         if __debug__ and self._trace_level>=1:
           self._trace_file.write('=> delay %s...\n' % (self._retry_delay))

But that made me question why that flag was needed at all. I assumed it was in order to prevent multiple threads from calling reconnect at the same time.
Unfortunately a quick test (two threads calling search_s in a loop, then kill the connection while they are running) shows that the reconnection is far from being thread-safe.
The main problem is that if a call is being made while the reconnect is being done, the internal structures might be invalid (because of "del self._l" and "SimpleLDAPObject.unbind_s(self)").

So I made a patch with the following assumptions in mind:
If multiple synchronous calls fail at the same time, only one reconnect should happen. This would avoid having a successful reconnect followed by another useless reconnect.
When a reconnection is needed, new synchronous calls should be blocked until the reconnection is done.
Pending synchronous calls need to finish before a reconnection can happen.
Asynchronous calls can happen while a reconnection takes place, but we want to avoid getting AttributeError exceptions because self._l is not present anymore.
Moreover, if asynchronous calls were blocked during a reconnection, we might wait indefinitely for a result with an unknown msgid. Asynchronous calls should thus be prepared to handle "LDAP connection invalid" LDAPError exceptions.
One big lock for all synchronous calls (i.e. in _apply_method_s) would have a big performance impact (around 60% slower in one of my test cases).

This makes the patch unfortunately quite complex. Here is what is changed:
It mostly uses thread conditions to achieve those goals: one condition to control whether a reconnection is pending and another to control the amount of pending synchronous calls. This part of the patch is well commented.
The call to start_tls_s done inside reconnect is done directly via SimpleLDAPObject to avoid recursively calling itself.
self._l is not deleted explicitly anymore to avoid AttributeError exceptions. It is replaced by another object anyway, so it will get deleted eventually.

I tested the patch a lot, but only under Python 2.7. AFAIK it should also work in previous releases, but it should be tested.

Let me know if you have any comments on this.

Best regards,
Jonathan Giannuzzi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-ldap/attachments/20130713/5d880f48/attachment-0002.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ldapobject_threadsafe.diff
Type: application/octet-stream
Size: 6743 bytes
Desc: not available
URL: <http://mail.python.org/pipermail/python-ldap/attachments/20130713/5d880f48/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-ldap/attachments/20130713/5d880f48/attachment-0003.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4145 bytes
Desc: not available
URL: <http://mail.python.org/pipermail/python-ldap/attachments/20130713/5d880f48/attachment-0001.bin>