From 5dc566284d736cb692f88685a3bcec70595a7689 Mon Sep 17 00:00:00 2001 From: Tomash Brechko Date: Thu, 24 Feb 2011 12:30:40 +0300 Subject: [PATCH 1/2] Workaround libevent bug https://sourceforge.net/tracker/index.php?func=detail&aid=3078187&group_id=50884&atid=461324 The problem is that bufferevent_disable() doesn't disable EV_WRITE when 'connecting' flag is set. However from evhttp_connection_reset() we want to disable EV_WRITE for sure (we are closing the socket next). So we add bufferevent_disable_hard(), which acts like bufferevent_disable(), but resets 'connecting' flag before the call to the actual handler. TODO: bufferevent_disable_hard() shouldn't be public, remove it from event2/bufferevent.h. --- bufferevent.c | 18 ++++++++++++++++++ http.c | 2 +- include/event2/bufferevent.h | 13 +++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/bufferevent.c b/bufferevent.c index 4c9e38fa..9855d183 100644 --- a/bufferevent.c +++ b/bufferevent.c @@ -473,6 +473,24 @@ bufferevent_settimeout(struct bufferevent *bufev, } +int +bufferevent_disable_hard(struct bufferevent *bufev, short event) +{ + int r = 0; + struct bufferevent_private *bufev_private = + EVUTIL_UPCAST(bufev, struct bufferevent_private, bev); + + BEV_LOCK(bufev); + bufev->enabled &= ~event; + + bufev_private->connecting = 0; + if (bufev->be_ops->disable(bufev, event) < 0) + r = -1; + + BEV_UNLOCK(bufev); + return r; +} + int bufferevent_disable(struct bufferevent *bufev, short event) { diff --git a/http.c b/http.c index 9cda977d..2f3a4ccf 100644 --- a/http.c +++ b/http.c @@ -1156,7 +1156,7 @@ evhttp_connection_reset(struct evhttp_connection *evcon) { struct evbuffer *tmp; - bufferevent_disable(evcon->bufev, EV_READ|EV_WRITE); + bufferevent_disable_hard(evcon->bufev, EV_READ|EV_WRITE); if (evcon->fd != -1) { /* inform interested parties about connection close */ diff --git a/include/event2/bufferevent.h b/include/event2/bufferevent.h index ee84e084..70627ea6 100644 --- a/include/event2/bufferevent.h +++ b/include/event2/bufferevent.h @@ -374,6 +374,19 @@ int bufferevent_enable(struct bufferevent *bufev, short event); */ int bufferevent_disable(struct bufferevent *bufev, short event); + +/** + Disable a bufferevent. Equivalent to bufferevent_disable(), but + first resets 'connecting' flag to force EV_WRITE down for sure. + + @param bufev the bufferevent to be disabled + @param event any combination of EV_READ | EV_WRITE. + @return 0 if successful, or -1 if an error occurred + @see bufferevent_disable() + */ +int bufferevent_disable_hard(struct bufferevent *bufev, short event); + + /** Return the events that are enabled on a given bufferevent. From c8baac90231ab3dc4179b302568c640ab4ae05c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 7 Mar 2011 21:55:47 -0500 Subject: [PATCH 2/2] Followup for Tomash Brechko's http patch This patch makes bufferevent_disable_hard() non-public, and adds a comment about what it's for and why it's used. --- bufferevent-internal.h | 15 +++++++++++++++ http.c | 12 ++++++++++++ include/event2/bufferevent.h | 13 ------------- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/bufferevent-internal.h b/bufferevent-internal.h index 9d84e739..71711a36 100644 --- a/bufferevent-internal.h +++ b/bufferevent-internal.h @@ -36,6 +36,7 @@ extern "C" { #include "evthread-internal.h" #include "event2/thread.h" #include "ratelim-internal.h" +#include "event2/bufferevent_struct.h" /* These flags are reasons that we might be declining to actually enable reading or writing on a bufferevent. @@ -287,6 +288,20 @@ void bufferevent_unsuspend_write(struct bufferevent *bufev, bufferevent_suspend_ #define bufferevent_wm_unsuspend_read(b) \ bufferevent_unsuspend_read((b), BEV_SUSPEND_WM) +/* + Disable a bufferevent. Equivalent to bufferevent_disable(), but + first resets 'connecting' flag to force EV_WRITE down for sure. + + XXXX this method will go away in the future; try not to add new users. + See comment in evhttp_connection_reset() for discussion. + + @param bufev the bufferevent to be disabled + @param event any combination of EV_READ | EV_WRITE. + @return 0 if successful, or -1 if an error occurred + @see bufferevent_disable() + */ +int bufferevent_disable_hard(struct bufferevent *bufev, short event); + /** Internal: Set up locking on a bufferevent. If lock is set, use it. * Otherwise, use a new lock. */ int bufferevent_enable_locking(struct bufferevent *bufev, void *lock); diff --git a/http.c b/http.c index 2f3a4ccf..3a24df2f 100644 --- a/http.c +++ b/http.c @@ -100,6 +100,7 @@ #include "util-internal.h" #include "http-internal.h" #include "mm-internal.h" +#include "bufferevent-internal.h" #ifndef _EVENT_HAVE_GETNAMEINFO #define NI_MAXSERV 32 @@ -1156,6 +1157,17 @@ evhttp_connection_reset(struct evhttp_connection *evcon) { struct evbuffer *tmp; + /* XXXX This is not actually an optimal fix. Instead we ought to have + an API for "stop connecting", or use bufferevent_setfd to turn off + connecting. But for Libevent 2.0, this seems like a minimal change + least likely to disrupt the rest of the bufferevent and http code. + + Why is this here? If the fd is set in the bufferevent, and the + bufferevent is connecting, then you can't actually stop the + bufferevent from trying to connect with bufferevent_disable(). The + connect will never trigger, since we close the fd, but the timeout + might. That caused an assertion failure in evhttp_connection_fail. + */ bufferevent_disable_hard(evcon->bufev, EV_READ|EV_WRITE); if (evcon->fd != -1) { diff --git a/include/event2/bufferevent.h b/include/event2/bufferevent.h index 70627ea6..ee84e084 100644 --- a/include/event2/bufferevent.h +++ b/include/event2/bufferevent.h @@ -374,19 +374,6 @@ int bufferevent_enable(struct bufferevent *bufev, short event); */ int bufferevent_disable(struct bufferevent *bufev, short event); - -/** - Disable a bufferevent. Equivalent to bufferevent_disable(), but - first resets 'connecting' flag to force EV_WRITE down for sure. - - @param bufev the bufferevent to be disabled - @param event any combination of EV_READ | EV_WRITE. - @return 0 if successful, or -1 if an error occurred - @see bufferevent_disable() - */ -int bufferevent_disable_hard(struct bufferevent *bufev, short event); - - /** Return the events that are enabled on a given bufferevent.