<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>Dylan Jay wrote:</div><blockquote type="cite">The details of how reproduce this are in the post by Maurits van Rees<br><br><a href="https://bugs.launchpad.net/ldapuserfolder/+bug/650371">https://bugs.launchpad.net/ldapuserfolder/+bug/650371</a><br></blockquote></div><br><div>The last comment (#6) shows that after a failed reconnect, the next call to reconnect() will get stuck in an infinite loop.</div><div>That is because the _pending_reconnect flag is not reset when the reconnection eventually fails.</div><div>The fix for that is pretty easy:</div><div><br></div><div><div>--- a/ldap/ldapobject.py<span class="Apple-tab-span" style="white-space:pre"> </span>2013-07-13 00:31:13.000000000 +0200</div><div>+++ b/ldap/ldapobject.py<span class="Apple-tab-span" style="white-space:pre"> </span>2013-07-13 00:54:10.000000000 +0200</div><div>@@ -792,6 +792,7 @@</div><div> ))</div><div> reconnect_counter = reconnect_counter-1</div><div> if not reconnect_counter:</div><div>+ self._pending_reconnect = 0</div><div> raise</div><div> if __debug__ and self._trace_level>=1:</div><div> self._trace_file.write('=> delay %s...\n' % (self._retry_delay))</div></div><div><br></div><div>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.</div><div>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.</div><div>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)").</div><div><br></div><div>So I made a patch with the following assumptions in mind:</div><div><ul><li>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.</li><li>When a reconnection is needed, new synchronous calls should be blocked until the reconnection is done.</li><li>Pending synchronous calls need to finish before a reconnection can happen.</li><li>Asynchronous calls can happen while a reconnection takes place, but we want to avoid getting AttributeError exceptions because self._l is not present anymore.</li><li>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.</li><li>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).</li></ul></div><div><div><br></div></div><div>This makes the patch unfortunately quite complex. Here is what is changed:</div><div><ul><li>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.</li><li>The call to start_tls_s done inside reconnect is done directly via SimpleLDAPObject to avoid recursively calling itself.</li><li>self._l is not deleted explicitly anymore to avoid AttributeError exceptions. It is replaced by another object anyway, so it will get deleted eventually.</li><li><br></li></ul></div><div>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.</div><div><br></div><div>Let me know if you have any comments on this.</div><div><br></div><div>Best regards,</div><div>Jonathan Giannuzzi</div><div></div></body></html>