Re: [Libevent-users] Send evbuffer to multiple clients

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Thu, Jun 9, 2011 at 5:47 PM, Joachim Bauch <jojo@xxxxxxxxxxx> wrote:

Nice stuff; we're getting closer here.

> Am Mittwoch, den 08.06.2011, 13:15 -0400 schrieb Nick Mathewson:
>> Could we simplify stuff by also declaring that if you
>> have an evbuffer which is added by reference to another evbuffer, the
>> entire original evbuffer is immutable?  It seems to me that would make
>> the rules for what you can use these for much easier to understand,
>> and it would prevent people from doing crazy stuff.
>
> There is no technical reason the original buffer should not be mutable
> after it was added to another buffer. Adding it takes the current
> contents as a snapshot. We must only take care that the referenced
> chains don't change. Also this keeps the code a lot simpler.

Hm.  I worry that people will expect changes to the source buffer to
appear after the source buffer is added.  Ah well, we can fix that one
with

 [...]
>> I think there's a race condition around evbuffer_chain_decref.  The
>> reference count needs to be protected by the lock of the evbuffer that
>> contains the referenced-chain, but right now it looks like it's only
>> protected by the lock of the evbuffer that contains the referencing
>> chain.
>
> Yep, there were also locking calls missing if an already-referencing
> chain was added to another buffer.

I think the locking mechanism has a few potential deadlocks and race
conditions we need to worry about.

1) For starters, consider the case where we add some data to buf1,
then add data to buf2.  Then we add buf1's contents by reference to
buf2, and add buf2's contents by reference to buf1.

Now suppose that thread decids to A drain buf1 completely while at the
same time thread B drain buf2 completely.  The drain operation in A
will grab buf1's lock, and the drain operation in B will grab buf2's
lock.  But when thread A gets to the EVBUFFER_MULTICAST chunk, it
won't be able to get the lock on it, since thread B will be holding
it.  And thread B will want to grab the lock that A is holding, so
we'll have a deadlock and neither thread will be able to progress.

2) Suppose that we add buf1's contents by reference to buf2.   Every
EVBUFFER_MULTICAST chain in buf2 now holds a pointer to buf1's lock.
Now say we free buf1.  This causes buf1's lock to get freed, so the
chains in buf2 now have dangling lock pointers.

I'm not sure what the easiest fix is here.  One option that would
solve both of these cases is to have referenced evbuffers be
immutable, and to use the evbuffer's refcnt rather than putting
refcnts onto individual chains.  By making referenced buffers
immutable, we could prevent any circular add-by-reference patterns
like the one behind 1) above, and by using the evbuffer's refcnt, we
could ensure that its lock wasn't freed until all the buffers
referencing it had also been freed.  But this would be a reduction in
functionality (though not one I'd argue with), and I wonder if there
might be other deadlock opportunities here that I haven't though of.

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