Fix a nasty bug in event_queue_reinsert_timeout()

What was I thinking?  The old function could handle heap-to-heap
transitions, and transitions within the same common timeout queue, but
it completely failed to handle heap/queue transitions, or transitions
between timeout queues.

Now, alas, it's complicated.  I should look hard at the assembly here
to see if it's actually better than the alternatives.

diff --git a/event.c b/event.c
index e5ed691cf01469e14593be15ca5f1c8d6fec2d2c..432d0060e7bc86ecf775e28a2ee5a14dd27bfa59 100644 (file)
--- a/event.c
+++ b/event.c
@@ -138,7 +138,7 @@ static void event_queue_insert_inserted(struct event_base *, struct event *);
 static void    event_queue_remove_active(struct event_base *, struct event *);
 static void    event_queue_remove_timeout(struct event_base *, struct event *);
 static void    event_queue_remove_inserted(struct event_base *, struct event *);
-static void    event_queue_reinsert_timeout(struct event_base *,struct event *);
+static void    event_queue_reinsert_timeout(struct event_base *,struct event *, int was_common, int is_common, int old_timeout_idx);
 
 static int     event_haveevents(struct event_base *);
 
@@ -2214,6 +2214,8 @@ event_add_internal(struct event *ev, const struct timeval *tv,
        if (res != -1 && tv != NULL) {
                struct timeval now;
                int common_timeout;
+               int was_common;
+               int old_timeout_idx;
 
                /*
                 * for persistent timeout events, we remember the
@@ -2245,6 +2247,9 @@ event_add_internal(struct event *ev, const struct timeval *tv,
                gettime(base, &now);
 
                common_timeout = is_common_timeout(tv, base);
+               was_common = is_common_timeout(&ev->ev_timeout, base);
+               old_timeout_idx = COMMON_TIMEOUT_IDX(&ev->ev_timeout);
+
                if (tv_is_absolute) {
                        ev->ev_timeout = *tv;
                } else if (common_timeout) {
@@ -2261,7 +2266,7 @@ event_add_internal(struct event *ev, const struct timeval *tv,
                         "event_add: event %p, timeout in %d seconds %d useconds, call %p",
                         ev, (int)tv->tv_sec, (int)tv->tv_usec, ev->ev_callback));
 
-               event_queue_reinsert_timeout(base, ev);
+               event_queue_reinsert_timeout(base, ev, was_common, common_timeout, old_timeout_idx);
 
                if (common_timeout) {
                        struct common_timeout_list *ctl =
@@ -2673,21 +2678,40 @@ event_queue_remove_timeout(struct event_base *base, struct event *ev)
 
 /* Remove and reinsert 'ev' into the timeout queue. */
 static void
-event_queue_reinsert_timeout(struct event_base *base, struct event *ev)
+event_queue_reinsert_timeout(struct event_base *base, struct event *ev,
+    int was_common, int is_common, int old_timeout_idx)
 {
+       struct common_timeout_list *ctl;
        if (!(ev->ev_flags & EVLIST_TIMEOUT)) {
                event_queue_insert_timeout(base, ev);
                return;
        }
 
-       if (is_common_timeout(&ev->ev_timeout, base)) {
-               struct common_timeout_list *ctl =
-                   get_common_timeout_list(base, &ev->ev_timeout);
+       switch ((was_common<<1) | is_common) {
+       case 3: /* Changing from one common timeout to another */
+               ctl = base->common_timeout_queues[old_timeout_idx];
                TAILQ_REMOVE(&ctl->events, ev,
                    ev_timeout_pos.ev_next_with_common_timeout);
+               ctl = get_common_timeout_list(base, &ev->ev_timeout);
                insert_common_timeout_inorder(ctl, ev);
-       } else {
+               break;
+       case 2: /* Was common; is no longer common */
+               ctl = base->common_timeout_queues[old_timeout_idx];
+               TAILQ_REMOVE(&ctl->events, ev,
+                   ev_timeout_pos.ev_next_with_common_timeout);
+               min_heap_push_(&base->timeheap, ev);
+               break;
+       case 1: /* Wasn't common; has become common. */
+               min_heap_erase_(&base->timeheap, ev);
+               ctl = get_common_timeout_list(base, &ev->ev_timeout);
+               insert_common_timeout_inorder(ctl, ev);
+               break;
+       case 0: /* was in heap; is still on heap. */
                min_heap_adjust_(&base->timeheap, ev);
+               break;
+       default:
+               EVUTIL_ASSERT(0); /* unreachable */
+               break;
        }
 }
 

diff --git a/test/regress.c b/test/regress.c
index 9f9ad50e7241dd37dcc67a04315a518e77390c34..e3f8dec8762e70662c2b47b3ca0075f7dcd85765 100644 (file)
--- a/test/regress.c
+++ b/test/regress.c
@@ -733,7 +733,23 @@ test_common_timeout(void *ptr)
                event_assign(&info[i].ev, base, -1, EV_TIMEOUT|EV_PERSIST,
                    common_timeout_cb, &info[i]);
                if (i % 2) {
-                       event_add(&info[i].ev, ms_100);
+                       if ((i%20)==1) {
+                               /* Glass-box test: Make sure we survive the
+                                * transition to non-common timeouts. It's
+                                * a little tricky. */
+                               event_add(&info[i].ev, ms_200);
+                               event_add(&info[i].ev, &tmp_100_ms);
+                       } else if ((i%20)==3) {
+                               /* Check heap-to-common too. */
+                               event_add(&info[i].ev, &tmp_200_ms);
+                               event_add(&info[i].ev, ms_100);
+                       } else if ((i%20)==3) {
+                               /* Also check common-to-common. */
+                               event_add(&info[i].ev, ms_200);
+                               event_add(&info[i].ev, ms_100);
+                       } else {
+                               event_add(&info[i].ev, ms_100);
+                       }
                } else {
                        event_add(&info[i].ev, ms_200);
                }