Re: Proposed patch to support: draft-zeilenga-ldap-c-api-concurrency
by masarati＠aero.polimi.it
> Doug,
>
> a list of relatively quick comments, as I didn't go into too much detail
> in the analysis of each bit of your work.
>
> I think your patch contains at least three levels of modifications; they
> all make sense separately and incrementally, so I think we should revise
> them separately.
>
> 1) "cosmetic" cleanup (like wrapping locks in specific macros); they do
> not strictly relate to the support of
> draft-zeilenga-ldap-c-api-concurrency (but they make sense, and I like
> them); they should be considered first, regardless of the rest (I note
> that according to the way you define LDAP_NEXT_MSGID when #ifdef
> LDAP_R_COMPILE, you don't need to #ifdef its definition :)
>
> 2) a set of issues that fix the concurrent behavior of libldap relatively
> irrespective of the support of draft-zeilenga-ldap-c-api-concurrency (see
> for example the private discussion we had on fixing concurrent access to
> lconn_refcnt).
>
> 3) specific support to draft-zeilenga-ldap-c-api-concurrency, including
> but not limited to the addition of ldap_dup(), ldap_destroy() and related
> stuff.
>
> As you may have understood, I'm especially interested in (2), because it
> fixes a number of potential (or actual) issues in OpenLDAP code that uses
> libldap concurrently, like the proxy backends. So far, that code used
> libldap taking into account known weaknesses from a concurrency
> standpoint. For example, proxy backends protect handlers until a bind
> succeeds, to prevent the undesired simultaneous creation of default
> connections. However, some issues may slip in (as I believe is the case
> of ITS#6574).
>
> One thing I'd like to ask is: you introduce a few additional mutexes:
>
> ldapoptions -> ldo_mutex
>
> ldapcommon -> ldc_msgid_mutex, ldc_abandon_mutex
>
> in addition of the already existing
>
> ldap -> ld_conn_mutex, ld_req_mutex, ld_res_mutex
>
> that move to ldap_common.
>
> My concern is: can you guarantee that the occurrences of locking/unlocking
> those additional mutexes, combined to the existing ones, do not result in
> deadlocks? I mean: did you explicitly check all possible logical paths or
> so, or take measures to avoid this possibility?
Another mostly "style" comment: I think you shouldn't be polluting
libldap's space with thread implementation specific details, like
init.c:
#if defined(HAVE_THR)
#elif defined(HAVE_PTHREADS)
PTHREAD_MUTEX_INITIALIZER
#if !(defined(HAVE_THR) || defined(HAVE_PTHREADS))
and so. I understand PTHREAD_MUTEX_INITIALIZER is handy, but the whole
thing should remain confined in libldap_r specific files.
Also, but since it's a test tool it is not a priority, slapd-mtread.c
should use libldap_r and hide thread implementation details under
OpenLDAP's portable interface.
p.
