From 8c36acd0b0b828c3cdf80972b38867285d21faec Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 23 Mar 2012 18:42:56 -0400 Subject: [PATCH] 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. --- event.c | 38 +++++++++++++++++++++++++++++++------- test/regress.c | 18 +++++++++++++++++- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/event.c b/event.c index e5ed691c..432d0060 100644 --- 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 9f9ad50e..e3f8dec8 100644 --- 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); }