Menu

#202 EVLOOP_ONCE doesn't handle internal events and deferred cbs

For_2.0
closed-fixed
7
2010-11-26
2010-11-13
No

From a post of mine to libevent-users:

The event_add locks the event_base's structures, makes the changes
necessary to add the event, and then notices that it isn't running in
the main thread, so it needs to "alert" the main thread to exit and
re-enter the dispatch function[*]. It does this by calling
evthread_notify_base(), which sends a byte down a pipe (on most Unix)
or a socketpair (on Windows), or by using an eventfd (on Linux) [**].
There's an (EVLIST_INTERNAL) event handler for this
pipe/socketpair/eventfd that notices that we've been "notified" so we
can drain the pipe/socketpair/eventfd.

Now here's where I think the bug is: that event counts as an active
event, and processing it gets counted as processing an event, so if
you're running the loop with EVLOOP_ONCE, the loop will exit right
after it notices that it should wake up, even though no user callbacks
were actually entered.

The logic is in event_base_loop():

if (N_ACTIVE_CALLBACKS(base)) {
event_process_active(base);
if (!base->event_count_active && (flags & EVLOOP_ONCE))
done = 1;
} ...

I think that the second "if" might be wrong. We maybe want to
continue looping not only if there are active callbacks, but also if
the only active events that we processed were internal events.

With the current behavior, with EVLOOP_ONCE, I think adding the event
from another thread will exit the loop right away. Is that consistent
with the behavior you're seeing? If not, then the bug is possibly in
the notification stuff, though that part actually is fairly well
tested.

It's also possible that I'm writing this too late at night for me, and
I've deeply misanalyzed the code here.

(As an aside, we should probably also continue looping if
N_ACTIVE_CALLBACKS(base) is nonzero.)

Discussion

  • Nick Mathewson

    Nick Mathewson - 2010-11-15

    See branch 20_once_fixes in my github repository.

    I also cleaned up the documentation: the declared "block at most once" behavior of evloop_once was a little misleading: it could be interpreted to mean that we would invoke the backend's dispatch function only once, when in fact we only do so in a _blocking way_ at most once.

     
  • Nick Mathewson

    Nick Mathewson - 2010-11-24

    oops; I forgot to actually push the branch. Should be there now.

     
  • Nick Mathewson

    Nick Mathewson - 2010-11-26

    Applied after review by Chris; closing.

     
  • Nick Mathewson

    Nick Mathewson - 2010-11-26

    Applied after review by Chris; closing.

     
  • Nick Mathewson

    Nick Mathewson - 2010-11-26
    • status: open --> closed-fixed
     

Log in to post a comment.