Re: [Libevent-users] workers for evhttp (was stable, now not!)

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Sun, May 22, 2011 at 2:35 AM, Michael Herf <herf@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.