[Libevent-users] EV_PERSIST + relative timeouts discussion

[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