attachment
0001-Ticket-48752-Page-result-search-should-return-empty-.patch

attachment
0002-Ticket-48752-Page-result-search-should-return-empty-.patch

Hi Thierry,

Instead of checking the search result count in pagedresults_set_response_control as you proposed:
{{{
ldap/servers/slapd/pagedresults.c

diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c
index d394dab..c6d6276 100644

a b pagedresults_set_response_control( Slapi_PBlock pb, int iscritical,
244 244 }
245 245
246 246 / begin sequence, payload, end sequence /
247 if (current_search_count < 0) {
247 if (current_search_count <= 0) {
248 248 cookie_str = slapi_ch_strdup("");
249 slapi_pblock_set ( pb, SLAPI_PAGED_RESULTS_COOKIE, 0 );
249 250 } else {
250 251 cookie_str = slapi_ch_smprintf("%d", index);
252 slapi_pblock_set ( pb, SLAPI_PAGED_RESULTS_COOKIE, index );
251 253 }
252 254 ber_printf ( ber, "{io}", estimate, cookie_str, strlen(cookie_str) );
253 255 if ( ber_flatten ( ber, &berval ) != LDAP_SUCCESS )
}}}
how about checking in the caller side who has the next_be info like this?
{{{
diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c
index 41a1b37..d870747 100644
--- a/ldap/servers/slapd/opshared.c
+++ b/ldap/servers/slapd/opshared.c
@@ -699,7 +699,7 @@ op_shared_search (Slapi_PBlock pb, int send_result)
}
pagedresults_unlock(pb->pb_conn, pr_idx);

• if (PAGEDRESULTS_SEARCH_END == pr_stat) {
• if ((PAGEDRESULTS_SEARCH_END == pr_stat) || (0 == pnentries)) {
  / no more entries to send in the backend /
  if (NULL == next_be) {
  / no more entries && no more backends /
  @@ -869,7 +869,7 @@ op_shared_search (Slapi_PBlock *pb, int send_result)

       curr_search_count = pnentries;
       slapi_pblock_get(pb, SLAPI_SEARCH_RESULT_SET, &sr);
  

  • if (PAGEDRESULTS_SEARCH_END == pr_stat) {
  • if ((PAGEDRESULTS_SEARCH_END == pr_stat) || (0 == pnentries)) {
    / no more entries, but at least another backend /
    PR_EnterMonitor(pb->pb_conn->c_mutex);
    pagedresults_set_search_result(pb->pb_conn, operation, NULL, 1, pr_idx);
    }}}
    This way if the search result count is 0 AND there's no more backend, -1 current_search_count is passed to pagedresults_set_response_control?

adding reference to the Red Hat bz 1326077

Hi Noriko,

The fix https://fedorahosted.org/389/ticket/48752#comment:5 looks more like a final fix than the one proposed https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch.

In fact https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch did not take into consideration PR over multiple backend. It was just proposed for a test with only one backend.

For information, we managed to reproduce twice a week. And proposed a test hotfix with https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch. It will be applied and run over a week to check if it fixes the problem and then check with the additional logs that we can have condition where there is 0 returned entry but not empty cookie.

Replying to [comment:7 tbordaz]:

For information, we managed to reproduce twice a week. And proposed a test hotfix with https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch. It will be applied and run over a week to check if it fixes the problem and then check with the additional logs that we can have condition where there is 0 returned entry but not empty cookie.

Thank you for the latest status, Thierry! If your patch fixes the problem, the concept is proved correct. I hope we can come to the conclusion and close this case... ;)

I'm happy with the look of the code for this fix. How did the test go? Is this ready to be merged do you think?

Replying to [comment:9 firstyear]:

I'm happy with the look of the code for this fix. How did the test go? Is this ready to be merged do you think?

Hi William,

The ticket comes from SSSD cascades of requests.
SSSD team was unable to reproduce and I was also unable to reproduce with direct PR searches.

Now reviewing SSSD code, it looks the only case it could cascade is that we return a valid cookie (non empty). As in the logs we can see that when SSSD cascade, the search returned no entry we just "guess" we could end in such situation: no returned entry + valid cookie.

From time to time, some SSSD users are hitting this issue but so far I have been enable to let them test a hotfix with that fix.

If the fix looks ok, we may decide to push it even if we are unable to verify it addresses the SSSD cascade. Or we may decide to wait for an other user being able to verify it fixes the cascades.

Hi Thierry,

Did you have a chance to try the diff in the comment (https://fedorahosted.org/389/ticket/48752#comment:5)? I think the patch you attached to this ticket is not a finalized one (since it is not supporting the multiple backends). Could it be possible to try the one in #comment:5? (Or I'd better come up with the patch?)

Thanks!

git patch file (master) -- reviewed 0002-Ticket-48752-Page-result-search-should-return-empty-.patch and applied the proposal in #comment:5 as well as fixed compiler warnings.
0001-Ticket-48752-Page-result-search-should-return-empty-.2.patch

Per discussion at the scrum, pushing the patch on behalf of Thierry (Thanks!)

Pushed to master:
ba3b844..f215fb6 master -> master
commit f215fb6

Pushed to 389-ds-base-1.3.4:
14bc6ce..ca50d1c 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit ca50d1c

attachment
0001-Ticket-48752-Add-CI-test.patch

attachment
0001-Ticket-48752-Add-CI-test.2.patch

Small, but important change to the test case (try-finally block + wrap the generator).
Please, review the second patch.

To ssh://git.fedorahosted.org/git/389/ds.git

Pushed to master:
0532a63..fc97900 master -> master
commit fc97900
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Jul 14 14:14:12 2016 +0200

Pushed to 389-ds-base-1.3.4:

42fcf97..7738a00 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 7738a00
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Jul 14 14:14:12 2016 +0200

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/1812

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.