[Freeipa-devel] [PATCH 487] ldap: Make ldap2 connection management thread-safe again

[Tomas Babej]

On 09/02/2015 04:47 PM, Jan Cholasta wrote:
> On 2.9.2015 16:20, thierry bordaz wrote:
>> On 09/02/2015 03:16 PM, Jan Cholasta wrote:
>>> On 2.9.2015 14:51, Martin Basti wrote:
>>>>
>>>>
>>>> On 09/02/2015 02:32 PM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patch fixes
>>>>> <https://fedorahosted.org/freeipa/ticket/5268>.
>>>>>
>>>>> Honza
>>>>>
>>>>>
>>>>>
>>>>
>>>> This patch needs a big rebase to ipa-4-2 branch
>>>
>>> Patch attached.
>>>
>>>
>>>
>> Hello,
>>
>> Two minors questions. LDAPClient close/__del__/__exit__ are now just
>> resetting self._conn without disconnecting the connection.
> 
> They do the same even without the patch, "object.__setattr__(self,
> '_conn', None)" is effectively the same as "self._conn = None".
> 
>> Only ldap2.close() disconnect the connection. Could it be a risk to see
>> connection leaks with __del__ or __exit__ ?
> 
> This behavior is unchanged, and so far no one complained about
> connection leaks.
> 
>>
>> Also in the fix there is:
>>
>> @@ -118,10 +115,11 @@ class ldap2(CrudBackend, LDAPClient):
>>           if debug_level:
>>               _ldap.set_option(_ldap.OPT_DEBUG_LEVEL, debug_level)
>>
>> -        LDAPClient._connect(self)
>> -        conn = self._conn
>> +        client = LDAPClient(self.ldap_uri,
>> +                           
>> force_schema_updates=self._force_schema_updates)
>> +        conn = client._conn
>>
>>
>> Is it the same as 'conn = client.conn()' ?
> 
> No. It's the same as "conn = client.conn", but I'd like to get rid of
> LDAPClient.conn in the future (internal attributes should not be
> public), hence the use of self._conn.
> 
>>
>> Thanks
>> thierry
>>
> 

This fixes the connection issue, ACK.

Tomas