Merge branch 'reuse-conn_address-on-retry-v11'

There is regression tests and also this code worked/tested during crawling a
huge number of pages (billions).

* reuse-conn_address-on-retry-v11:
  be_sock: bufferevent_socket_set_conn_address(): assert instead of silent no-op
  http: reuse connected address only with EVHTTP_CON_REUSE_CONNECTED_ADDR
  be_sock: sanity check in bufferevent_socket_set_conn_address()
  be: replace sockaddr_storage with sockaddr_in6 for conn_address
  be: we don't need to use getpeername() we we have conn_address
  be: replace conn_address by full struct instead of pointer
  test/http: cover retrying with saved conn_address by shutting down dns server
  http: use IP address that we got before (if any) during retrying
  bufferevent: move conn_address out from http into bufferevent
  be: make @sa const for bufferevent_socket_connect()
  util: make @sa const for evutil_socket_connect_()

bufferevent-internal.h

@@ -205,6 +205,18 @@ struct bufferevent_private {
 
 	/** Rate-limiting information for this bufferevent */
 	struct bufferevent_rate_limit *rate_limiting;
+
+	/* Saved conn_addr, to extract IP address from it.
+	 *
+	 * Because some servers may reset/close connection without waiting clients,
+	 * in that case we can't extract IP address even in close_cb.
+	 * So we need to save it, just after we connected to remote server, or
+	 * after resolving (to avoid extra dns requests during retrying, since UDP
+	 * is slow) */
+	union {
+		struct sockaddr_in6 in6;
+		struct sockaddr_in in;
+	} conn_address;
 };
 
 /** Possible operations for a control callback. */
@@ -392,6 +404,9 @@ int bufferevent_generic_adj_timeouts_(struct bufferevent *bev);
 
 enum bufferevent_options bufferevent_get_options_(struct bufferevent *bev);
 
+const struct sockaddr*
+bufferevent_socket_get_conn_address_(struct bufferevent *bev);
+
 /** Internal use: We have just successfully read data into an inbuf, so
  * reset the read timeout (if any). */
 #define BEV_RESET_GENERIC_READ_TIMEOUT(bev)				\

bufferevent_sock.c

@@ -100,6 +100,31 @@ const struct bufferevent_ops bufferevent_ops_socket = {
 #define be_socket_add(ev, t)			\
 	bufferevent_add_event_((ev), (t))
 
+const struct sockaddr*
+bufferevent_socket_get_conn_address_(struct bufferevent *bev)
+{
+	struct bufferevent_private *bev_p =
+	    EVUTIL_UPCAST(bev, struct bufferevent_private, bev);
+
+	return (struct sockaddr *)&bev_p->conn_address;
+}
+static void
+bufferevent_socket_set_conn_address_fd(struct bufferevent_private *bev_p, int fd)
+{
+	socklen_t len = sizeof(bev_p->conn_address);
+
+	struct sockaddr *addr = (struct sockaddr *)&bev_p->conn_address;
+	if (addr->sa_family != AF_UNSPEC)
+		getpeername(fd, addr, &len);
+}
+static void
+bufferevent_socket_set_conn_address(struct bufferevent_private *bev_p,
+	struct sockaddr *addr, size_t addrlen)
+{
+	EVUTIL_ASSERT(addrlen <= sizeof(bev_p->conn_address));
+	memcpy(&bev_p->conn_address, addr, addrlen);
+}
+
 static void
 bufferevent_socket_outbuf_cb(struct evbuffer *buf,
     const struct evbuffer_cb_info *cbinfo,
@@ -239,6 +264,7 @@ bufferevent_writecb(evutil_socket_t fd, short event, void *arg)
 			goto done;
 		} else {
 			connected = 1;
+			bufferevent_socket_set_conn_address_fd(bufev_p, fd);
 #ifdef _WIN32
 			if (BEV_IS_ASYNC(bufev)) {
 				event_del(&bufev->ev_write);
@@ -351,7 +377,7 @@ bufferevent_socket_new(struct event_base *base, evutil_socket_t fd,
 
 int
 bufferevent_socket_connect(struct bufferevent *bev,
-    struct sockaddr *sa, int socklen)
+    const struct sockaddr *sa, int socklen)
 {
 	struct bufferevent_private *bufev_p =
 	    EVUTIL_UPCAST(bev, struct bufferevent_private, bev);
@@ -457,6 +483,7 @@ bufferevent_connect_getaddrinfo_cb(int result, struct evutil_addrinfo *ai,
 
 	/* XXX use the other addrinfos? */
 	/* XXX use this return value */
+	bufferevent_socket_set_conn_address(bev_p, ai->ai_addr, (int)ai->ai_addrlen);
 	r = bufferevent_socket_connect(bev, ai->ai_addr, (int)ai->ai_addrlen);
 	(void)r;
 	bufferevent_decref_and_unlock_(bev);

evutil.c

@@ -523,7 +523,7 @@ evutil_socket_geterror(evutil_socket_t sock)
 /* XXX we should use an enum here. */
 /* 2 for connection refused, 1 for connected, 0 for not yet, -1 for error. */
 int
-evutil_socket_connect_(evutil_socket_t *fd_ptr, struct sockaddr *sa, int socklen)
+evutil_socket_connect_(evutil_socket_t *fd_ptr, const struct sockaddr *sa, int socklen)
 {
 	int made_fd = 0;
 

http-internal.h

@@ -101,13 +101,6 @@ struct evhttp_connection {
 	struct event_base *base;
 	struct evdns_base *dns_base;
 	int ai_family;
-
-	/* Saved conn_addr, to extract IP address from it.
-	 *
-	 * Because some servers may reset/close connection without waiting clients,
-	 * in that case we can't extract IP address even in close_cb.
-	 * So we need to save it, just after we connected to remote server. */
-	struct sockaddr_storage *conn_address;
 };
 
 /* A callback for an http server */

http.c

@@ -1196,9 +1196,6 @@ evhttp_connection_free(struct evhttp_connection *evcon)
 	if (evcon->address != NULL)
 		mm_free(evcon->address);
 
-	if (evcon->conn_address != NULL)
-		mm_free(evcon->conn_address);
-
 	mm_free(evcon);
 }
 
@@ -1452,7 +1449,6 @@ evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg)
 	struct evhttp_connection *evcon = arg;
 	int error;
 	ev_socklen_t errsz = sizeof(error);
-	socklen_t conn_address_len = sizeof(*evcon->conn_address);
 
 	if (evcon->fd == -1)
 		evcon->fd = bufferevent_getfd(bufev);
@@ -1503,14 +1499,6 @@ evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg)
 	evcon->retry_cnt = 0;
 	evcon->state = EVCON_IDLE;
 
-	if (!evcon->conn_address) {
-		evcon->conn_address = mm_malloc(sizeof(*evcon->conn_address));
-	}
-	if (getpeername(evcon->fd, (struct sockaddr *)evcon->conn_address, &conn_address_len)) {
-		mm_free(evcon->conn_address);
-		evcon->conn_address = NULL;
-	}
-
 	/* reset the bufferevent cbs */
 	bufferevent_setcb(evcon->bufev,
 	    evhttp_read_cb,
@@ -2346,6 +2334,20 @@ void evhttp_connection_set_family(struct evhttp_connection *evcon,
 	evcon->ai_family = family;
 }
 
+int evhttp_connection_set_flags(struct evhttp_connection *evcon,
+	int flags)
+{
+	if (flags & ~(EVHTTP_CON_REUSE_CONNECTED_ADDR)) {
+		return 1;
+	}
+
+	evcon->flags &= ~(EVHTTP_CON_REUSE_CONNECTED_ADDR);
+
+	evcon->flags |= EVHTTP_CON_REUSE_CONNECTED_ADDR;
+
+	return 0;
+}
+
 void
 evhttp_connection_set_base(struct evhttp_connection *evcon,
     struct event_base *base)
@@ -2423,13 +2425,16 @@ evhttp_connection_get_peer(struct evhttp_connection *evcon,
 const struct sockaddr*
 evhttp_connection_get_addr(struct evhttp_connection *evcon)
 {
-	return (struct sockaddr *)evcon->conn_address;
+	return bufferevent_socket_get_conn_address_(evcon->bufev);
 }
 
 int
 evhttp_connection_connect_(struct evhttp_connection *evcon)
 {
 	int old_state = evcon->state;
+	const char *address = evcon->address;
+	const struct sockaddr *sa = evhttp_connection_get_addr(evcon);
+	int ret;
 
 	if (evcon->state == EVCON_CONNECTING)
 		return (0);
@@ -2470,8 +2475,22 @@ evhttp_connection_connect_(struct evhttp_connection *evcon)
 
 	evcon->state = EVCON_CONNECTING;
 
-	if (bufferevent_socket_connect_hostname(evcon->bufev, evcon->dns_base,
-		evcon->ai_family, evcon->address, evcon->port) < 0) {
+	if (evcon->flags & EVHTTP_CON_REUSE_CONNECTED_ADDR &&
+		sa &&
+		(sa->sa_family == AF_INET || sa->sa_family == AF_INET6)) {
+		int socklen;
+		if (sa->sa_family == AF_INET) {
+			socklen = sizeof(struct sockaddr_in);
+		} else if (sa->sa_family == AF_INET6) {
+			socklen = sizeof(struct sockaddr_in6);
+		}
+		ret = bufferevent_socket_connect(evcon->bufev, sa, socklen);
+	} else {
+		ret = bufferevent_socket_connect_hostname(evcon->bufev,
+				evcon->dns_base, evcon->ai_family, address, evcon->port);
+	}
+
+	if (ret < 0) {
 		evcon->state = old_state;
 		event_sock_warn(evcon->fd, "%s: connection to \"%s\" failed",
 		    __func__, evcon->address);

include/event2/bufferevent.h

@@ -209,7 +209,7 @@ struct bufferevent *bufferevent_socket_new(struct event_base *base, evutil_socke
    @return 0 on success, -1 on failure.
  */
 EVENT2_EXPORT_SYMBOL
-int bufferevent_socket_connect(struct bufferevent *, struct sockaddr *, int);
+int bufferevent_socket_connect(struct bufferevent *, const struct sockaddr *, int);
 
 struct evdns_base;
 /**

include/event2/http.h

@@ -636,6 +636,18 @@ struct evhttp_connection *evhttp_connection_base_new(
 void evhttp_connection_set_family(struct evhttp_connection *evcon,
 	int family);
 
+#define EVHTTP_CON_REUSE_CONNECTED_ADDR  0x0008  /* reuse connection address on retry */
+/**
+ * Set connection flags.
+ *
+ * @see EVHTTP_CON_*
+ * @return 0 on success, otherwise non zero (for example if flag doesn't
+ * supported).
+ */
+EVENT2_EXPORT_SYMBOL
+int evhttp_connection_set_flags(struct evhttp_connection *evcon,
+	int flags);
+
 /** Takes ownership of the request object
  *
  * Can be used in a request callback to keep onto the request until
@@ -728,7 +740,7 @@ void evhttp_connection_get_peer(struct evhttp_connection *evcon,
     char **address, ev_uint16_t *port);
 
 /** Get the remote address associated with this connection.
- * extracted from getpeername().
+ * extracted from getpeername() OR from nameserver.
  *
  * @return NULL if getpeername() return non success,
  * or connection is not connected,

test/regress_http.c

@@ -3326,7 +3326,7 @@ http_make_web_server(evutil_socket_t fd, short what, void *arg)
 }
 
 static void
-http_connection_retry_test(void *arg)
+http_connection_retry_test_impl(void *arg, const char *addr, struct evdns_base *dns_base)
 {
 	struct basic_test_data *data = arg;
 	ev_uint16_t port = 0;
@@ -3342,8 +3342,10 @@ http_connection_retry_test(void *arg)
 	evhttp_free(http);
 	http = NULL;
 
-	evcon = evhttp_connection_base_new(data->base, NULL, "127.0.0.1", port);
+	evcon = evhttp_connection_base_new(data->base, dns_base, addr, port);
 	tt_assert(evcon);
+	if (dns_base)
+		tt_assert(!evhttp_connection_set_flags(evcon, EVHTTP_CON_REUSE_CONNECTED_ADDR));
 
 	evhttp_connection_set_timeout(evcon, 1);
 	/* also bind to local host */
@@ -3377,6 +3379,9 @@ http_connection_retry_test(void *arg)
 	 * now test the same but with retries
 	 */
 	test_ok = 0;
+	/** Shutdown dns server, to test conn_address reusing */
+	if (dns_base)
+		regress_clean_dnsserver();
 
 	{
 		const struct timeval tv_timeout = { 0, 500000 };
@@ -3449,6 +3454,37 @@ http_connection_retry_test(void *arg)
 		evhttp_free(http);
 }
 
+static void
+http_connection_retry_conn_address_test(void *arg)
+{
+	struct basic_test_data *data = arg;
+	ev_uint16_t portnum = 0;
+	struct evdns_base *dns_base;
+	char address[64];
+
+	tt_assert(regress_dnsserver(data->base, &portnum, search_table));
+	dns_base = evdns_base_new(data->base, 0/* init name servers */);
+	tt_assert(dns_base);
+
+	/* Add ourself as the only nameserver, and make sure we really are
+	 * the only nameserver. */
+	evutil_snprintf(address, sizeof(address), "127.0.0.1:%d", portnum);
+	evdns_base_nameserver_ip_add(dns_base, address);
+
+	http_connection_retry_test_impl(arg, "localhost", dns_base);
+
+ end:
+	if (dns_base)
+		evdns_base_free(dns_base, 0);
+	/** dnsserver will be cleaned in http_connection_retry_test_impl() */
+}
+
+static void
+http_connection_retry_test(void *arg)
+{
+	return http_connection_retry_test_impl(arg, "127.0.0.1", NULL);
+}
+
 static void
 http_primitives(void *ptr)
 {
@@ -3976,6 +4012,8 @@ struct testcase_t http_testcases[] = {
 
 	HTTP(connection_fail),
 	{ "connection_retry", http_connection_retry_test, TT_ISOLATED|TT_OFF_BY_DEFAULT, &basic_setup, NULL },
+	{ "connection_retry_conn_address", http_connection_retry_conn_address_test,
+	  TT_ISOLATED|TT_OFF_BY_DEFAULT, &basic_setup, NULL },
 
 	HTTP(data_length_constraints),
 

util-internal.h

@@ -261,7 +261,7 @@ int evutil_open_closeonexec_(const char *pathname, int flags, unsigned mode);
 int evutil_read_file_(const char *filename, char **content_out, size_t *len_out,
     int is_binary);
 
-int evutil_socket_connect_(evutil_socket_t *fd_ptr, struct sockaddr *sa, int socklen);
+int evutil_socket_connect_(evutil_socket_t *fd_ptr, const struct sockaddr *sa, int socklen);
 
 int evutil_socket_finished_connecting_(evutil_socket_t fd);