On Thu, Jun 9, 2011 at 5:47 PM, Joachim Bauch <j...@struktur.de> 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 majord...@freehaven.net with unsubscribe libevent-users in the body.
