On Sun, May 22, 2011 at 2:35 AM, Michael Herf <h...@pobox.com> wrote: > Ok, I made some progress, with help from Google's ThreadSanitizer! > http://code.google.com/p/data-race-test/wiki/ThreadSanitizer > > My libevent's http.c now allows me to hand off an evhttp request to a > worker, which can call evhttp_send_reply() safely (and nothing else after > that). Quite a bit closer to stable with a threadpool handling some of the > connections. The big advantage of this for me is: I can parse the http > request in the network thread, and then decide to hand "expensive" > operations to a worker, and handle event-driven stuff in the network thread.
Sounds like useful stuff! > A few notes so far: > 1. I added BEV_OPT_THREADSAFE in evhttp_connection_base_new (evcon->bufev = > bufferevent_socket_new(NULL, -1, BEV_OPT_THREADSAFE); + callbacks). This > means switching callbacks, etc., are threadsafe now. We won't want to do this unconditionally: not everybody will want the overhead of locking in an evhttp that isn't going to be used in multiple threads. > 2. I refcount bufev while the thread is writing. In particular, > evhttp_send_reply does incref while it's running (decref after, but > carefully because request may have been deleted). This prevents the network > thread from deleting the bufferevent in the middle of the send. > > Third patch I'm not sure about, but maybe someone can provide insight if it > matters or not: > 3. Move bufferevent_enable(evcon->bufev, EV_WRITE); in all cases (http.c) to > the end of each function, setting up callbacks first. This ensures the > network thread uses the correct callbacks, so it doesn't race. (Each call > does a BEV_LOCK separately, so the order does matter.) There are a few of > these I've missed below. > > My #2 patch needs to be generalized (for other code paths: chunked, etc.) > perhaps evhttp_write_buffer should do the refcount instead? But appears to > be the important piece for this. > > Here's the patch as-is (needs quite a lot of cleanup): First thing to do is, in the future, please use the "unified diff" format that "diff -u" generates? It's lots easier to read. As for the patch itself: there's going to be more work needed to make evhttp threadsafe. It looks like most of what you're doing so far is to make evhttp's use of bufferevents threadsafe. That's a necessary component, but we'll also need to make sure that the evhttp structures themselves are safe to use from multiple threads at a time. That means at a minimum getting them locks to prevent concurrent modifications. We'll also need to figure out the correct behavior wrt preventing deadlocks between http locking and bufferevent locking -- possibly by the expedient of just having the bufferevent and its evhttp objects share a lock. yrs, -- Nick *********************************************************************** To unsubscribe, send an e-mail to majord...@freehaven.net with unsubscribe libevent-users in the body.
