From 3b7fb4319f050f4e10593138d6b352cc09c5aad5 Mon Sep 17 00:00:00 2001 From: Noriko Hosoi Date: Thu, 4 Jun 2015 16:27:05 -0700 Subject: [PATCH 332/333] Ticket #48192 - Individual abandoned simple paged results request has no chance to be cleaned up Description: When allocating a slot in simple paged results array stashed in a connection in pagedresults_parse_control_value, a new code is added to scan the array if the existing slot is timed out or not. If it is, the slot is released and a search results is released if it is attached. Also, if a request is abandoned, instead of returning a valid cookie, it changed to return an empty cookie to inform the client the request was not successfully completed. https://fedorahosted.org/389/ticket/48192 Reviewed by rmeggins@redhat.com (Thank you, Rich!!) (cherry picked from commit 9035617b50cec984a8ccfea5a0f37d4d0b11f9cd) --- ldap/servers/slapd/opshared.c | 41 +++++++------- ldap/servers/slapd/pagedresults.c | 110 ++++++++++++++++++++++++-------------- 2 files changed, 89 insertions(+), 62 deletions(-) diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c index d446f50..7b160dd 100644 --- a/ldap/servers/slapd/opshared.c +++ b/ldap/servers/slapd/opshared.c @@ -510,8 +510,7 @@ op_shared_search (Slapi_PBlock *pb, int send_result) rc = pagedresults_parse_control_value(pb, ctl_value, &pagesize, &pr_idx, be); /* Let's set pr_idx even if it fails; in case, pr_idx == -1. */ slapi_pblock_set(pb, SLAPI_PAGED_RESULTS_INDEX, &pr_idx); - if ((LDAP_SUCCESS == rc) || - (LDAP_CANCELLED == rc) || (0 == pagesize)) { + if ((LDAP_SUCCESS == rc) || (LDAP_CANCELLED == rc) || (0 == pagesize)) { unsigned int opnote = SLAPI_OP_NOTE_SIMPLEPAGED; op_set_pagedresults(operation); pr_be = pagedresults_get_current_be(pb->pb_conn, pr_idx); @@ -538,9 +537,8 @@ op_shared_search (Slapi_PBlock *pb, int send_result) } slapi_pblock_set( pb, SLAPI_OPERATION_NOTES, &opnote ); if ((LDAP_CANCELLED == rc) || (0 == pagesize)) { - /* paged-results-request was abandoned */ - pagedresults_set_response_control(pb, 0, estimate, - curr_search_count, pr_idx); + /* paged-results-request was abandoned; making an empty cookie. */ + pagedresults_set_response_control(pb, 0, estimate, -1, pr_idx); send_ldap_result(pb, 0, NULL, "Simple Paged Results Search abandoned", 0, NULL); @@ -567,10 +565,21 @@ op_shared_search (Slapi_PBlock *pb, int send_result) /* adjust time and size limits */ compute_limits (pb); - - /* call the pre-search plugins. if they succeed, call the backend - search function. then call the post-search plugins. */ + /* set the timelimit to clean up the too-long-lived-paged results requests */ + if (op_is_pagedresults(operation)) { + time_t optime, time_up; + int tlimit; + slapi_pblock_get( pb, SLAPI_SEARCH_TIMELIMIT, &tlimit ); + slapi_pblock_get( pb, SLAPI_OPINITIATED_TIME, &optime ); + time_up = (tlimit==-1 ? -1 : optime + tlimit); /* -1: no time limit */ + pagedresults_set_timelimit(pb->pb_conn, pb->pb_op, time_up, pr_idx); + } + + /* + * call the pre-search plugins. if they succeed, call the backend + * search function. then call the post-search plugins. + */ /* ONREPL - should regular plugin be called for internal searches ? */ if (plugin_call_plugins(pb, SLAPI_PLUGIN_PRE_SEARCH_FN) == 0) { @@ -635,16 +644,6 @@ op_shared_search (Slapi_PBlock *pb, int send_result) } } - /* set the timelimit to clean up the too-long-lived-paged results requests */ - if (op_is_pagedresults(operation)) { - time_t optime, time_up; - int tlimit; - slapi_pblock_get( pb, SLAPI_SEARCH_TIMELIMIT, &tlimit ); - slapi_pblock_get( pb, SLAPI_OPINITIATED_TIME, &optime ); - time_up = (tlimit==-1 ? -1 : optime + tlimit); /* -1: no time limit */ - pagedresults_set_timelimit(pb->pb_conn, pb->pb_op, time_up, pr_idx); - } - /* PAR: now filters have been rewritten, we can assign plugins to work on them */ index_subsys_assign_filter_decoders(pb); @@ -873,10 +872,7 @@ op_shared_search (Slapi_PBlock *pb, int send_result) slapi_pblock_get(pb, SLAPI_SEARCH_RESULT_SET, &sr); if (PAGEDRESULTS_SEARCH_END == pr_stat) { if (sr) { /* in case a left over sr is found, clean it up */ - PR_Lock(pb->pb_conn->c_mutex); - pagedresults_set_search_result(pb->pb_conn, operation, NULL, 1, pr_idx); - be->be_search_results_release(&sr); - PR_Unlock(pb->pb_conn->c_mutex); + pagedresults_free_one(pb->pb_conn, operation, pr_idx); } if (NULL == next_be) { /* no more entries && no more backends */ @@ -1299,7 +1295,6 @@ iterate(Slapi_PBlock *pb, Slapi_Backend *be, int send_result, operation_out_of_disk_space(); } pr_stat = PAGEDRESULTS_SEARCH_END; - pagedresults_set_timelimit(pb->pb_conn, pb->pb_op, 0, pr_idx); rval = -1; done = 1; continue; diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c index 327da54..402dd10 100644 --- a/ldap/servers/slapd/pagedresults.c +++ b/ldap/servers/slapd/pagedresults.c @@ -41,6 +41,27 @@ #include "slap.h" +/* helper function to clean up one prp slot */ +static void +_pr_cleanup_one_slot(PagedResults *prp) +{ + PRLock *prmutex = NULL; + if (!prp) { + return; + } + if (prp->pr_current_be && prp->pr_current_be->be_search_results_release) { + /* sr is left; release it. */ + prp->pr_current_be->be_search_results_release(&(prp->pr_search_result_set)); + } + /* clean up the slot */ + if (prp->pr_mutex) { + /* pr_mutex is reused; back it up and reset it. */ + prmutex = prp->pr_mutex; + } + memset(prp, '\0', sizeof(PagedResults)); + prp->pr_mutex = prmutex; +} + /* * Parse the value from an LDAPv3 "Simple Paged Results" control. They look * like this: @@ -65,6 +86,7 @@ pagedresults_parse_control_value( Slapi_PBlock *pb, Connection *conn = pb->pb_conn; Operation *op = pb->pb_op; BerElement *ber = NULL; + PagedResults *prp = NULL; LDAPDebug0Args(LDAP_DEBUG_TRACE, "--> pagedresults_parse_control_value\n"); if ( NULL == conn || NULL == op || NULL == pagesize || NULL == index ) { @@ -117,13 +139,31 @@ pagedresults_parse_control_value( Slapi_PBlock *pb, } *index = maxlen; /* the first position in the new area */ } else { - for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++) { - if (!conn->c_pagedresults.prl_list[i].pr_current_be) { - conn->c_pagedresults.prl_list[i].pr_current_be = be; + time_t ctime = current_time(); + prp = conn->c_pagedresults.prl_list; + for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++, prp++) { + if (!prp->pr_current_be) { /* unused slot; take it */ + prp->pr_current_be = be; + *index = i; + break; + } else if (((prp->pr_timelimit > 0) && (ctime < prp->pr_timelimit)) || /* timelimit exceeded */ + (prp->pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED) /* abandoned */) { + _pr_cleanup_one_slot(prp); + conn->c_pagedresults.prl_count--; + prp->pr_current_be = be; *index = i; break; } } + /* cleaning up the rest of the timedout if any */ + for (++i; i < conn->c_pagedresults.prl_maxlen; i++, prp++) { + if (prp->pr_current_be && + (((prp->pr_timelimit > 0) && (ctime < prp->pr_timelimit)) || /* timelimit exceeded */ + (prp->pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED)) /* abandoned */) { + _pr_cleanup_one_slot(prp); + conn->c_pagedresults.prl_count--; + } + } } if ((*index > -1) && (*index < conn->c_pagedresults.prl_maxlen) && !conn->c_pagedresults.prl_list[*index].pr_mutex) { @@ -131,7 +171,6 @@ pagedresults_parse_control_value( Slapi_PBlock *pb, } conn->c_pagedresults.prl_count++; } else { - PagedResults *prp = NULL; /* Repeated paged results request. * PagedResults is already allocated. */ char *ptr = slapi_ch_malloc(cookie.bv_len + 1); @@ -156,8 +195,10 @@ pagedresults_parse_control_value( Slapi_PBlock *pb, slapi_ch_free((void **)&cookie.bv_val); if ((*index > -1) && (*index < conn->c_pagedresults.prl_maxlen)) { - if (conn->c_pagedresults.prl_list[*index].pr_flags & - CONN_FLAG_PAGEDRESULTS_ABANDONED) { + if (conn->c_pagedresults.prl_list[*index].pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED) { + /* repeated case? */ + prp = conn->c_pagedresults.prl_list + *index; + _pr_cleanup_one_slot(prp); rc = LDAP_CANCELLED; } else { /* Need to keep the latest msgid to prepare for the abandon. */ @@ -273,20 +314,8 @@ pagedresults_free_one( Connection *conn, Operation *op, int index ) "conn=%d paged requests list count is %d\n", conn->c_connid, conn->c_pagedresults.prl_count); } else if (index < conn->c_pagedresults.prl_maxlen) { - PRLock *prmutex = NULL; PagedResults *prp = conn->c_pagedresults.prl_list + index; - if (prp && prp->pr_current_be && - prp->pr_current_be->be_search_results_release && - prp->pr_search_result_set) { - prp->pr_current_be->be_search_results_release(&(prp->pr_search_result_set)); - } - prp->pr_current_be = NULL; - if (prp->pr_mutex) { - /* pr_mutex is reused; back it up and reset it. */ - prmutex = prp->pr_mutex; - } - memset(prp, '\0', sizeof(PagedResults)); - prp->pr_mutex = prmutex; + _pr_cleanup_one_slot(prp); conn->c_pagedresults.prl_count--; rc = 0; } @@ -309,7 +338,7 @@ pagedresults_free_one_msgid_nolock( Connection *conn, ber_int_t msgid ) ; /* Not a paged result. */ } else { LDAPDebug1Arg(LDAP_DEBUG_TRACE, - "--> pagedresults_free_one: msgid=%d\n", msgid); + "--> pagedresults_free_one_msgid_nolock: msgid=%d\n", msgid); for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++) { if (conn->c_pagedresults.prl_list[i].pr_msgid == msgid) { PagedResults *prp = conn->c_pagedresults.prl_list + i; @@ -318,16 +347,14 @@ pagedresults_free_one_msgid_nolock( Connection *conn, ber_int_t msgid ) prp->pr_search_result_set) { prp->pr_current_be->be_search_results_release(&(prp->pr_search_result_set)); } - prp->pr_current_be = NULL; prp->pr_flags |= CONN_FLAG_PAGEDRESULTS_ABANDONED; prp->pr_flags &= ~CONN_FLAG_PAGEDRESULTS_PROCESSING; - conn->c_pagedresults.prl_count--; rc = 0; break; } } LDAPDebug1Arg(LDAP_DEBUG_TRACE, - "<-- pagedresults_free_one: %d\n", rc); + "<-- pagedresults_free_one_msgid_nolock: %d\n", rc); } } @@ -845,37 +872,42 @@ pagedresults_reset_processing(Connection *conn, int index) } #endif -/* Are all the paged results requests timed out? */ +/* + * This timedout is mainly for an end user leaves a commandline untouched + * for a long time. This should not affect a permanent connection which + * manages multiple simple paged results requests over the connection. + * + * [rule] + * If there is just one slot and it's timed out, we return it is timedout. + * If there are multiple slots, the connection may be a permanent one. + * Do not return timed out here. But let the next request take care the + * timedout slot(s). + */ int pagedresults_is_timedout_nolock(Connection *conn) { - int i; PagedResults *prp = NULL; time_t ctime; - int rc = 0; LDAPDebug0Args(LDAP_DEBUG_TRACE, "--> pagedresults_is_timedout\n"); - if (NULL == conn || 0 == conn->c_pagedresults.prl_count) { - LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: -\n"); - return rc; + if (!conn || (0 == conn->c_pagedresults.prl_maxlen)) { + LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: false\n"); + return 0; } ctime = current_time(); - for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++) { - prp = conn->c_pagedresults.prl_list + i; + prp = conn->c_pagedresults.prl_list; + if (prp && (1 == conn->c_pagedresults.prl_maxlen)) { if (prp->pr_current_be && (prp->pr_timelimit > 0)) { - if (ctime < prp->pr_timelimit) { - LDAPDebug0Args(LDAP_DEBUG_TRACE, - "<-- pagedresults_is_timedout: 0\n"); - return 0; /* at least, one request is not timed out. */ - } else { - rc = 1; /* possibly timed out */ + if (ctime > prp->pr_timelimit) { + LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: true\n"); + return 1; } } } - LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: 1\n"); - return rc; /* all requests are timed out. */ + LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: false\n"); + return 0; } /* reset all timeout */ -- 1.9.3