[Freeipa-devel] [PATCH 0018] Deadlock detection logic
Adam Tkac
atkac at redhat.com
Mon May 7 10:50:12 UTC 2012
On 05/07/2012 12:35 PM, Petr Spacek wrote:
> On 05/03/2012 02:18 PM, Adam Tkac wrote:
>> On Tue, Apr 24, 2012 at 03:52:00PM +0200, Petr Spacek wrote:
>>> On 04/24/2012 03:21 PM, Petr Spacek wrote:
>>>> Hello,
>>>>
>>>> this patch adds deadlock detection (based on simple timeout) to
>>>> current code.
>>>> If (probable) deadlock is detected, current action is stopped with
>>>> proper error.
>>>>
>>>> It properly detects "Simo's deadlock" with 'connections' parameter
>>>> == 1.
>>>> (Described in https://fedorahosted.org/bind-dyndb-ldap/ticket/66)
>>>>
>>>> Deadlock itself will be fixed by separate patch.
>>>>
>>>> Petr^2 Spacek
>>>
>>> Self-NACK :-)
>>>
>>> Second version of the patch is attached. ldap_pool_getconnection()
>>> and ldap_pool_putconnection() now has same interface and more
>>> consistent behaviour.
>>>
>>> Overall functionality is same.
>>
>> Hi,
>>
>> although I'm not fully happy with current design of the detection
>> logic, we can
>> include it before we create something better, for example based on
>> thread IDs
>> (one thread can acquire semaphore only one time).
> I agree, it's far from ideal. Connection and result handling needs
> redesign at first. After that I can modify detection logic to be more
> accurate.
>
>>
>> Please check my comments inside the patch.
> All done.
Ack. Please push it to master.
Regards, Adam
>
>>
>> Regards, Adam
>>
>>> From ed7596a9410269c0c29418247971f262b7fa77f3 Mon Sep 17 00:00:00 2001
>>> From: Petr Spacek<pspacek at redhat.com>
>>> Date: Tue, 24 Apr 2012 15:09:32 +0200
>>> Subject: [PATCH] Add simple semaphore deadlock detection logic.
>>> Signed-off-by: Petr Spacek<pspacek at redhat.com>
>>>
>>> ---
>>> src/ldap_helper.c | 78
>>> ++++++++++++++++++++++++++++++++---------------------
>>> src/semaphore.c | 26 +++++++++++++++---
>>> src/semaphore.h | 6 +++-
>>> 3 files changed, 74 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
>>> index 47c0559..f4c4d02 100644
>>> --- a/src/ldap_helper.c
>>> +++ b/src/ldap_helper.c
>>> @@ -296,9 +296,10 @@ ldap_delete_zone2(ldap_instance_t *inst,
>>> dns_name_t *name, isc_boolean_t lock);
>>> static isc_result_t ldap_pool_create(isc_mem_t *mctx, unsigned int
>>> connections,
>>> ldap_pool_t **poolp);
>>> static void ldap_pool_destroy(ldap_pool_t **poolp);
>>> -static ldap_connection_t * ldap_pool_getconnection(ldap_pool_t *pool);
>>> +static isc_result_t ldap_pool_getconnection(ldap_pool_t *pool,
>>> + ldap_connection_t ** conn);
>>> static void ldap_pool_putconnection(ldap_pool_t *pool,
>>> - ldap_connection_t *ldap_conn);
>>> + ldap_connection_t ** conn);
>>> static isc_result_t ldap_pool_connect(ldap_pool_t *pool,
>>> ldap_instance_t *ldap_inst);
>>>
>>> @@ -402,6 +403,10 @@ new_ldap_instance(isc_mem_t *mctx, const char
>>> *db_name,
>>> ldap_settings[i++].target =&ldap_inst->dyn_update;
>>> CHECK(set_settings(ldap_settings, argv));
>>>
>>> + /* Set timer for deadlock detection inside semaphore_wait_timed
>>> . */
>>> + if (semaphore_wait_timeout.seconds<
>>> ldap_inst->timeout+SEM_WAIT_TIMEOUT_ADD)
>>> + semaphore_wait_timeout.seconds =
>>> ldap_inst->timeout+SEM_WAIT_TIMEOUT_ADD;
>>
>> I'm affraid such low timeout (12 sec by default) will cause too many
>> false
>> positives. I recommend to set semaphore timeout to at least 60 seconds.
>>
>>> /* Validate and check settings. */
>>> str_toupper(ldap_inst->sasl_mech);
>>> if (ldap_inst->connections< 1) {
>>> @@ -1089,7 +1094,7 @@ isc_result_t
>>> refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
>>> {
>>> isc_result_t result = ISC_R_SUCCESS;
>>> - ldap_connection_t *ldap_conn;
>>> + ldap_connection_t *ldap_conn = NULL;
>>> int zone_count = 0;
>>> ldap_entry_t *entry;
>>> dns_rbt_t *rbt = NULL;
>>> @@ -1114,7 +1119,7 @@ refresh_zones_from_ldap(ldap_instance_t
>>> *ldap_inst)
>>>
>>> log_debug(2, "refreshing list of zones for %s",
>>> ldap_inst->db_name);
>>>
>>> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
>>> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
>>>
>>> CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(ldap_inst->base),
>>> LDAP_SCOPE_SUBTREE, config_attrs, 0,
>>> @@ -1227,7 +1232,7 @@ cleanup:
>>> if (invalidate_nodechain)
>>> dns_rbtnodechain_invalidate(&chain);
>>>
>>> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
>>> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
>>>
>>> log_debug(2, "finished refreshing list of zones");
>>>
>>> @@ -1381,7 +1386,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx,
>>> ldap_instance_t *ldap_inst, dns_name_t *nam
>>> dns_name_t *origin, ldapdb_nodelist_t *nodelist)
>>> {
>>> isc_result_t result;
>>> - ldap_connection_t *ldap_conn;
>>> + ldap_connection_t *ldap_conn = NULL;
>>> ldap_entry_t *entry;
>>> ld_string_t *string = NULL;
>>> ldapdb_node_t *node;
>>> @@ -1392,7 +1397,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx,
>>> ldap_instance_t *ldap_inst, dns_name_t *nam
>>> REQUIRE(nodelist != NULL);
>>>
>>> /* RRs aren't in the cache, perform ordinary LDAP query */
>>> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
>>> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
>>>
>>> INIT_LIST(*nodelist);
>>> CHECK(str_new(mctx,&string));
>>> @@ -1439,7 +1444,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx,
>>> ldap_instance_t *ldap_inst, dns_name_t *nam
>>> result = ISC_R_SUCCESS;
>>>
>>> cleanup:
>>> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
>>> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
>>> str_destroy(&string);
>>>
>>> return result;
>>> @@ -1450,7 +1455,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx,
>>> ldap_instance_t *ldap_inst, dns_name_t *na
>>> dns_name_t *origin, ldapdb_rdatalist_t *rdatalist)
>>> {
>>> isc_result_t result;
>>> - ldap_connection_t *ldap_conn;
>>> + ldap_connection_t *ldap_conn = NULL;
>>> ldap_entry_t *entry;
>>> ld_string_t *string = NULL;
>>> ldap_cache_t *cache;
>>> @@ -1468,12 +1473,11 @@ ldapdb_rdatalist_get(isc_mem_t *mctx,
>>> ldap_instance_t *ldap_inst, dns_name_t *na
>>> return result;
>>>
>>> /* RRs aren't in the cache, perform ordinary LDAP query */
>>> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
>>> -
>>> INIT_LIST(*rdatalist);
>>> CHECK(str_new(mctx,&string));
>>> CHECK(dnsname_to_dn(ldap_inst->zone_register, name, string));
>>>
>>> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
>>> CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(string),
>>> LDAP_SCOPE_BASE, NULL, 0, "(objectClass=idnsRecord)"));
>>>
>>> @@ -1500,7 +1504,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx,
>>> ldap_instance_t *ldap_inst, dns_name_t *na
>>> result = ISC_R_NOTFOUND;
>>>
>>> cleanup:
>>> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
>>> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
>>> str_destroy(&string);
>>>
>>> if (result != ISC_R_SUCCESS)
>>> @@ -2250,7 +2254,7 @@ modify_ldap_common(dns_name_t *owner,
>>> ldap_instance_t *ldap_inst,
>>> zone_dn += 1; /* skip whitespace */
>>> }
>>>
>>> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
>>> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
>>> CHECK(ldap_query(ldap_inst, ldap_conn, zone_dn,
>>> LDAP_SCOPE_BASE, attrs, 0,
>>>
>>> "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
>>> @@ -2481,9 +2485,7 @@ modify_ldap_common(dns_name_t *owner,
>>> ldap_instance_t *ldap_inst,
>>> }
>>>
>>> cleanup:
>>> - if (ldap_conn != NULL)
>>> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
>>> -
>>> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
>>> str_destroy(&owner_dn_ptr);
>>> str_destroy(&owner_dn);
>>> free_ldapmod(mctx,&change[0]);
>>> @@ -2565,15 +2567,18 @@ ldap_pool_destroy(ldap_pool_t **poolp)
>>> MEM_PUT_AND_DETACH(pool);
>>> }
>>>
>>> -static ldap_connection_t *
>>> -ldap_pool_getconnection(ldap_pool_t *pool)
>>> +static isc_result_t
>>> +ldap_pool_getconnection(ldap_pool_t *pool, ldap_connection_t ** conn)
>>> {
>>> ldap_connection_t *ldap_conn = NULL;
>>> unsigned int i;
>>> + isc_result_t result;
>>>
>>> REQUIRE(pool != NULL);
>>> + REQUIRE(conn != NULL&& *conn == NULL);
>>> + ldap_conn = *conn;
>>>
>>> - semaphore_wait(&pool->conn_semaphore);
>>> + CHECK(semaphore_wait_timed(&pool->conn_semaphore));
>>> for (i = 0; i< pool->connections; i++) {
>>> ldap_conn = pool->conns[i];
>>> if (isc_mutex_trylock(&ldap_conn->lock) == ISC_R_SUCCESS)
>>> @@ -2583,16 +2588,30 @@ ldap_pool_getconnection(ldap_pool_t *pool)
>>> RUNTIME_CHECK(ldap_conn != NULL);
>>>
>>> INIT_LIST(ldap_conn->ldap_entries);
>>> + *conn = ldap_conn;
>>>
>>> - return ldap_conn;
>>> +cleanup:
>>> + if (result != ISC_R_SUCCESS) {
>>> + log_error("timeout in ldap_pool_getconnection(): try to
>>> raise "
>>> + "'connections' parameter; potential deadlock?");
>>> + }
>>> + return result;
>>> }
>>>
>>> static void
>>> -ldap_pool_putconnection(ldap_pool_t *pool, ldap_connection_t
>>> *ldap_conn)
>>> +ldap_pool_putconnection(ldap_pool_t *pool, ldap_connection_t **conn)
>>> {
>>> + REQUIRE(conn);
>>
>> Although this is correct, please use REQUIRE(conn != NULL);
>>
>>> + ldap_connection_t *ldap_conn = *conn;
>>> +
>>> + if (ldap_conn == NULL)
>>> + return;
>>> +
>>> ldap_query_free(ldap_conn);
>>> UNLOCK(&ldap_conn->lock);
>>> semaphore_signal(&pool->conn_semaphore);
>>> +
>>> + *conn = NULL;
>>> }
>>>
>>> static isc_result_t
>>> @@ -2719,7 +2738,7 @@ update_action(isc_task_t *task, isc_event_t
>>> *event)
>>> ldap_psearchevent_t *pevent = (ldap_psearchevent_t *)event;
>>> isc_result_t result ;
>>> ldap_instance_t *inst = NULL;
>>> - ldap_connection_t *conn;
>>> + ldap_connection_t *conn = NULL;
>>> ldap_entry_t *entry;
>>> isc_boolean_t delete = ISC_TRUE;
>>> isc_mem_t *mctx;
>>> @@ -2736,7 +2755,7 @@ update_action(isc_task_t *task, isc_event_t
>>> *event)
>>> if (result != ISC_R_SUCCESS)
>>> goto cleanup;
>>>
>>> - conn = ldap_pool_getconnection(inst->pool);
>>> + CHECK(ldap_pool_getconnection(inst->pool,&conn));
>>>
>>> CHECK(ldap_query(inst, conn, pevent->dn,
>>> LDAP_SCOPE_BASE, attrs, 0,
>>> @@ -2754,14 +2773,13 @@ update_action(isc_task_t *task, isc_event_t
>>> *event)
>>> if (delete)
>>> CHECK(ldap_delete_zone(inst, pevent->dn, ISC_TRUE));
>>>
>>> - ldap_pool_putconnection(inst->pool, conn);
>>> -
>>> cleanup:
>>> if (result != ISC_R_SUCCESS)
>>> log_error("update_action (psearch) failed for %s. "
>>> "Zones can be outdated, run `rndc reload`",
>>> pevent->dn);
>>>
>>> + ldap_pool_putconnection(inst->pool,&conn);
>>> isc_mem_free(mctx, pevent->dbname);
>>> isc_mem_free(mctx, pevent->dn);
>>> isc_mem_detach(&mctx);
>>> @@ -2790,7 +2808,7 @@ update_config(isc_task_t *task, isc_event_t
>>> *event)
>>> if (result != ISC_R_SUCCESS)
>>> goto cleanup;
>>>
>>> - conn = ldap_pool_getconnection(inst->pool);
>>> + CHECK(ldap_pool_getconnection(inst->pool,&conn));
>>>
>>> CHECK(ldap_query(inst, conn, pevent->dn,
>>> LDAP_SCOPE_BASE, attrs, 0,
>>> @@ -2809,14 +2827,12 @@ update_config(isc_task_t *task, isc_event_t
>>> *event)
>>>
>>>
>>> cleanup:
>>> - if (conn != NULL)
>>> - ldap_pool_putconnection(inst->pool, conn);
>>> -
>>> if (result != ISC_R_SUCCESS)
>>> log_error("update_config (psearch) failed for %s. "
>>> "Configuration can be outdated, run `rndc reload`",
>>> pevent->dn);
>>>
>>> + ldap_pool_putconnection(inst->pool,&conn);
>>> isc_mem_free(mctx, pevent->dbname);
>>> isc_mem_free(mctx, pevent->dn);
>>> isc_mem_detach(&mctx);
>>> @@ -3079,7 +3095,7 @@ ldap_psearch_watcher(isc_threadarg_t arg)
>>> tv.tv_usec = 0;
>>>
>>> /* Pick connection, one is reserved purely for this thread */
>>> - conn = ldap_pool_getconnection(inst->pool);
>>> + CHECK(ldap_pool_getconnection(inst->pool,&conn));
>>>
>>> /* Try to connect. */
>>> while (conn->handle == NULL) {
>>> @@ -3189,7 +3205,7 @@ soft_err:
>>> log_debug(1, "Ending ldap_psearch_watcher");
>>>
>>> cleanup:
>>> - ldap_pool_putconnection(inst->pool, conn);
>>> + ldap_pool_putconnection(inst->pool,&conn);
>>>
>>> return (isc_threadresult_t)0;
>>> }
>>> diff --git a/src/semaphore.c b/src/semaphore.c
>>> index 41d6a30..76e6aa2 100644
>>> --- a/src/semaphore.c
>>> +++ b/src/semaphore.c
>>> @@ -27,8 +27,19 @@
>>> #include<isc/condition.h>
>>> #include<isc/result.h>
>>> #include<isc/util.h>
>>> +#include<isc/time.h>
>>>
>>> #include "semaphore.h"
>>> +#include "util.h"
>>> +
>>> +/*
>>> + * Timer setting for deadlock detection. Format: seconds, nanoseconds.
>>> + * These values will be overwriten during initialization
>>> + * from set_settings() with max(setting+SEM_WAIT_TIMEOUT_ADD,
>>> curr_value).
>>> + *
>>> + * Initial value can be useful in early phases of initialization.
>>> + */
>>> +isc_interval_t semaphore_wait_timeout = { 3, 0 };
>>>
>>> /*
>>> * Initialize a semaphore.
>>> @@ -74,20 +85,27 @@ semaphore_destroy(semaphore_t *sem)
>>> /*
>>> * Wait on semaphore. This operation will try to acquire a lock on
>>> the
>>> * semaphore. If the semaphore is already acquired as many times
>>> at it allows,
>>> - * the function will block until someone releases the lock.
>>> + * the function will block until someone releases the lock OR
>>> timeout expire.
>>> + *
>>> + * @return ISC_R_SUCCESS or ISC_R_TIMEDOUT or other errors from ISC
>>> libs
>>> */
>>> -void
>>> -semaphore_wait(semaphore_t *sem)
>>> +isc_result_t
>>> +semaphore_wait_timed(semaphore_t *sem)
>>> {
>>> + isc_result_t result;
>>> + isc_time_t abs_timeout;
>>> REQUIRE(sem != NULL);
>>>
>>> LOCK(&sem->mutex);
>>> +
>>> CHECK(isc_time_nowplusinterval(&abs_timeout,&semaphore_wait_timeout));
>>
>> The isc_time_nowplusinterval doesn't modify shared data, please call
>> it before
>> LOCK().
>>
>>> while (sem->value<= 0)
>>> - WAIT(&sem->cond,&sem->mutex);
>>> + CHECK(WAITUNTIL(&sem->cond,&sem->mutex,&abs_timeout));
>>> sem->value--;
>>>
>>> +cleanup:
>>> UNLOCK(&sem->mutex);
>>> + return result;
>>> }
>>>
>>> /*
>>> diff --git a/src/semaphore.h b/src/semaphore.h
>>> index 4ca4f65..5aef5d2 100644
>>> --- a/src/semaphore.h
>>> +++ b/src/semaphore.h
>>> @@ -24,6 +24,10 @@
>>> #include<isc/condition.h>
>>> #include<isc/mutex.h>
>>>
>>> +/* How many seconds will be added to user-defined connection
>>> parameter 'timeout'. */
>>> +#define SEM_WAIT_TIMEOUT_ADD 2 /* seconds */
>>> +extern isc_interval_t semaphore_wait_timeout;
>>> +
>>> /*
>>> * Semaphore can be "acquired" multiple times. However, it has a
>>> maximum
>>> * number of times someone can acquire him. If a semaphore is
>>> already acquired
>>> @@ -40,7 +44,7 @@ typedef struct semaphore semaphore_t;
>>> /* Public functions. */
>>> isc_result_t semaphore_init(semaphore_t *sem, int value);
>>> void semaphore_destroy(semaphore_t *sem);
>>> -void semaphore_wait(semaphore_t *sem);
>>> +isc_result_t semaphore_wait_timed(semaphore_t *sem);
>>> void semaphore_signal(semaphore_t *sem);
>>>
>>> #endif /* !_LD_SEMAPHORE_H_ */
>>> --
>>> 1.7.7.6
>>>
>>
>>
>
>
> bind-dyndb-ldap-pspacek-0018-2-Add-simple-semaphore-deadlock-detection-logic
>
>
> From ff06086e55d64cd6893a6d064d8e101f09a71956 Mon Sep 17 00:00:00 2001
> From: Petr Spacek<pspacek at redhat.com>
> Date: Tue, 24 Apr 2012 15:09:32 +0200
> Subject: [PATCH] Add simple semaphore deadlock detection logic.
> Signed-off-by: Petr Spacek<pspacek at redhat.com>
>
> ---
> src/ldap_helper.c | 78 ++++++++++++++++++++++++++++++++---------------------
> src/semaphore.c | 26 +++++++++++++++---
> src/semaphore.h | 6 +++-
> 3 files changed, 74 insertions(+), 36 deletions(-)
>
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 47c05599cb48d22e172244b2c3229b9320f37d59..304c37296f8f3a428c4c72b45fe4154b2c9e954c 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -296,9 +296,10 @@ ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock);
> static isc_result_t ldap_pool_create(isc_mem_t *mctx, unsigned int connections,
> ldap_pool_t **poolp);
> static void ldap_pool_destroy(ldap_pool_t **poolp);
> -static ldap_connection_t * ldap_pool_getconnection(ldap_pool_t *pool);
> +static isc_result_t ldap_pool_getconnection(ldap_pool_t *pool,
> + ldap_connection_t ** conn);
> static void ldap_pool_putconnection(ldap_pool_t *pool,
> - ldap_connection_t *ldap_conn);
> + ldap_connection_t ** conn);
> static isc_result_t ldap_pool_connect(ldap_pool_t *pool,
> ldap_instance_t *ldap_inst);
>
> @@ -402,6 +403,10 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
> ldap_settings[i++].target =&ldap_inst->dyn_update;
> CHECK(set_settings(ldap_settings, argv));
>
> + /* Set timer for deadlock detection inside semaphore_wait_timed . */
> + if (semaphore_wait_timeout.seconds< ldap_inst->timeout*SEM_WAIT_TIMEOUT_MUL)
> + semaphore_wait_timeout.seconds = ldap_inst->timeout*SEM_WAIT_TIMEOUT_MUL;
> +
> /* Validate and check settings. */
> str_toupper(ldap_inst->sasl_mech);
> if (ldap_inst->connections< 1) {
> @@ -1089,7 +1094,7 @@ isc_result_t
> refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
> {
> isc_result_t result = ISC_R_SUCCESS;
> - ldap_connection_t *ldap_conn;
> + ldap_connection_t *ldap_conn = NULL;
> int zone_count = 0;
> ldap_entry_t *entry;
> dns_rbt_t *rbt = NULL;
> @@ -1114,7 +1119,7 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
>
> log_debug(2, "refreshing list of zones for %s", ldap_inst->db_name);
>
> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
>
> CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(ldap_inst->base),
> LDAP_SCOPE_SUBTREE, config_attrs, 0,
> @@ -1227,7 +1232,7 @@ cleanup:
> if (invalidate_nodechain)
> dns_rbtnodechain_invalidate(&chain);
>
> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
>
> log_debug(2, "finished refreshing list of zones");
>
> @@ -1381,7 +1386,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
> dns_name_t *origin, ldapdb_nodelist_t *nodelist)
> {
> isc_result_t result;
> - ldap_connection_t *ldap_conn;
> + ldap_connection_t *ldap_conn = NULL;
> ldap_entry_t *entry;
> ld_string_t *string = NULL;
> ldapdb_node_t *node;
> @@ -1392,7 +1397,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
> REQUIRE(nodelist != NULL);
>
> /* RRs aren't in the cache, perform ordinary LDAP query */
> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
>
> INIT_LIST(*nodelist);
> CHECK(str_new(mctx,&string));
> @@ -1439,7 +1444,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
> result = ISC_R_SUCCESS;
>
> cleanup:
> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
> str_destroy(&string);
>
> return result;
> @@ -1450,7 +1455,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
> dns_name_t *origin, ldapdb_rdatalist_t *rdatalist)
> {
> isc_result_t result;
> - ldap_connection_t *ldap_conn;
> + ldap_connection_t *ldap_conn = NULL;
> ldap_entry_t *entry;
> ld_string_t *string = NULL;
> ldap_cache_t *cache;
> @@ -1468,12 +1473,11 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
> return result;
>
> /* RRs aren't in the cache, perform ordinary LDAP query */
> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
> -
> INIT_LIST(*rdatalist);
> CHECK(str_new(mctx,&string));
> CHECK(dnsname_to_dn(ldap_inst->zone_register, name, string));
>
> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
> CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(string),
> LDAP_SCOPE_BASE, NULL, 0, "(objectClass=idnsRecord)"));
>
> @@ -1500,7 +1504,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
> result = ISC_R_NOTFOUND;
>
> cleanup:
> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
> str_destroy(&string);
>
> if (result != ISC_R_SUCCESS)
> @@ -2250,7 +2254,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> zone_dn += 1; /* skip whitespace */
> }
>
> - ldap_conn = ldap_pool_getconnection(ldap_inst->pool);
> + CHECK(ldap_pool_getconnection(ldap_inst->pool,&ldap_conn));
> CHECK(ldap_query(ldap_inst, ldap_conn, zone_dn,
> LDAP_SCOPE_BASE, attrs, 0,
> "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
> @@ -2481,9 +2485,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> }
>
> cleanup:
> - if (ldap_conn != NULL)
> - ldap_pool_putconnection(ldap_inst->pool, ldap_conn);
> -
> + ldap_pool_putconnection(ldap_inst->pool,&ldap_conn);
> str_destroy(&owner_dn_ptr);
> str_destroy(&owner_dn);
> free_ldapmod(mctx,&change[0]);
> @@ -2565,34 +2567,51 @@ ldap_pool_destroy(ldap_pool_t **poolp)
> MEM_PUT_AND_DETACH(pool);
> }
>
> -static ldap_connection_t *
> -ldap_pool_getconnection(ldap_pool_t *pool)
> +static isc_result_t
> +ldap_pool_getconnection(ldap_pool_t *pool, ldap_connection_t ** conn)
> {
> ldap_connection_t *ldap_conn = NULL;
> unsigned int i;
> + isc_result_t result;
>
> REQUIRE(pool != NULL);
> + REQUIRE(conn != NULL&& *conn == NULL);
> + ldap_conn = *conn;
>
> - semaphore_wait(&pool->conn_semaphore);
> + CHECK(semaphore_wait_timed(&pool->conn_semaphore));
> for (i = 0; i< pool->connections; i++) {
> ldap_conn = pool->conns[i];
> if (isc_mutex_trylock(&ldap_conn->lock) == ISC_R_SUCCESS)
> break;
> }
>
> RUNTIME_CHECK(ldap_conn != NULL);
>
> INIT_LIST(ldap_conn->ldap_entries);
> + *conn = ldap_conn;
>
> - return ldap_conn;
> +cleanup:
> + if (result != ISC_R_SUCCESS) {
> + log_error("timeout in ldap_pool_getconnection(): try to raise "
> + "'connections' parameter; potential deadlock?");
> + }
> + return result;
> }
>
> static void
> -ldap_pool_putconnection(ldap_pool_t *pool, ldap_connection_t *ldap_conn)
> +ldap_pool_putconnection(ldap_pool_t *pool, ldap_connection_t **conn)
> {
> + REQUIRE(conn != NULL);
> + ldap_connection_t *ldap_conn = *conn;
> +
> + if (ldap_conn == NULL)
> + return;
> +
> ldap_query_free(ldap_conn);
> UNLOCK(&ldap_conn->lock);
> semaphore_signal(&pool->conn_semaphore);
> +
> + *conn = NULL;
> }
>
> static isc_result_t
> @@ -2719,7 +2738,7 @@ update_action(isc_task_t *task, isc_event_t *event)
> ldap_psearchevent_t *pevent = (ldap_psearchevent_t *)event;
> isc_result_t result ;
> ldap_instance_t *inst = NULL;
> - ldap_connection_t *conn;
> + ldap_connection_t *conn = NULL;
> ldap_entry_t *entry;
> isc_boolean_t delete = ISC_TRUE;
> isc_mem_t *mctx;
> @@ -2736,7 +2755,7 @@ update_action(isc_task_t *task, isc_event_t *event)
> if (result != ISC_R_SUCCESS)
> goto cleanup;
>
> - conn = ldap_pool_getconnection(inst->pool);
> + CHECK(ldap_pool_getconnection(inst->pool,&conn));
>
> CHECK(ldap_query(inst, conn, pevent->dn,
> LDAP_SCOPE_BASE, attrs, 0,
> @@ -2754,14 +2773,13 @@ update_action(isc_task_t *task, isc_event_t *event)
> if (delete)
> CHECK(ldap_delete_zone(inst, pevent->dn, ISC_TRUE));
>
> - ldap_pool_putconnection(inst->pool, conn);
> -
> cleanup:
> if (result != ISC_R_SUCCESS)
> log_error("update_action (psearch) failed for %s. "
> "Zones can be outdated, run `rndc reload`",
> pevent->dn);
>
> + ldap_pool_putconnection(inst->pool,&conn);
> isc_mem_free(mctx, pevent->dbname);
> isc_mem_free(mctx, pevent->dn);
> isc_mem_detach(&mctx);
> @@ -2790,7 +2808,7 @@ update_config(isc_task_t *task, isc_event_t *event)
> if (result != ISC_R_SUCCESS)
> goto cleanup;
>
> - conn = ldap_pool_getconnection(inst->pool);
> + CHECK(ldap_pool_getconnection(inst->pool,&conn));
>
> CHECK(ldap_query(inst, conn, pevent->dn,
> LDAP_SCOPE_BASE, attrs, 0,
> @@ -2809,14 +2827,12 @@ update_config(isc_task_t *task, isc_event_t *event)
>
>
> cleanup:
> - if (conn != NULL)
> - ldap_pool_putconnection(inst->pool, conn);
> -
> if (result != ISC_R_SUCCESS)
> log_error("update_config (psearch) failed for %s. "
> "Configuration can be outdated, run `rndc reload`",
> pevent->dn);
>
> + ldap_pool_putconnection(inst->pool,&conn);
> isc_mem_free(mctx, pevent->dbname);
> isc_mem_free(mctx, pevent->dn);
> isc_mem_detach(&mctx);
> @@ -3079,7 +3095,7 @@ ldap_psearch_watcher(isc_threadarg_t arg)
> tv.tv_usec = 0;
>
> /* Pick connection, one is reserved purely for this thread */
> - conn = ldap_pool_getconnection(inst->pool);
> + CHECK(ldap_pool_getconnection(inst->pool,&conn));
>
> /* Try to connect. */
> while (conn->handle == NULL) {
> @@ -3189,7 +3205,7 @@ soft_err:
> log_debug(1, "Ending ldap_psearch_watcher");
>
> cleanup:
> - ldap_pool_putconnection(inst->pool, conn);
> + ldap_pool_putconnection(inst->pool,&conn);
>
> return (isc_threadresult_t)0;
> }
> diff --git a/src/semaphore.c b/src/semaphore.c
> index 41d6a306b76022a35967cd157ff767cdf2f2588d..352219f113a233218b5522beea5520dddbd748e6 100644
> --- a/src/semaphore.c
> +++ b/src/semaphore.c
> @@ -27,8 +27,19 @@
> #include<isc/condition.h>
> #include<isc/result.h>
> #include<isc/util.h>
> +#include<isc/time.h>
>
> #include "semaphore.h"
> +#include "util.h"
> +
> +/*
> + * Timer setting for deadlock detection. Format: seconds, nanoseconds.
> + * These values will be overwriten during initialization
> + * from set_settings() with max(setting+SEM_WAIT_TIMEOUT_ADD, curr_value).
> + *
> + * Initial value can be useful in early phases of initialization.
> + */
> +isc_interval_t semaphore_wait_timeout = { 3, 0 };
>
> /*
> * Initialize a semaphore.
> @@ -74,20 +85,27 @@ semaphore_destroy(semaphore_t *sem)
> /*
> * Wait on semaphore. This operation will try to acquire a lock on the
> * semaphore. If the semaphore is already acquired as many times at it allows,
> - * the function will block until someone releases the lock.
> + * the function will block until someone releases the lock OR timeout expire.
> + *
> + * @return ISC_R_SUCCESS or ISC_R_TIMEDOUT or other errors from ISC libs
> */
> -void
> -semaphore_wait(semaphore_t *sem)
> +isc_result_t
> +semaphore_wait_timed(semaphore_t *sem)
> {
> + isc_result_t result;
> + isc_time_t abs_timeout;
> REQUIRE(sem != NULL);
>
> + CHECK(isc_time_nowplusinterval(&abs_timeout,&semaphore_wait_timeout));
> LOCK(&sem->mutex);
>
> while (sem->value<= 0)
> - WAIT(&sem->cond,&sem->mutex);
> + CHECK(WAITUNTIL(&sem->cond,&sem->mutex,&abs_timeout));
> sem->value--;
>
> +cleanup:
> UNLOCK(&sem->mutex);
> + return result;
> }
>
> /*
> diff --git a/src/semaphore.h b/src/semaphore.h
> index 4ca4f652f599b520d759f656f42aa782c4ba9b38..1367747c03b9c022dce82b1b0191a496c5d359af 100644
> --- a/src/semaphore.h
> +++ b/src/semaphore.h
> @@ -24,6 +24,10 @@
> #include<isc/condition.h>
> #include<isc/mutex.h>
>
> +/* Multiplier for to user-defined connection parameter 'timeout'. */
> +#define SEM_WAIT_TIMEOUT_MUL 6 /* times */
> +extern isc_interval_t semaphore_wait_timeout;
> +
> /*
> * Semaphore can be "acquired" multiple times. However, it has a maximum
> * number of times someone can acquire him. If a semaphore is already acquired
> @@ -40,7 +44,7 @@ typedef struct semaphore semaphore_t;
> /* Public functions. */
> isc_result_t semaphore_init(semaphore_t *sem, int value);
> void semaphore_destroy(semaphore_t *sem);
> -void semaphore_wait(semaphore_t *sem);
> +isc_result_t semaphore_wait_timed(semaphore_t *sem);
> void semaphore_signal(semaphore_t *sem);
>
> #endif /* !_LD_SEMAPHORE_H_ */
More information about the Freeipa-devel
mailing list