On 4/30/20 11:34 AM, Joe Orton wrote:
> On Wed, Apr 29, 2020 at 03:09:28PM -0400, Eric Covener wrote:
>> On Wed, Apr 29, 2020 at 2:43 PM Ruediger Pluem <[email protected]> wrote:
>>> Hm. Don't we need to do the apr_pool_clear(ldc->rebind_pool); in any case
>>> if ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON or
>>> does this only makes sense if ldc->ldap != NULL?
>>> And can we still do an ldap_unbind_s(ldc->ldap); if we did an
>>> apr_ldap_rebind_remove(ldc->ldap); before?
>>
>> I see what you mean. If we lost the ldc->ldap some other way, anything
>> allocated from ldc->rebind_pool is leaked because the LDAP key can't
>> be looked up in the xref linked list.
>> We would likely have linked list growth in that case too.
>
> Shouldn't clearing the pool here be sufficient anyway? Since there is a
> cleanup registered by apr_ldap_rebind_add() which calls
> apr_ldap_rebind_remove() with the original LDAP * pointer value it looks
> safe, and it avoids entering _rebind_remove twice.
>
> Doing it before the ldap_unbind() call so that LDAP * pointer is not a
> dangling pointer seems better.
>
> The underlying problem here is a performance issue which looked like
> linked list growth so actually with:
>
> a) threaded MPM and large number of threads,
> b) each thread may have its own LDAP * and an associated rebind entry (is
> that really right?!)
> c) on unbind each thread walks the linked list w/mutex held
>
> which looks quite painful regardless, and doing (c) twice is clearly
> worse. So as well as changing this the indexed linked list should
> really be a hash table?
Not looked at the problem further, but there is now a PR for this:
https://bz.apache.org/bugzilla/show_bug.cgi?id=64414
Maybe this revives the discussion :-)
Regards
Rüdiger
