libevent/backport-http-eliminate-redundant-bev-fd-manipulating-and-cac.patch

From afa66ea4200a018bfc59abef8c2ffa11ef2b8363 Mon Sep 17 00:00:00 2001
From: Azat Khuzhin <azat@libevent.org>
Date: Wed, 4 Sep 2019 00:56:20 +0300
Subject: [PATCH] http: eliminate redundant bev fd manipulating and caching
 [WIP]

Reference:https://github.com/libevent/libevent/commit/afa66ea4200a018bfc59abef8c2ffa11ef2b8363
Conflict:NA

At the very beginning we reset the bufferevent fd (if bev has it), which
is not a good idea, since if user passes bufferevent with existing fd he
has some intention.

So we need to:
- use BEV_OPT_CLOSE_ON_FREE for default bufferevent_socket_new() (to
  avoid manual shutdown/closee)
- drop getsockopt(SOL_SOCKET, SO_ERROR), since bufferevent already has
  evutil_socket_finished_connecting_()
- drop supperior bufferevent_setfd(bev, -1) in
  evhttp_connection_connect_()

Closes: #795
Refs: #875
---
 http-internal.h       |   3 +-
 http.c                | 146 +++++++++++++++++-------------------------
 include/event2/http.h |   6 +-
 3 files changed, 64 insertions(+), 91 deletions(-)

diff --git a/http-internal.h b/http-internal.h
index 9f9b5ab..bf54d61 100644
--- a/http-internal.h
+++ b/http-internal.h
@@ -53,7 +53,6 @@ struct evhttp_connection {
 	 * server */
 	TAILQ_ENTRY(evhttp_connection) next;
 
-	evutil_socket_t fd;
 	struct bufferevent *bufev;
 
 	struct event retry_ev;		/* for retrying connects */
@@ -176,7 +175,7 @@ struct evhttp {
 /* XXX most of these functions could be static. */
 
 /* resets the connection; can be reused for more requests */
-void evhttp_connection_reset_(struct evhttp_connection *);
+void evhttp_connection_reset_(struct evhttp_connection *, int);
 
 /* connects if necessary */
 int evhttp_connection_connect_(struct evhttp_connection *);
diff --git a/http.c b/http.c
index 785def9..551b63b 100644
--- a/http.c
+++ b/http.c
@@ -777,7 +777,7 @@ evhttp_connection_fail_(struct evhttp_connection *evcon,
 	evhttp_request_free_(evcon, req);
 
 	/* reset the connection */
-	evhttp_connection_reset_(evcon);
+	evhttp_connection_reset_(evcon, 1);
 
 	/* We are trying the next request that was queued on us */
 	if (TAILQ_FIRST(&evcon->requests) != NULL)
@@ -837,7 +837,7 @@ evhttp_connection_done(struct evhttp_connection *evcon)
 
 		/* check if we got asked to close the connection */
 		if (need_close)
-			evhttp_connection_reset_(evcon);
+			evhttp_connection_reset_(evcon, 1);
 
 		if (TAILQ_FIRST(&evcon->requests) != NULL) {
 			/*
@@ -1171,7 +1171,7 @@ evhttp_read_cb(struct bufferevent *bufev, void *arg)
 				__func__, EV_SIZE_ARG(total_len)));
 #endif
 
-			evhttp_connection_reset_(evcon);
+			evhttp_connection_reset_(evcon, 1);
 		}
 		break;
 	case EVCON_DISCONNECTED:
@@ -1221,13 +1221,10 @@ void
 evhttp_connection_free(struct evhttp_connection *evcon)
 {
 	struct evhttp_request *req;
-	int need_close = 0;
 
 	/* notify interested parties that this connection is going down */
-	if (evcon->fd != -1) {
-		if (evhttp_connected(evcon) && evcon->closecb != NULL)
-			(*evcon->closecb)(evcon, evcon->closecb_arg);
-	}
+	if (evhttp_connected(evcon) && evcon->closecb != NULL)
+		(*evcon->closecb)(evcon, evcon->closecb_arg);
 
 	/* remove all requests that might be queued on this
 	 * connection.  for server connections, this should be empty.
@@ -1252,20 +1249,9 @@ evhttp_connection_free(struct evhttp_connection *evcon)
 	    &evcon->read_more_deferred_cb);
 
 	if (evcon->bufev != NULL) {
-		need_close =
-			!(bufferevent_get_options_(evcon->bufev) & BEV_OPT_CLOSE_ON_FREE);
-		if (evcon->fd == -1)
-			evcon->fd = bufferevent_getfd(evcon->bufev);
-
 		bufferevent_free(evcon->bufev);
 	}
 
-	if (evcon->fd != -1) {
-		shutdown(evcon->fd, EVUTIL_SHUT_WR);
-		if (need_close)
-			evutil_closesocket(evcon->fd);
-	}
-
 	if (evcon->bind_address != NULL)
 		mm_free(evcon->bind_address);
 
@@ -1324,16 +1310,19 @@ evhttp_request_dispatch(struct evhttp_connection* evcon)
 	evhttp_write_buffer(evcon, evhttp_write_connectioncb, NULL);
 }
 
-/* Reset our connection state: disables reading/writing, closes our fd (if
-* any), clears out buffers, and puts us in state DISCONNECTED. */
-void
-evhttp_connection_reset_(struct evhttp_connection *evcon)
+/** Hard-reset our connection state
+ *
+ * This will:
+ * - reset fd
+ * - clears out buffers
+ * - call closecb
+ */
+static void
+evhttp_connection_reset_hard_(struct evhttp_connection *evcon)
 {
 	struct evbuffer *tmp;
 	int err;
 
-	bufferevent_setcb(evcon->bufev, NULL, NULL, NULL, NULL);
-
 	/* 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
@@ -1347,18 +1336,11 @@ evhttp_connection_reset_(struct evhttp_connection *evcon)
 	*/
 	bufferevent_disable_hard_(evcon->bufev, EV_READ|EV_WRITE);
 
-	if (evcon->fd == -1)
-		evcon->fd = bufferevent_getfd(evcon->bufev);
-
-	if (evcon->fd != -1) {
-		/* inform interested parties about connection close */
-		if (evhttp_connected(evcon) && evcon->closecb != NULL)
-			(*evcon->closecb)(evcon, evcon->closecb_arg);
+	/* inform interested parties about connection close */
+	if (evhttp_connected(evcon) && evcon->closecb != NULL)
+		(*evcon->closecb)(evcon, evcon->closecb_arg);
 
-		shutdown(evcon->fd, EVUTIL_SHUT_WR);
-		evutil_closesocket(evcon->fd);
-		evcon->fd = -1;
-	}
+	/** FIXME: manipulating with fd is unwanted */
 	err = bufferevent_setfd(evcon->bufev, -1);
 	EVUTIL_ASSERT(!err && "setfd");
 
@@ -1369,9 +1351,26 @@ evhttp_connection_reset_(struct evhttp_connection *evcon)
 	tmp = bufferevent_get_input(evcon->bufev);
 	err = evbuffer_drain(tmp, -1);
 	EVUTIL_ASSERT(!err && "drain input");
+}
 
-	evcon->flags &= ~EVHTTP_CON_READING_ERROR;
+/** Reset our connection state
+ *
+ * This will:
+ * - disables reading/writing
+ * - puts us in DISCONNECTED state
+ *
+ * @param hard - hard reset will (@see evhttp_connection_reset_hard_())
+ */
+void
+evhttp_connection_reset_(struct evhttp_connection *evcon, int hard)
+{
+	bufferevent_setcb(evcon->bufev, NULL, NULL, NULL, NULL);
 
+	if (hard) {
+		evhttp_connection_reset_hard_(evcon);
+	}
+
+	evcon->flags &= ~EVHTTP_CON_READING_ERROR;
 	evcon->state = EVCON_DISCONNECTED;
 }
 
@@ -1403,7 +1402,7 @@ evhttp_connection_cb_cleanup(struct evhttp_connection *evcon)
 {
 	struct evcon_requestq requests;
 
-	evhttp_connection_reset_(evcon);
+	evhttp_connection_reset_(evcon, 1);
 	if (evcon->retry_max < 0 || evcon->retry_cnt < evcon->retry_max) {
 		struct timeval tv_retry = evcon->initial_retry_timeout;
 		int i;
@@ -1481,16 +1480,13 @@ evhttp_error_cb(struct bufferevent *bufev, short what, void *arg)
 	struct evhttp_connection *evcon = arg;
 	struct evhttp_request *req = TAILQ_FIRST(&evcon->requests);
 
-	if (evcon->fd == -1)
-		evcon->fd = bufferevent_getfd(bufev);
-
 	switch (evcon->state) {
 	case EVCON_CONNECTING:
 		if (what & BEV_EVENT_TIMEOUT) {
 			event_debug(("%s: connection timeout for \"%s:%d\" on "
 				EV_SOCK_FMT,
 				__func__, evcon->address, evcon->port,
-				EV_SOCK_ARG(evcon->fd)));
+				EV_SOCK_ARG(bufferevent_getfd(bufev))));
 			evhttp_connection_cb_cleanup(evcon);
 			return;
 		}
@@ -1526,7 +1522,7 @@ evhttp_error_cb(struct bufferevent *bufev, short what, void *arg)
 		 * disconnected.
 		 */
 		EVUTIL_ASSERT(evcon->state == EVCON_IDLE);
-		evhttp_connection_reset_(evcon);
+		evhttp_connection_reset_(evcon, 1);
 
 		/*
 		 * If we have no more requests that need completion
@@ -1572,11 +1568,6 @@ static void
 evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg)
 {
 	struct evhttp_connection *evcon = arg;
-	int error;
-	ev_socklen_t errsz = sizeof(error);
-
-	if (evcon->fd == -1)
-		evcon->fd = bufferevent_getfd(bufev);
 
 	if (!(what & BEV_EVENT_CONNECTED)) {
 		/* some operating systems return ECONNREFUSED immediately
@@ -1591,34 +1582,10 @@ evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg)
 		return;
 	}
 
-	if (evcon->fd == -1) {
-		event_debug(("%s: bufferevent_getfd returned -1",
-			__func__));
-		goto cleanup;
-	}
-
-	/* Check if the connection completed */
-	if (getsockopt(evcon->fd, SOL_SOCKET, SO_ERROR, (void*)&error,
-		       &errsz) == -1) {
-		event_debug(("%s: getsockopt for \"%s:%d\" on "EV_SOCK_FMT,
-			__func__, evcon->address, evcon->port,
-			EV_SOCK_ARG(evcon->fd)));
-		goto cleanup;
-	}
-
-	if (error) {
-		event_debug(("%s: connect failed for \"%s:%d\" on "
-			EV_SOCK_FMT": %s",
-			__func__, evcon->address, evcon->port,
-			EV_SOCK_ARG(evcon->fd),
-			evutil_socket_error_to_string(error)));
-		goto cleanup;
-	}
-
 	/* We are connected to the server now */
 	event_debug(("%s: connected to \"%s:%d\" on "EV_SOCK_FMT"\n",
 			__func__, evcon->address, evcon->port,
-			EV_SOCK_ARG(evcon->fd)));
+			EV_SOCK_ARG(bufferevent_getfd(bufev))));
 
 	/* Reset the retry count as we were successful in connecting */
 	evcon->retry_cnt = 0;
@@ -2280,7 +2247,7 @@ evhttp_read_firstline(struct evhttp_connection *evcon,
 	if (res == DATA_CORRUPTED || res == DATA_TOO_LONG) {
 		/* Error while reading, terminate */
 		event_debug(("%s: bad header lines on "EV_SOCK_FMT"\n",
-			__func__, EV_SOCK_ARG(evcon->fd)));
+			__func__, EV_SOCK_ARG(bufferevent_getfd(evcon->bufev)))); 
 		evhttp_connection_fail_(evcon, EVREQ_HTTP_INVALID_HEADER);
 		return;
 	} else if (res == MORE_DATA_EXPECTED) {
@@ -2297,7 +2264,7 @@ evhttp_read_header(struct evhttp_connection *evcon,
 		   struct evhttp_request *req)
 {
 	enum message_read_status res;
-	evutil_socket_t fd = evcon->fd;
+	evutil_socket_t fd = bufferevent_getfd(evcon->bufev);
 
 	res = evhttp_parse_headers_(req, bufferevent_get_input(evcon->bufev));
 	if (res == DATA_CORRUPTED || res == DATA_TOO_LONG) {
@@ -2388,7 +2355,6 @@ evhttp_connection_base_bufferevent_new(struct event_base *base, struct evdns_bas
 		goto error;
 	}
 
-	evcon->fd = -1;
 	evcon->port = port;
 
 	evcon->max_headers_size = EV_SIZE_MAX;
@@ -2403,7 +2369,7 @@ evhttp_connection_base_bufferevent_new(struct event_base *base, struct evdns_bas
 	}
 
 	if (bev == NULL) {
-		if (!(bev = bufferevent_socket_new(base, -1, 0))) {
+		if (!(bev = bufferevent_socket_new(base, -1, BEV_OPT_CLOSE_ON_FREE))) {
 			event_warn("%s: bufferevent_socket_new failed", __func__);
 			goto error;
 		}
@@ -2571,24 +2537,30 @@ evhttp_connection_connect_(struct evhttp_connection *evcon)
 	if (evcon->state == EVCON_CONNECTING)
 		return (0);
 
-	evhttp_connection_reset_(evcon);
+	/* Do not do hard reset, since this will reset the fd, but someone may
+	* change some options for it (i.e. setsockopt(), #875)
+	*
+	* However don't think that this options will be preserved for all
+	* connection lifetime, they will be reseted in the following cases:
+	* - evhttp_connection_set_local_address()
+	* - evhttp_connection_set_local_port()
+	* - evhttp_connection_set_retries()
+	* */
+	evhttp_connection_reset_(evcon, 0);
 
 	EVUTIL_ASSERT(!(evcon->flags & EVHTTP_CON_INCOMING));
 	evcon->flags |= EVHTTP_CON_OUTGOING;
 
 	if (evcon->bind_address || evcon->bind_port) {
-		evcon->fd = bind_socket(
-			evcon->bind_address, evcon->bind_port, 0 /*reuse*/);
-		if (evcon->fd == -1) {
+		int fd = bind_socket(evcon->bind_address, evcon->bind_port,
+			0 /*reuse*/);
+		if (fd == -1) {
 			event_debug(("%s: failed to bind to \"%s\"",
 				__func__, evcon->bind_address));
 			return (-1);
 		}
 
-		if (bufferevent_setfd(evcon->bufev, evcon->fd))
-			return (-1);
-	} else {
-		if (bufferevent_setfd(evcon->bufev, -1))
+		if (bufferevent_setfd(evcon->bufev, fd))
 			return (-1);
 	}
 
@@ -2625,7 +2597,7 @@ evhttp_connection_connect_(struct evhttp_connection *evcon)
 
 	if (ret < 0) {
 		evcon->state = old_state;
-		event_sock_warn(evcon->fd, "%s: connection to \"%s\" failed",
+		event_sock_warn(bufferevent_getfd(evcon->bufev), "%s: connection to \"%s\" failed",
 		    __func__, evcon->address);
 		/* some operating systems return ECONNREFUSED immediately
 		 * when connecting to a local address.  the cleanup is going
@@ -4273,8 +4245,6 @@ evhttp_get_request_connection(
 	evcon->flags |= EVHTTP_CON_INCOMING;
 	evcon->state = EVCON_READING_FIRSTLINE;
 
-	evcon->fd = fd;
-
 	if (bufferevent_setfd(evcon->bufev, fd))
 		goto err;
 	if (bufferevent_enable(evcon->bufev, EV_READ))
diff --git a/include/event2/http.h b/include/event2/http.h
index ed9acf4..c1521ac 100644
--- a/include/event2/http.h
+++ b/include/event2/http.h
@@ -739,7 +739,11 @@ void evhttp_connection_free(struct evhttp_connection *evcon);
 EVENT2_EXPORT_SYMBOL
 void evhttp_connection_free_on_completion(struct evhttp_connection *evcon);
 
-/** sets the ip address from which http connections are made */
+/** Sets the IP address from which http connections are made
+ *
+ * Note this resets internal bufferevent fd, so any options that had been
+ * installed will be flushed.
+ */
 EVENT2_EXPORT_SYMBOL
 void evhttp_connection_set_local_address(struct evhttp_connection *evcon,
     const char *address);
-- 
2.33.0