From 8d4eac7f69e59d322b9a0b77f6ccc89139cd30bf Mon Sep 17 00:00:00 2001 From: Shane Bryldt Date: Mon, 17 Apr 2017 11:10:05 -0600 Subject: [PATCH] FS-10167: fixed a couple deadlock issues and a misconception about the locks on hash --- libs/libblade/src/blade_session.c | 2 +- libs/libblade/src/blade_stack.c | 24 ++++++++++++++++-------- libs/libks/src/simclist.c | 12 ++++++------ 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/libs/libblade/src/blade_session.c b/libs/libblade/src/blade_session.c index ea7e0509fd..41ddd0f64a 100644 --- a/libs/libblade/src/blade_session.c +++ b/libs/libblade/src/blade_session.c @@ -319,8 +319,8 @@ KS_DECLARE(void) blade_session_state_set(blade_session_t *bs, blade_session_stat blade_handle_session_state_callbacks_execute(bs, BLADE_SESSION_STATE_CONDITION_PRE); - ks_cond_try_signal(bs->cond); ks_mutex_unlock(bs->mutex); + ks_cond_try_signal(bs->cond); } KS_DECLARE(void) blade_session_hangup(blade_session_t *bs) diff --git a/libs/libblade/src/blade_stack.c b/libs/libblade/src/blade_stack.c index 3c628631a9..77a7cfe98c 100644 --- a/libs/libblade/src/blade_stack.c +++ b/libs/libblade/src/blade_stack.c @@ -196,22 +196,23 @@ KS_DECLARE(ks_status_t) blade_handle_create(blade_handle_t **bhP, ks_pool_t *poo bh->pool = pool; bh->tpool = tpool; - ks_hash_create(&bh->transports, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + // @todo this needs to be reviewed, NOLOCK is incorrect, but RWLOCK causes deadlock somewhere + ks_hash_create(&bh->transports, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->transports); - ks_hash_create(&bh->spaces, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + ks_hash_create(&bh->spaces, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->spaces); - ks_hash_create(&bh->events, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + ks_hash_create(&bh->events, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->events); - ks_hash_create(&bh->connections, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + ks_hash_create(&bh->connections, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->connections); - ks_hash_create(&bh->sessions, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + ks_hash_create(&bh->sessions, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->sessions); - ks_hash_create(&bh->session_state_callbacks, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + ks_hash_create(&bh->session_state_callbacks, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->session_state_callbacks); - ks_hash_create(&bh->requests, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_NOLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); + ks_hash_create(&bh->requests, KS_HASH_MODE_CASE_INSENSITIVE, KS_HASH_FLAG_RWLOCK | KS_HASH_FLAG_DUP_CHECK, bh->pool); ks_assert(bh->requests); *bhP = bh; @@ -665,6 +666,13 @@ KS_DECLARE(blade_session_t *) blade_handle_sessions_get(blade_handle_t *bh, cons ks_assert(bh); ks_assert(sid); + // @todo consider using blade_session_t via reference counting, rather than locking a mutex to simulate a reference count to halt cleanups while in use + // using actual reference counting would mean that mutexes would not need to be held locked when looking up a session by id just to prevent cleanup, + // instead cleanup would automatically occur when the last reference is actually removed (which SHOULD be at the end of the state machine thread), + // which is safer than another thread potentially waiting on the write lock to release while it's being destroyed, or external code forgetting to unlock + // then use short lived mutex or rwl for accessing the content of the session while it is referenced + // this approach should also be used for blade_connection_t, which has a similar threaded state machine + ks_hash_read_lock(bh->sessions); bs = ks_hash_search(bh->sessions, (void *)sid, KS_UNLOCKED); if (bs && blade_session_read_lock(bs, KS_FALSE) != KS_STATUS_SUCCESS) bs = NULL; @@ -766,7 +774,7 @@ KS_DECLARE(ks_status_t) blade_handle_session_state_callback_unregister(blade_han ks_hash_write_lock(bh->session_state_callbacks); bhsscr = (blade_handle_session_state_callback_registration_t *)ks_hash_remove(bh->session_state_callbacks, (void *)id); if (!bhsscr) ret = KS_STATUS_FAIL; - ks_hash_write_lock(bh->session_state_callbacks); + ks_hash_write_unlock(bh->session_state_callbacks); if (bhsscr) blade_handle_session_state_callback_registration_destroy(&bhsscr); diff --git a/libs/libks/src/simclist.c b/libs/libks/src/simclist.c index 52b0bddf78..4aacd9e06f 100755 --- a/libs/libks/src/simclist.c +++ b/libs/libks/src/simclist.c @@ -267,8 +267,6 @@ static void ks_list_cleanup(ks_pool_t *pool, void *ptr, void *arg, ks_pool_clean break; case KS_MPCL_TEARDOWN: ks_list_clear(l); - break; - case KS_MPCL_DESTROY: ks_rwl_write_lock(l->lock); for (unsigned int i = 0; i < l->spareelsnum; i++) ks_pool_free(l->pool, &l->spareels[i]); l->spareelsnum = 0; @@ -278,6 +276,8 @@ static void ks_list_cleanup(ks_pool_t *pool, void *ptr, void *arg, ks_pool_clean ks_rwl_write_unlock(l->lock); ks_rwl_destroy(&l->lock); break; + case KS_MPCL_DESTROY: + break; } } @@ -648,13 +648,13 @@ KS_DECLARE(int) ks_list_delete_at(ks_list_t *restrict l, unsigned int pos) { KS_DECLARE(int) ks_list_delete_iterator(ks_list_t *restrict l) { struct ks_list_entry_s *delendo; - if (!l->iter_active || l->iter_pos >= l->numels) return -1; + if (!l->iter_active || l->iter_pos > l->numels) return -1; ks_rwl_write_lock(l->lock); - delendo = ks_list_findpos(l, l->iter_pos); - - ks_list_drop_elem(l, delendo, l->iter_pos); + delendo = ks_list_findpos(l, l->iter_pos - 1); + + ks_list_drop_elem(l, delendo, l->iter_pos - 1); l->numels--;