From: Behdad Esfahbod Date: Tue, 10 May 2011 23:12:49 +0000 (-0400) Subject: Fix set implementation to be truly threadsafe even with destroy() callbacks X-Git-Tag: submit/2.0alpha-wayland/20121130.004132~9^2~227 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=45bfa99034512e886d75b1d45a5a649647f4711f;p=profile%2Fivi%2Forg.tizen.video-player.git Fix set implementation to be truly threadsafe even with destroy() callbacks The test/object test is passing again, instead of deadlocking. --- diff --git a/src/hb-common.cc b/src/hb-common.cc index 02314b3..782d85a 100644 --- a/src/hb-common.cc +++ b/src/hb-common.cc @@ -162,7 +162,7 @@ struct hb_language_item_t { void finish (void) { free (lang); } }; -static hb_threadsafe_set_t langs; +static hb_static_threadsafe_set_t langs; hb_language_t hb_language_from_string (const char *str) @@ -293,8 +293,7 @@ hb_script_get_horizontal_direction (hb_script_t script) * should switch to using that insted for these too. */ -/* XXX this can result in deadlocks because we call user callbacks */ -static hb_static_mutex_t user_data_mutex; +static hb_static_mutex_t user_data_lock; bool hb_user_data_array_t::set (hb_user_data_key_t *key, @@ -304,16 +303,12 @@ hb_user_data_array_t::set (hb_user_data_key_t *key, if (!key) return false; - hb_mutex_lock (&user_data_mutex); - if (!data && !destroy) { - items.remove (key); + items.remove (key, user_data_lock); return true; } hb_user_data_item_t item = {key, data, destroy}; - bool ret = !!items.insert (item); - - hb_mutex_unlock (&user_data_mutex); + bool ret = !!items.replace_or_insert (item, user_data_lock); return ret; } @@ -321,14 +316,15 @@ hb_user_data_array_t::set (hb_user_data_key_t *key, void * hb_user_data_array_t::get (hb_user_data_key_t *key) { - hb_mutex_lock (&user_data_mutex); + hb_user_data_item_t item = {NULL }; - hb_user_data_item_t *item = items.find (key); - void *ret = item ? item->data : NULL; - - hb_mutex_unlock (&user_data_mutex); + return items.find (key, &item, user_data_lock) ? item.data : NULL; +} - return ret; +void +hb_user_data_array_t::finish (void) +{ + items.finish (user_data_lock); } diff --git a/src/hb-mutex-private.hh b/src/hb-mutex-private.hh index b52d970..32b387c 100644 --- a/src/hb-mutex-private.hh +++ b/src/hb-mutex-private.hh @@ -105,62 +105,49 @@ struct hb_static_mutex_t : hb_mutex_t hb_static_mutex_t (void) { hb_mutex_init (this); } + + inline void lock (void) { hb_mutex_lock (this); } + inline void unlock (void) { hb_mutex_unlock (this); } }; HB_END_DECLS -/* XXX If the finish() callbacks of items in the set recursively try to - * modify the set, deadlock occurs. This needs fixing in set proper in - * fact. */ - template -struct hb_threadsafe_set_t +struct hb_static_threadsafe_set_t { - hb_set_t set; - hb_static_mutex_t mutex; + hb_lockable_set_t set; + hb_static_mutex_t lock; template - inline item_t *insert (T v) + inline item_t *replace_or_insert (T v) { - hb_mutex_lock (&mutex); - item_t *item = set.insert (v); - hb_mutex_unlock (&mutex); - return item; + return set.replace_or_insert (v, lock); } template inline void remove (T v) { - hb_mutex_lock (&mutex); - set.remove (v); - hb_mutex_unlock (&mutex); + set.remove (v, lock); } template - inline item_t *find (T v) + inline bool find (T v, item_t *i) { - hb_mutex_lock (&mutex); - item_t *item = set.find (v); - hb_mutex_unlock (&mutex); - return item; + return set.find (v, i, lock); } template - inline item_t *find_or_insert (T v) { - hb_mutex_lock (&mutex); - item_t *item = set.find_or_insert (v); - hb_mutex_unlock (&mutex); - return item; + inline item_t *find_or_insert (T v) + { + return set.find_or_insert (v, lock); } - void finish (void) { - hb_mutex_lock (&mutex); - set.finish (); - hb_mutex_unlock (&mutex); + void finish (void) + { + set.finish (lock); } - }; diff --git a/src/hb-object-private.hh b/src/hb-object-private.hh index a0d5fe8..08f4bb1 100644 --- a/src/hb-object-private.hh +++ b/src/hb-object-private.hh @@ -34,6 +34,8 @@ #include "hb-private.hh" +#include "hb-mutex-private.hh" + HB_BEGIN_DECLS @@ -117,7 +119,7 @@ struct hb_user_data_array_t { void finish (void) { if (destroy) destroy (data); } }; - hb_set_t items; + hb_lockable_set_t items; HB_INTERNAL bool set (hb_user_data_key_t *key, void * data, @@ -125,7 +127,7 @@ struct hb_user_data_array_t { HB_INTERNAL void *get (hb_user_data_key_t *key); - void finish (void) { items.finish (); } + HB_INTERNAL void finish (void); }; diff --git a/src/hb-private.hh b/src/hb-private.hh index 577389d..f22e0d2 100644 --- a/src/hb-private.hh +++ b/src/hb-private.hh @@ -319,56 +319,131 @@ template struct hb_array_t : hb_prealloced_array_t {}; -template -struct hb_set_t +template +struct hb_lockable_set_t { hb_array_t items; template - inline item_t *insert (T v) + inline item_t *replace_or_insert (T v, lock_t &l) { + l.lock (); item_t *item = items.find (v); - if (item) - item->finish (); - else + if (item) { + item_t old = *item; + *item = v; + l.unlock (); + old.finish (); + } else { item = items.push (); - if (unlikely (!item)) return NULL; - *item = v; + if (likely (item)) + *item = v; + l.unlock (); + } return item; } template - inline void remove (T v) + inline void remove (T v, lock_t &l) { + l.lock (); item_t *item = items.find (v); - if (!item) return; - - item->finish (); - *item = items[items.len - 1]; - items.pop (); + if (item) { + item_t old = *item; + *item = items[items.len - 1]; + items.pop (); + l.unlock (); + old.finish (); + } else { + l.unlock (); + } } template - inline item_t *find (T v) + inline bool find (T v, item_t *i, lock_t &l) { - return items.find (v); + l.lock (); + item_t *item = items.find (v); + if (item) + *i = *item; + l.unlock (); + return !!item; } template - inline item_t *find_or_insert (T v) { - item_t *item = find (v); + inline item_t *find_or_insert (T v, lock_t &l) + { + l.lock (); + item_t *item = items.find (v); if (!item) { item = items.push (); if (likely (item)) *item = v; } + l.unlock (); return item; } - void finish (void) { - for (unsigned i = 0; i < items.len; i++) - items[i].finish (); + inline void finish (lock_t &l) + { + l.lock (); + while (items.len) { + item_t old = items[items.len - 1]; + items.pop (); + l.unlock (); + old.finish (); + l.lock (); + } items.shrink (0); + l.unlock (); + } + +}; + +template +struct hb_set_t +{ + struct lock_t { + int unused; + + inline void lock (void) {} + inline void unlock (void) {} + }; + + hb_lockable_set_t set; + + template + inline item_t *replace_or_insert (T v) + { + lock_t lock; + return set.replace_or_insert (v, lock); + } + + template + inline void remove (T v) + { + lock_t lock; + set.remove (v, lock); + } + + template + inline bool find (T v, item_t *i) + { + lock_t lock; + return set.find (v, i, lock); + } + + template + inline item_t *find_or_insert (T v) + { + lock_t lock; + return set.find_or_insert (v, lock); + } + + void finish (void) + { + lock_t lock; + set.finish (lock); } };