I think for postop_add and postop_del this check coould be done when memberof_oktodo() is called

Ludwig, setting the next 1.3.2 to the milestone.
Please feel free to adjust it to your plan.
Thanks!

Is this still an issue in the IPA upgrade (from which version to which)?

If so, could you put the 389-ds-base version and the stacktraces with the matched debuginfo (there are some functions which options and lines are missing...)?

Setting to FUTURE.

Comment by Ludwig:
global lock and lmdb redesign should handle this

Version: 389-ds-base-1.3.3.1-15.el7_1.x86_64

Replying to [comment:10 mreynolds]:
Thanks to Mark for picking up this ticket.
Setting Milestone to 1.3.4.3...

attachment
0001-Ticket-47931-memberOf-retrocl-deadlocks.patch

Hi Mark!

I'm reviewing your fix. First, I'm trying to understand the "fix description"... The deadlock among plug-ins is always hard to solve, isn't it? ;)
{{{
Bug Description: When concurrently updating multiple backends the
memberOf and retrocl plugins can deadlock on each
other. This is caused by the required retrocl lock,
and the db lock on the changenumber index in the
retrocl db.

Fix Description: Added scoping to the retrocl that allows subtrees/suffixes
to be included or excluded. Also moved the existing
memberOf scoping outside of its global lock.
}}}
So, the deadlock was caused by the memberof scoping inside the global lock and retrocl lock (retrocl_internal_lock or retrocl_cn_lock?)? And basically just moving the memberof scoping out solves the deadlock?

Thank you for your explanation, Mark! Now I understood the design of your fix.

2 minor issues...
1. Is 'i' supposed to get incremented?
{{{
645 / Check if the the entry include scope is a child of the sdn /
646 static Slapi_DN
647 memberof_scope_is_child_of_dn(MemberOfConfig config, Slapi_DN sdn)
648 {
649 int i = 0;
650
651 while(config->entryScopes && config->entryScopes[i]){
652 if(slapi_sdn_issuffix(config->entryScopes[i], sdn)){
653 return config->entryScopes[i];
654 }
655 }
656 return NULL;
657 }
}}}
2. Is retro changelog applicable to the dynamic plug-in? If yes, retrocl_includes|excludes could leak?
{{{
48 static Slapi_DN retrocl_includes = NULL;
49 static Slapi_DN retrocl_excludes = NULL;
391 retrocl_excludes = (Slapi_DN )slapi_ch_calloc(sizeof(Slapi_DN *),num_vals+1);
411 retrocl_includes = (Slapi_DN )slapi_ch_calloc(sizeof(Slapi_DN ),num_vals+1);
}}}

Replying to [comment:16 nhosoi]:

Thank you for your explanation, Mark! Now I understood the design of your fix.

2 minor issues...
1. Is 'i' supposed to get incremented?
{{{
645 / Check if the the entry include scope is a child of the sdn /
646 static Slapi_DN
647 memberof_scope_is_child_of_dn(MemberOfConfig config, Slapi_DN *sdn)
648 {
649 int i = 0;
650
651 while(config->entryScopes && config->entryScopes[i]){
652 if(slapi_sdn_issuffix(config->entryScopes[i], sdn)){
653 return config->entryScopes[i];
654 }
655 }
656 return NULL;
657 }
}}}

Good catch

1. Is retro changelog applicable to the dynamic plug-in? If yes, retrocl_includes|excludes could leak?
   {{{
   48 static Slapi_DN retrocl_includes = NULL;
   49 static Slapi_DN retrocl_excludes = NULL;
   391 retrocl_excludes = (Slapi_DN )slapi_ch_calloc(sizeof(Slapi_DN *),num_vals+1);
   411 retrocl_includes = (Slapi_DN )slapi_ch_calloc(sizeof(Slapi_DN *),num_vals+1);
   }}}

This is freed in the plugin stop function, and that's how dynamic plugins work. Any change to plugin's config triggers the plugin to be restarted. So this is not leaked.

revision
0001-Ticket-47931-memberOf-retrocl-deadlocks.2.patch

New patch attached...

Hi Mark, thanks for waiting. Please push it!

6b138a2..fd959ac master -> master
commit fd959ac
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Aug 4 12:19:31 2015 -0400

e62b481..d810847 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit d810847

Coverity issues found, reopening...

Fix coverity issues
0002-Ticket-47931-Fix-coverity-issues.patch

fd959ac..5daea97 master -> master
commit 5daea97
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Aug 10 10:42:40 2015 -0400

d810847..9a0047e 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 9a0047e

08eabd5..f86dab3 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit f86dab3

f86dab3..e6343e6 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit e6343e6

Debug logging was accidentally left in the source - it has now been removed:

29c09a5..db7153f master -> master
commit db7153f
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Aug 19 10:03:50 2015 -0400

2c5e0d5..1781280 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 1781280

895dc4f..5c48f28 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 5c48f28

lib389 test script
0001-Ticket-47931-lib389-test-memberof-and-retrocl-deadlo.patch

Replying to [comment:27 mreynolds]:
Probably, a silly question... :p Could you tell me where test_ticket47931 is called?

Replying to [comment:28 nhosoi]:

Replying to [comment:27 mreynolds]:
Probably, a silly question... :p Could you tell me where test_ticket47931 is called?

Simon modified create-ticket.py to make new ticket tests more compliant with the pytest way. So now in the main function we call

{{{
pytest.main("-s %s" % CURRENT_FILE)
}}}

Which executes all the functions that start with "test".

I see. Thanks!

Thanks Noriko!

4277f2b..48470e7 master -> master
commit 48470e7
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Aug 28 15:01:58 2015 -0400

8c9c2fc..22e4de7 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 22e4de7

f10e2e6..e7d3b2f 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit e7d3b2f

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1262

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.