Make event_del(E) block while E is running in another thread.

This gives you the property that once you have called event_del(E),
you know that E is no longer running or pending or active at all, and
so it is safe to delete the resource used by E's callback.

svn:r1341

diff --git a/ChangeLog b/ChangeLog
index db8698d362f51ccdcfa0cea4cf68405d625129ba..27a6a571c1a0f136a854db2e194967185a1cb9e2 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -42,6 +42,7 @@ Changes in 2.0.2-alpha:
  o Allow C identifiers as struct names; allow multiple comments in .rpc files; from Zack Weinberg
  o Mitigate a race condition when using socket bufferevents in multiple threads.
  o Use AC_SEARCH_LIBS, not AC_CHECK_LIB to avoid needless library use.
+ o Do not allow event_del(ev) to return while that event's callback is executing in another thread.  This fixes a nasty race condition.
 
 
 Changes in 2.0.1-alpha:

diff --git a/event-internal.h b/event-internal.h
index e41e5a321ed667caf1acc6d9ec0e5454c604edc1..cca5feb6333141a94b38d11df52dc9d438544365 100644 (file)
--- a/event-internal.h
+++ b/event-internal.h
@@ -12,7 +12,7 @@
  *    documentation and/or other materials provided with the distribution.
  * 3. The name of the author may not be used to endorse or promote products
  *    derived from this software without specific prior written permission.
- *
+ * 
  * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
  * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
  * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
@@ -133,6 +133,9 @@ struct event_base {
        struct event_list **activequeues;
        int nactivequeues;
 
+       /** The event whose callback is executing right now */
+       struct event *current_event;
+
        /** Deferred callback management: a list of deferred callbacks to
         * run active the active events. */
        TAILQ_HEAD (deferred_cb_list, deferred_cb) deferred_cb_list;
@@ -159,6 +162,9 @@ struct event_base {
        unsigned long th_owner_id;
        /** A lock to prevent conflicting accesses to this event_base */
        void *th_base_lock;
+       /** A lock to prevent event_del from deleting an event while its
+        * callback is executing. */
+       void *current_event_lock;
 #endif
 
 #ifdef WIN32

diff --git a/event.c b/event.c
index 0fdc9c8178b8149549d1b08c0fe2cb86b9ac9d0f..aaee1185a8fbb631e447c370f8179c2411ae9cea 100644 (file)
--- a/event.c
+++ b/event.c
@@ -301,6 +301,7 @@ event_base_new_with_config(struct event_config *cfg)
        if (!cfg || !(cfg->flags & EVENT_BASE_FLAG_NOLOCK)) {
                int r;
                EVTHREAD_ALLOC_LOCK(base->th_base_lock);
+               EVTHREAD_ALLOC_LOCK(base->current_event_lock);
                r = evthread_make_base_notifiable(base);
                if (r<0) {
                        event_base_free(base);
@@ -382,6 +383,7 @@ event_base_free(struct event_base *base)
        evmap_signal_clear(&base->sigmap);
 
        EVTHREAD_FREE_LOCK(base->th_base_lock);
+       EVTHREAD_FREE_LOCK(base->current_event_lock);
 
        mm_free(base);
 }
@@ -646,6 +648,10 @@ event_process_active_single_queue(struct event_base *base,
                        ev->ev_res & EV_WRITE ? "EV_WRITE " : " ",
                        ev->ev_callback));
 
+               base->current_event = ev;
+
+               EVBASE_ACQUIRE_LOCK(base, EVTHREAD_WRITE, current_event_lock);
+
                EVBASE_RELEASE_LOCK(base,
                    EVTHREAD_WRITE, th_base_lock);
 
@@ -663,9 +669,12 @@ event_process_active_single_queue(struct event_base *base,
                        break;
                }
 
+               EVBASE_RELEASE_LOCK(base, EVTHREAD_WRITE, current_event_lock);
+               EVBASE_ACQUIRE_LOCK(base, EVTHREAD_WRITE, th_base_lock);
+               base->current_event = NULL;
+
                if (base->event_break)
                        return -1;
-               EVBASE_ACQUIRE_LOCK(base, EVTHREAD_WRITE, th_base_lock);
        }
        return count;
 }
@@ -711,7 +720,7 @@ event_process_active(struct event_base *base)
                        activeq = base->activequeues[i];
                        c = event_process_active_single_queue(base, activeq);
                        if (c < 0)
-                               return; /* already unlocked */
+                               goto unlock;
                        else if (c > 0)
                                break; /* Processed a real event; do not
                                        * consider lower-priority events */
@@ -722,6 +731,7 @@ event_process_active(struct event_base *base)
 
        event_process_deferred_callbacks(base);
 
+unlock:
        EVBASE_RELEASE_LOCK(base, EVTHREAD_WRITE, th_base_lock);
 }
 
@@ -1267,11 +1277,13 @@ event_del(struct event *ev)
        return (res);
 }
 
+/* Helper for event_del: always called with th_base_lock held. */
 static inline int
 event_del_internal(struct event *ev)
 {
        struct event_base *base;
        int res = 0;
+       int need_cur_lock;
 
        event_debug(("event_del: %p, callback %p",
                 ev, ev->ev_callback));
@@ -1280,7 +1292,15 @@ event_del_internal(struct event *ev)
        if (ev->ev_base == NULL)
                return (-1);
 
+       /* If the main thread is currently executing this event's callback,
+        * and we are not the main thread, then we want to wait until the
+        * callback is done before we start removing the event.  That way,
+        * when this function returns, it will be safe to free the
+        * user-supplied argument. */
        base = ev->ev_base;
+       need_cur_lock = (base->current_event == ev);
+       if (need_cur_lock)
+               EVBASE_ACQUIRE_LOCK(base, EVTHREAD_WRITE, current_event_lock);
 
        assert(!(ev->ev_flags & ~EVLIST_ALL));
 
@@ -1310,6 +1330,9 @@ event_del_internal(struct event *ev)
        if (res != -1 && !EVBASE_IN_THREAD(base))
                evthread_notify_base(base);
 
+       if (need_cur_lock)
+               EVBASE_RELEASE_LOCK(base, EVTHREAD_WRITE, current_event_lock);
+
        return (res);
 }