[python-ldap] Patch for ReconnectLDAPObject

[Dylan Jay]

Hi,

This patch seems to work really well. Any chance of getting it included in a release?

---
Dylan Jay
Technical Solutions Manager
PretaWeb: Multisite Performance Support
P: +612 80819071 | M: +61421477460 | twitter.com/pretaweb | linkedin.com/in/djay75



On 13/07/2013, at 10:26 AM, Jonathan Giannuzzi <jonathan at giannuzzi.be> wrote:

> 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
> <ldapobject_threadsafe.diff>