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.
- "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 :)
- 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).
- 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.