#15 EV_PERSIST and timeout interactions

closed
nobody
None
5
2014-08-26
2007-11-13
No

From Christopher Layne:
As it is, EV_PERSIST is somewhat obtuse when trying to use it in combination
with EV_READ or EV_WRITE while still handling timeout events. The standard
idiom is basically:

if (select_epoll_etc_indicates_something)
do_something_and_add_fd_back_to_something_set();
else if (time_since_last_something > something_timeout)
our_client_or_server_is_dead_lets_cleanup();

One method is to do something like this with EV_PERSIST is:

void read_cb(int fd, short event, void *a)
{
opaque *obj = a;
ssize_t l;

switch (event) {
case EV_TIMEOUT:
close_cleanup_whatever(fd);
opaque_reset(obj);
event_del(obj->ev_r);
break;
case EV_READ:
l = recv(fd, ...);
/*
* Here we force a timeout reschedule. if we don't,
* our timeout will eventually trigger, regardless
* of the fact that we've been processing active events.
* It isn't really added with EV_PERSIST, just the timeout
* is rescheduled.
*/
event_add(obj->ev_r, obj->tout_recv_tv);
break;
default: break;
}

return;
}

Same goes for EV_SIGNAL and EV_TIMEOUT events. event_add(event, timeout),
always, if you want to reschedule the timer associated with it (which for
all intensive purposes you almost always will if you're using EV_PERSIST
in the first place).

Now it's not like this is a huge hassle, but it does seem like it would
be more inutitive for libevent to handle latching timers of this sort when
using EV_PERSIST. I've written a patch to handle latching timers when using
EV_PERSIST and currently I can see some pros and cons.

Pros:
1. EV_PERSIST + anything else works intuitively without having to worry
about managing the timeout one passed to event_add(). Set it and
forget it.
2. For some people it may be one less thing to truck around in their god
objects. I.e. the space taken up by storing the relative timer within
the event struct itself is balanced by the space saved by not carrying
it around in user objects. Not a gain, however, for people using
global timeout structs.
3. It's not really much of a boogeyman to current users of libevent as the
people who are using EV_PERSIST typically are aware of said timeout
issues. Also, the current workaround of event_add() will work fine
in existing code.
4. Focuses on optimizing the standard case rather than the exception (timeout).

Cons:
1. Cyclic timer still needs event_add() in the callback, as a timeout is
still setup to delete the event automatically. I'm torn on this. If
it's changed it will break current behaviour. If it isn't changed,
then it's another thing to remember when using the library - that is
events registered with EV_PERSIST will have their timeouts reset on
activity, however a timeout occuring will automatically delete the
event as per normal. In day to day usage this is probably a preferred
idiom though, even though it's an exception to the latching timer
pattern. One concern though is that since things are less explicit,
flow is not as self-evident.
2. Timeout rescheduling is done when event_active() is called. This is not
entirely precise because event timeouts may end up being rescheduled
to a slightly earlier moment in time. One way to get around this is to
move timeout_schedule() to event_process_active(), but it would take
some reworking of timeout_process().
3. Maybe this is all just a waste of time and we should just continue to use
event_add(obj, obj->tout);

With the model changed, it would look like this:

void read_cb(int fd, short event, void *a)
{
opaque *obj = a;
ssize_t l;

switch (event) {
case EV_TIMEOUT:
close_cleanup_whatever(fd);
opaque_reset(obj);
break;
case EV_READ:
l = recv(fd, ...);
/* etc */
break;
default: break;
}

return;
}

void dispatcher(opaque *obj)
{
event_set(obj->ev_r, obj->fd, EV_READ | EV_PERSIST, read_cb, obj);
event_add(obj->ev_r, obj->tout_recv_tv);
return;
}

Interested to hear thoughts before I go posting patches.

-cl

Discussion

  • Niels Provos

    Niels Provos - 2007-11-13

    Logged In: YES
    user_id=245089
    Originator: YES

    A quick search on Google:

    http://www.google.com/codesearch?q=_set%5C%28.\*EV_PERSIST&hl=en&btnG=Search+Code

    revealed that the prevalent usage of EV_PERSIST is without timeouts. Probably, because the timeout behavior is a little bit wonky. We might be able to just change the behavior without breaking any existing code.

    Nick, do you have an opinion on this?

     
  • christopher layne

    Logged In: YES
    user_id=873020
    Originator: NO

    http://sourceforge.net/tracker/index.php?func=detail&aid=1826578&group_id=50884&atid=461325
    http://sourceforge.net/tracker/index.php?func=detail&aid=1829383&group_id=50884&atid=461325

    The patch is done, I'll post it to libevent-users - but one thing I'm still torn on is..

    Break existing EV_TIMEOUT behaviour by making it abide by EV_PERSIST rules, or explicitly not
    mess with deletion and only reschedule (event_add() [which people use for the same purpose] is harmless if called in the callback).

    Personally, I vote to break the funky behavior, and add the following backwards compat flag:
    EV_TIMEOUT_DEL - which is also a useful future flag in that it specifies "if an event is EV_PERSIST, but a timeout happens, delete the event". Alternatively we could flip the logic and say EV_PERSIST_TIMEOUT which would keep current behaviour but allow timeouts to follow the EV_PERSIST (future) logic.

     
  • Nick Mathewson

    Nick Mathewson - 2007-11-17

    Logged In: YES
    user_id=499
    Originator: NO

    I prefer the latter behavior. Instead of EV_PERSIST_TIMEOUT, I'd prefer having it called EV_TIMEOUT_PERIODIC or something. The semantics for periodic timeouts are different enough from regular EV_TIMEOUT and EV_PERSIST semantics that I think they'd deserve a separate flag.

     
  • Niels Provos

    Niels Provos - 2007-11-18

    Logged In: YES
    user_id=245089
    Originator: YES

    After some spending some more thought on this, I agree with Nick, that it probably deserves a separate flag. This patch might also be easier to implement once EV_PERSIST has been pulled out of the event backends.

     
  • christopher layne

    Logged In: YES
    user_id=873020
    Originator: NO

    Alright.

    Nick: How does a periodic timer differ from a persistent EV_TIMEOUT event?

    Either one is doing event_add() in the callback to keep the timer persistent, or they're letting a flag like EV_PERSIST do it for them. Which layer is resetting the timer is a matter of semantics.

    BTW: On another semi-related note, a bug that I just came across today, in present code, is that event_add() does not remove a timeout if passed a NULL tv.

     
  • Nick Mathewson

    Nick Mathewson - 2007-11-19

    Logged In: YES
    user_id=499
    Originator: NO

    IMO, periodic timers are the same as the semantics you've suggested for periodic timeout events. There is some question as to what to do when the system clock moves around, but they're IMO orthogonal to the other semantics

    The reasons I'm suggesting a different flag name are:
    1) so as not to break old code that relies on the current EV_PERSIST|EV_TIMEOUT behavior.
    2) because, IMO, if you want a timer that happens every 15 seconds, it is more reasonable to say EV_TIMEOUT_PERIODIC than to have to know that EV_PERSIST|EV_TIMEOUT means "periodic" in libevent 1.5 and later.

    Is that what you were asking?

     
  • Niels Provos

    Niels Provos - 2008-05-03

    Logged In: YES
    user_id=245089
    Originator: YES

    Trunk has periodic timer support now; please check if that does what you want. If so, we can probably close this feature request.

     
  • Forest

    Forest - 2008-05-13

    Logged In: YES
    user_id=27617
    Originator: NO

    Some thoughts:

    If a timed event fires at a predetermined time, regardless of data on the socket, then I think of it as a "timer", not a "timeout". Of course, a data callback can continually re-schedule a timer to achieve timeout-like behavior, but the underlying mechanism is still just a timer to me. This is the behavior I observe in libevent, so I say our favorite little event library has a mis-named feature. It had me rather puzzled for a while.

    As I understand the situation, we are considering these questions:

    1. Should we change libevent's timeout behavior when EV_PERSIST is present?

    In my view, the current behavior is useless. If I wanted a timed event to fire regardless of when my persistent data event fires, I would use a stand-alone timer. It's also confusing, because it is not actually data timeout behavior despite being documented as such.

    In order to avoid an API that allows me to specify useless and confusing behavior, I suggest we change the flag's timeout behavior. Persistent event timeouts should stop their countdown whenever the socket becomes data-ready, and start a new countdown when the socket becomes unready. (I hope I just described those rules correctly. :)

    2. Should the firing of a timeout cancel the persistence of its data event?

    I think the application should control this. I expect a persistent event to actually persist, until told to stop. One app might want to stop listening for data when a timeout occurs, while another might want to send a keepalive and keep listening. Therefore, libevent probably should not unilaterally delete a persistent event when EV_TIMEOUT is fired. Let the application do so in cases where it makes sense.

    3. If we make a change, how do we handle backward compatibility?

    Here's a possible migration path:

    1. Officially deprecate EV_PERSIST, but leave it in place for now.
    2. Make event_add() fail if the caller passes a timeout value for an event with EV_PERSIST.
    3. Introduce a new flag for requesting the new persistent event behavior.
    4. Remove EV_PERSIST in some future release.

    This should allow many projects to continue working with deprecated flag, so long as they don't use the (broken) combination of EV_PERSIST and EV_TIMEOUT. It should also shake out any projects that need updating to use the new flag, thus avoiding introduction of hidden bugs.

     
  • Nick Mathewson

    Nick Mathewson - 2008-05-14

    Logged In: YES
    user_id=499
    Originator: NO

    Hi, Forest!

    I haven't been into the timer code in a little while, so let me summarize for my own benefit. ;) I understand it, the situation is:
    1) Periodic timers are genuinely periodic timers. That's fine.
    2) EV_PERSIST on timers with no corresponding events does nothing useful; we can safely deprecate or disable it.
    3) EV_PERSIST on events with both an fd and a timer has the behavior that the timer will elapse independently of how many times the event is triggered. This behavior is not useful to you and you believe it may not be very useful for anybody.
    4) It would be good to have a flag like EV_PERSIST for events with fds and timers such that the timeout is rescheduled whenever the fd event fires. This is the behaviore you'd like another flag to provide.

    Sounds reasonable to me, though I rather suspect we're stuck leaving the "feature" in case 3 in for a while.

    Niels, thoughts?

     
  • Niels Provos

    Niels Provos - 2008-05-15

    Logged In: YES
    user_id=245089
    Originator: YES

    EV_PERSIST is clearly useful for IO events by themselves. The confusion happens when they are combined with timeouts. I would prefer to make associated timeouts with EV_PERSIST repeat at a fixed interval and break existing code. The rationale would be that the behavior of EV_PERSIST with timeout was undocumented. We'd better make sure that we document it now though. I am okay with failing EV_PERSIST if there are no associated IO events.

     
  • Forest

    Forest - 2008-05-16

    Logged In: YES
    user_id=27617
    Originator: NO

    > Hi, Forest!

    Hi!

    > 2) EV_PERSIST on timers with no corresponding events does nothing
    > useful; we can safely deprecate or disable it.

    I have no opinion on that, probably because I'm not sure what the current behavior is. :)

    > 3) EV_PERSIST on events with both an fd and a timer has the behavior
    > that the timer will elapse independently of how many times the event is
    > triggered. This behavior is not useful to you and you believe it may not
    > be very useful for anybody.

    That's a reasonable summary. Separated from my earlier (wordy) explanation, though, I might rephrase the last sentence to, "this behavior is easily implemented and more clearly represented as a separate timer event."

    > 4) It would be good to have a flag like EV_PERSIST for events with fds
    > and timers such that the timeout is rescheduled whenever the fd event
    > fires. This is the behaviore you'd like another flag to provide.

    Yes.

    My first reading of the docs actually had me thinking this behavior was already provided. I guess that's because the Timeouts section says, "In addition to simple timers, libevent can assign timeout events to file descriptors that are triggered whenever a certain amount of time has passed with no activity on a file descriptor." That quote gave the impression that "timeouts" were aware of activity on a file descriptor. Now, looking more closely, I see that the activity-awareness described in the docs actually has to be implemented by the application.

    > Sounds reasonable to me, though I rather suspect we're stuck leaving the
    > "feature" in case 3 in for a while.

    I suspect you're right. Backward compatibility is valuable.

     
  • Niels Provos

    Niels Provos - 2009-01-22

    We changed the semantics of EV_PERSIST in conjunction with timeouts, so that they become periodic or get re-added when the event becomes active.

     
  • Niels Provos

    Niels Provos - 2009-01-22
    • status: open --> closed
     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks