#14 Move EV_PERSIST logic into event.c, and delay deletes.

closed
nobody
None
5
2007-11-27
2007-11-10
Nick Mathewson
No

Right now, the logic for removing events without EV_PERSIST is implemented independently in every event handler. That's duplicate code, and that's unfortunate.

Worse still, it makes writing an efficient win32 select-based handler hard. The reasonable algorithm is (assuming a socket-to-event map):
for sockets in readfds:
event_active(map[socket], EV_READ)
for sockets in writefds:
event_active(map[socket], EV_WRITE)

But if we need to handle removing non-persistent events as we call event_active, it's harder to later add EV_WRITE to the now non-pending event.

Instead, could setting EV_PERSIST make event.c delete events before it calls their callbacks? That would remove duplicate code, simplify backends, and make win32.c less totally awful.

Discussion

  • Logged In: YES
    user_id=873020
    Originator: NO

    I have a feeling this could be rolled into changes with the EV_PERSIST logic from the other bug ("timer interactions"). In addition it would kill the periodic timers feature request as well, as EV_PERSIST would also apply to timers.

    One small issue is timeout_process() and min_heap_top(), but it's a micro-detail.

    I'll get back to work on it and see what I can come up with. The EV_PERSIST patch itself is done, with regressions written as well, but this could could/should probably go along with it.

     
  • Nick Mathewson
    Nick Mathewson
    2007-11-14

    Logged In: YES
    user_id=499
    Originator: YES

    BTW, one slightly subtle thing: every activated non-persistent event needs to get deleted before we call _any_ event's callbacks. Consider what happens when ev1 has a callback that calls event_add(ev2), and ev2 is non-persistent. If the internal delete for ev2 is triggered after the ev1 callback, the user will wind up with unexpected results.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    I'm pretty sure we should be safe from this.

    event_active() can now function as a conduit which will saturate a specific event's watched events (that code is already there in the early return), reschedule timeout if persist, otherwise delete it once if it's not on the active queue, and return.

    event_process_active() will then pick up from there after we return from the individual dispatches.

    I think this is the way to go and while it does couple event_active() to event_del() in an implicit way, it should perform better and result in less sinew in the code.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    Just to sanity check this, in your example, we're talking something like this, right?

    read_cb()
    {
    if (event & EV_READ) {
    do_my_stuff(obj);
    /* persistent, we don't readd ourselves */
    /* but we do add this write event to push data out */
    event_add(obj->ev_write, write_tout);
    }
    }

    write_cb()
    {
    if (event & EV_WRITE)
    do_my_stuff(obj);
    /* implicit event_del */
    }

     
  • Nick Mathewson
    Nick Mathewson
    2007-11-14

    Logged In: YES
    user_id=499
    Originator: YES

    Yes, that's the example I had in mind. Also consider the example of how win32 select should work:

    for socket in readfds:
    event_active(map[socket], EV_READ)
    for sockets in writefds:
    event_active(map[socket], EV_WRITE)

    If some event is listening for read *and* write on a socket, and event_active calls event_del, and event_del removs the event from the socket-to-event map, I think we're in trouble when we get to the second loop.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    "If some event is listening for read *and* write on a socket, and
    event_active calls event_del, and event_del removs the event from the
    socket-to-event map, I think we're in trouble when we get to the second
    loop."

    Yep, I see what you mean with that one.

    This is something that breaks EV_READ|EV_WRITE as it is now, actually.

    Use res passed to event_active() and mask off what
    just occured. Once mask is empty, then call event_del():

    if (~ev->ev_events & EV_PERSIST)
    ev->ev_events &= ~res;
    else if (ev->ev_events == 0)
    event_del(ev);

    Which as far as I can tell should work sanely. Sanity check?

     
  • Nick Mathewson
    Nick Mathewson
    2007-11-14

    Logged In: YES
    user_id=499
    Originator: YES

    Alas, that won't work, for two reasons:
    1) We want to remove the event if _either_ read or write triggers, not only when both have triggered.
    2) It's supposed to be okay to re-add an event that has triggered and been removed. If we alter ev_events to remove EV_READ and/or EV_WRITE and the user then re-adds the event, the behavior will not be as expected.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    Woops. I was even thinking that internally ("don't forget about event_add()") but somehow forgot it anyways.

    What do you think is the best approach?

     
  • Nick Mathewson
    Nick Mathewson
    2007-11-14

    Logged In: YES
    user_id=499
    Originator: YES

    The best that I can think to do right now is another list in the event_base, of non-persistent active events. Call it the "pending deletion" list. event_active() can add to this list when it makes a non-persistent previously non-active event active. event_process_active() then removes these events before it processes any events.

    (As for the other lists, there's an EVLIST_ flag that's set if the event has been added to the pending deletion list.)

    Of course, that last part will be a little tricky: event_del makes an active event non-active. We don't actually want to do this for events on the "pending deletion" list.

    I'm not sure that this is the simplest implementation, but it seems to be correct.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    Well one way would just be pushing event_del() to event_process_active(). I'm currently playing with that, but as I'm sure you may already know, it has some stickyness associated with it.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    Actually - it's simpler than I thought to delay the delete until event_process_active(). I've got the regression tests passing, and it will obey ncalls, because the loop abort is only done if pncalls is actually set (which is won't be until after event_del() from within event_process_active()), but I'm still analyzing things for gremlins.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    This is what I have now, and what works for me, and what I believe *should* work:

    event_process_active:
    [...]
    for (ev = TAILQ_FIRST(activeq); ev; ev = TAILQ_FIRST(activeq)) {
    event_queue_remove(base, ev, EVLIST_ACTIVE);
    if (~ev->ev_events & EV_PERSIST)
    event_del(ev);

    /* process */
    }
    [...]

    event_active:
    [...]
    /* We get different kinds of events, add them together */
    if (ev->ev_flags & EVLIST_ACTIVE) {
    ev->ev_res |= res;
    return;
    }

    ev->ev_res = res;
    ev->ev_ncalls = ncalls;
    ev->ev_pncalls = NULL;

    if ((ev->ev_events & EV_PERSIST) && (ev->ev_flags & EVLIST_TIMEOUT))
    timeout_schedule(ev->ev_base, ev, &ev->ev_timeout_rel);
    event_queue_insert(ev->ev_base, ev, EVLIST_ACTIVE);
    [...]

    timeout_process:
    [...]
    while ((ev = min_heap_top(&base->timeheap))) {
    if (evutil_timercmp(&ev->ev_timeout, &now, >))
    break;
    if (~ev->ev_events & EV_PERSIST)
    event_queue_remove(base, ev, EVLIST_TIMEOUT);

    event_debug(("timeout_process: call %p", ev->ev_callback));
    event_active(ev, EV_TIMEOUT, 1);
    }
    [...]

    If we can define that any event which becomes active will immediately delete (and reschedule if PERSIST) any pending timeout associated with it - this could be simplified more by moving event_queue_remove(..., EVLIST_TIMEOUT) into event_active(), accepting the side-effect as "by design." Otherwise, it is at it is.

    This should delay any deletions until after we're out of any dispatch loop without needing an extra queue and functioning fairly logically, what do you guys think?

     
  • Nick Mathewson
    Nick Mathewson
    2007-11-17

    Logged In: YES
    user_id=499
    Originator: YES

    That approach fails because of the reason in my comment on Date: 2007-11-13 21:17:

    "BTW, one slightly subtle thing: every activated non-persistent event needs
    to get deleted before we call _any_ event's callbacks. Consider what
    happens when ev1 has a callback that calls event_add(ev2), and ev2 is
    non-persistent. If the internal delete for ev2 is triggered after the ev1
    callback, the user will wind up with unexpected results."

    Suppose that ev1 is in activeq before ev2. The code for event_process_active that you give below will do the following:
    process ev1. This will call event_add on ev2, but ev2 is already added.
    event_del(ev2)
    process ev2.

    This will break code that assumes ev1 can re-add ev2.

     
  • Logged In: YES
    user_id=873020
    Originator: NO

    Right.

    The queue of "now active, but probably going to need deletion" is there, in the form of activeq and ~EV_PERSIST, it's just how activeq is affected during processing it. Surely within the activeq section, rather than pop the head, one can just traverse and event_queue_remove non-persistent events from all queues but the activeq, then pop off in reverse order (or start from front again if necessary), and call callbacks. Either way, one is looking at similar costs.

    If another queue is going to be added, it might as well be added to separate EV_READ and EV_WRITE (as that's all EVLIST_INSERTED really is) - rather than a specific "intemediary state" queue.

     
  • Nick Mathewson
    Nick Mathewson
    2007-11-19

    Logged In: YES
    user_id=499
    Originator: YES

    The approach you suggest is reasonable.

    The only reason that a multi-queue approach would do better is if there are lots of active events, most of which are not-persistant. But optimizing for this case isn't necessary in a first version.

     
  • Niels Provos
    Niels Provos
    2007-11-27

    Logged In: YES
    user_id=245089
    Originator: NO

    a fix has been put into trunk; we event_del() right before executing the callback now; this establishes the invariant that when a non persist event is executed that the event is never pending.

     
  • Niels Provos
    Niels Provos
    2007-11-27

    • status: open --> closed