Re: [Libevent-users] RFC: strategy for deadlock avoidance and safe finalization in Libevent 2.1

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Wed, Apr 10, 2013 at 3:51 AM, Alexander Drozdov <dzal_mail@xxxxxxxxxx> wrote:
> The idea is good, but why to not just register finalizer callback on
> event initialization? A data pointer is passed to event_assign() or
> event_new()
> so we can pass finalizer along with the data.

Hi, Alexander! Thanks for the feedback.

My main reasons were:

   * Adding an extra function pointer in every event_callback
structure[*] seemed like something to avoid.
   * There's a handy "finalize_many" pattern I wanted to use for
bufferevents to avoid having to call 6-8 finalizer functions on every
single bufferevent_free()[*].  That function has weird semantics,
though, that are less compatible with the .
   * To me, it seemed more intuitive and easy to understand than the
alternative.  This way, it is explicit what code will get invoked by
event_free_finalize() and event_finalize().  (There's going to be
action-at-a-distance either way: either the finalization call doesn't
tell you what will happen, or the setup call doesn't tell you how the
code will finalize it later.)

> With this approach,
> - finalizer, if set, will be called automatically after calling
>   event_free() or event_del() and after event execution

event_del() wouldn't be an appropriate time to do that.  event_del()
doesn't mean we're tearing down a struct event; it means that we're
turning it from active and/or pending into non-active, non-pending.

> - The event initialization code includes reference to event deinitialization
> one,
>   which makes the code easier to understand

That part does sound like a good thing, if the current approach really
is hard to understand. (I wonder how others feel about this point?)

> - event_assign(), event_new() and event_get_assignment() should be extended
>   (for compatibility, new functions should be added)

Or, if you wanted to do this, a single event_set_finalizer() function
seems like it would work fine too.

> - event_get_finalizer() should also be added
> - event_del_noblock() & event_free_noblock() or event_del_ext() &
> event_free_ext()
>   with extra flags is still needed. One extra flag DONT_FINALIZE is
> desirable to
>   allow user to call finalizer manually.

Again, event_del() does not mean that we should finalize an event.
Remember, after event_del(), it's legal to call event_add() on the
same event again, so we would need to have an explicit
event_finalize() call.


So, how do other folks feel?  Is it really more readable to specify a
cleanup function at event initialization time than at event teardown
time?  Is it worth it?

I'll go with something like Alexander's proposal if there seems like a
rough consensus that it's better.


[*] event_callback is a new feature in the 2.1 backend logic: it
unifies "deferred_callback" and "event" by giving them a supertype. If
we convert bufferevent and evbuffer callbacks to event_callback too,
we can simplify their code significantly, and use common logic to make
all the callbacks in libevent get handled more efficiently.

best wishes,
-- 
Nick
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.