Merge branch 'bufev-cancellations-v5'

This patch set fixes bufferevent via http request cancellations (connect() and
dns-request), it survives tests and cancel.. with --no-fork, so this must be ok
(though I have one patch for dns layer pending).
But I'm not sure about cancel.. unit tests on win32, will fix disable them
later if they will differs (plus maybe we must make them skip-by-default?).

Fixes: #333

* bufev-cancellations-v5:
  http: set fd to -1 unconditioally, to avoid leaking of DNS requests
  test/http: cover NS timed out during request cancellations separatelly
  http: avoid leaking of fd in evhttp_connection_free()
  http: get fd from be layer during connection reset
  be_sock: cancel in-progress dns requests
  evdns: export cancel via callbacks in util (like async lib core/extra issues)
  test/http: request cancellation with resolving/{conn,write}-timeouts in progress

bufferevent-internal.h

@@ -228,6 +228,8 @@ struct bufferevent_private {
 		struct sockaddr_in6 in6;
 		struct sockaddr_in in;
 	} conn_address;
+
+	struct evdns_getaddrinfo_request *dns_request;
 };
 
 /** Possible operations for a control callback. */

bufferevent_sock.c

@@ -468,6 +468,13 @@ bufferevent_connect_getaddrinfo_cb(int result, struct evutil_addrinfo *ai,
 	bufferevent_unsuspend_write_(bev, BEV_SUSPEND_LOOKUP);
 	bufferevent_unsuspend_read_(bev, BEV_SUSPEND_LOOKUP);
 
+	bev_p->dns_request = NULL;
+
+	if (result == EVUTIL_EAI_CANCEL) {
+		bev_p->dns_error = result;
+		bufferevent_decref_and_unlock_(bev);
+		return;
+	}
 	if (result != 0) {
 		bev_p->dns_error = result;
 		bufferevent_run_eventcb_(bev, BEV_EVENT_ERROR, 0);
@@ -514,8 +521,8 @@ bufferevent_socket_connect_hostname(struct bufferevent *bev,
 	bufferevent_suspend_read_(bev, BEV_SUSPEND_LOOKUP);
 
 	bufferevent_incref_(bev);
-	evutil_getaddrinfo_async_(evdns_base, hostname, portbuf,
-	    &hint, bufferevent_connect_getaddrinfo_cb, bev);
+	bev_p->dns_request = evutil_getaddrinfo_async_(evdns_base, hostname,
+	    portbuf, &hint, bufferevent_connect_getaddrinfo_cb, bev);
 	BEV_UNLOCK(bev);
 
 	return 0;
@@ -603,6 +610,8 @@ be_socket_destruct(struct bufferevent *bufev)
 
 	if ((bufev_p->options & BEV_OPT_CLOSE_ON_FREE) && fd >= 0)
 		EVUTIL_CLOSESOCKET(fd);
+
+	evutil_getaddrinfo_cancel_async_(bufev_p->dns_request);
 }
 
 static int
@@ -616,6 +625,9 @@ be_socket_flush(struct bufferevent *bev, short iotype,
 static void
 be_socket_setfd(struct bufferevent *bufev, evutil_socket_t fd)
 {
+	struct bufferevent_private *bufev_p =
+	    EVUTIL_UPCAST(bufev, struct bufferevent_private, bev);
+
 	BEV_LOCK(bufev);
 	EVUTIL_ASSERT(bufev->be_ops == &bufferevent_ops_socket);
 
@@ -633,6 +645,8 @@ be_socket_setfd(struct bufferevent *bufev, evutil_socket_t fd)
 	if (fd >= 0)
 		bufferevent_enable(bufev, bufev->enabled);
 
+	evutil_getaddrinfo_cancel_async_(bufev_p->dns_request);
+
 	BEV_UNLOCK(bufev);
 }
 

evdns.c

@@ -3908,6 +3908,7 @@ evdns_base_new(struct event_base *event_base, int flags)
 	 * functionality.  We can't just call evdns_getaddrinfo directly or
 	 * else libevent-core will depend on libevent-extras. */
 	evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo);
+	evutil_set_evdns_getaddrinfo_cancel_fn_(evdns_getaddrinfo_cancel);
 
 	base = mm_malloc(sizeof(struct evdns_base));
 	if (base == NULL)

evutil.c

@@ -1546,6 +1546,7 @@ evutil_freeaddrinfo(struct evutil_addrinfo *ai)
 }
 
 static evdns_getaddrinfo_fn evdns_getaddrinfo_impl = NULL;
+static evdns_getaddrinfo_cancel_fn evdns_getaddrinfo_cancel_impl = NULL;
 
 void
 evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo_fn fn)
@@ -1553,27 +1554,40 @@ evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo_fn fn)
 	if (!evdns_getaddrinfo_impl)
 		evdns_getaddrinfo_impl = fn;
 }
+void
+evutil_set_evdns_getaddrinfo_cancel_fn_(evdns_getaddrinfo_cancel_fn fn)
+{
+	if (!evdns_getaddrinfo_cancel_impl)
+		evdns_getaddrinfo_cancel_impl = fn;
+}
 
 /* Internal helper function: act like evdns_getaddrinfo if dns_base is set;
  * otherwise do a blocking resolve and pass the result to the callback in the
  * way that evdns_getaddrinfo would.
  */
-int
-evutil_getaddrinfo_async_(struct evdns_base *dns_base,
+struct evdns_getaddrinfo_request *evutil_getaddrinfo_async_(
+    struct evdns_base *dns_base,
     const char *nodename, const char *servname,
     const struct evutil_addrinfo *hints_in,
     void (*cb)(int, struct evutil_addrinfo *, void *), void *arg)
 {
 	if (dns_base && evdns_getaddrinfo_impl) {
-		evdns_getaddrinfo_impl(
+		return evdns_getaddrinfo_impl(
 			dns_base, nodename, servname, hints_in, cb, arg);
 	} else {
 		struct evutil_addrinfo *ai=NULL;
 		int err;
 		err = evutil_getaddrinfo(nodename, servname, hints_in, &ai);
 		cb(err, ai, arg);
+		return NULL;
+	}
+}
+
+void evutil_getaddrinfo_cancel_async_(struct evdns_getaddrinfo_request *data)
+{
+	if (evdns_getaddrinfo_cancel_impl && data) {
+		evdns_getaddrinfo_cancel_impl(data);
 	}
-	return 0;
 }
 
 const char *

http.c

@@ -1244,6 +1244,9 @@ evhttp_connection_free(struct evhttp_connection *evcon)
 	event_deferred_cb_cancel_(get_deferred_queue(evcon),
 	    &evcon->read_more_deferred_cb);
 
+	if (evcon->fd == -1)
+		evcon->fd = bufferevent_getfd(evcon->bufev);
+
 	if (evcon->fd != -1) {
 		bufferevent_disable(evcon->bufev, EV_READ|EV_WRITE);
 		shutdown(evcon->fd, EVUTIL_SHUT_WR);
@@ -1329,6 +1332,9 @@ 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)
@@ -1336,9 +1342,9 @@ evhttp_connection_reset_(struct evhttp_connection *evcon)
 
 		shutdown(evcon->fd, EVUTIL_SHUT_WR);
 		evutil_closesocket(evcon->fd);
-		bufferevent_setfd(evcon->bufev, -1);
 		evcon->fd = -1;
 	}
+	bufferevent_setfd(evcon->bufev, -1);
 
 	/* we need to clean up any buffered data */
 	tmp = bufferevent_get_output(evcon->bufev);

test/regress_http.c

@@ -1268,6 +1268,21 @@ http_request_never_call(struct evhttp_request *req, void *arg)
 	fprintf(stdout, "FAILED\n");
 	exit(1);
 }
+static void
+http_failed_request_done(struct evhttp_request *req, void *arg)
+{
+	tt_assert(!req);
+end:
+	event_base_loopexit(arg, NULL);
+}
+static void
+http_timed_out_request_done(struct evhttp_request *req, void *arg)
+{
+	tt_assert(req);
+	tt_int_op(evhttp_request_get_response_code(req), !=, HTTP_OK);
+end:
+	event_base_loopexit(arg, NULL);
+}
 
 static void
 http_request_error_cb_with_cancel(enum evhttp_request_error error, void *arg)
@@ -1293,7 +1308,59 @@ http_do_cancel(evutil_socket_t fd, short what, void *arg)
 	evhttp_cancel_request(req);
 	++test_ok;
 }
+static void
+http_no_write(struct evbuffer *buffer, const struct evbuffer_cb_info *info, void *arg)
+{
+	fprintf(stdout, "FAILED\n");
+	exit(1);
+}
+static void
+http_free_evcons(struct evhttp_connection **evcons)
+{
+	if (!evcons)
+		return;
 
+	struct evhttp_connection *evcon, **orig = evcons;
+	while ((evcon = *evcons++)) {
+		evhttp_connection_free(evcon);
+	}
+	free(orig);
+}
+/** fill the backlog to force server drop packages for timeouts */
+static struct evhttp_connection **
+http_fill_backlog(struct event_base *base, int port)
+{
+#define BACKLOG_SIZE 256
+		struct evhttp_connection **evcon = malloc(sizeof(*evcon) * (BACKLOG_SIZE + 1));
+		int i;
+
+		for (i = 0; i < BACKLOG_SIZE; ++i) {
+			struct evhttp_request *req;
+
+			evcon[i] = evhttp_connection_base_new(base, NULL, "127.0.0.1", port);
+			tt_assert(evcon[i]);
+			evhttp_connection_set_timeout(evcon[i], 5);
+
+			req = evhttp_request_new(http_request_never_call, NULL);
+			tt_assert(req);
+			tt_int_op(evhttp_make_request(evcon[i], req, EVHTTP_REQ_GET, "/delay"), !=, -1);
+		}
+		evcon[i] = NULL;
+
+		return evcon;
+ end:
+		fprintf(stderr, "Couldn't fill the backlog");
+		return NULL;
+}
+
+enum http_cancel_test_type {
+	BASIC = 1,
+	BY_HOST = 2,
+	NO_NS = 4,
+	INACTIVE_SERVER = 8,
+	SERVER_TIMEOUT = 16,
+	NS_TIMEOUT = 32,
+};
 static void
 http_cancel_test(void *arg)
 {
@@ -1301,7 +1368,36 @@ http_cancel_test(void *arg)
 	ev_uint16_t port = 0;
 	struct evhttp_connection *evcon = NULL;
 	struct evhttp_request *req = NULL;
+	struct bufferevent *bufev = NULL;
 	struct timeval tv;
+	struct evdns_base *dns_base = NULL;
+	ev_uint16_t portnum = 0;
+	char address[64];
+	struct evhttp *inactive_http = NULL;
+	struct event_base *inactive_base = NULL;
+	struct evhttp_connection **evcons = NULL;
+
+	enum http_cancel_test_type type;
+	type = (enum http_cancel_test_type)data->setup_data;
+
+	if (type & BY_HOST) {
+		tt_assert(regress_dnsserver(data->base, &portnum, search_table));
+
+		dns_base = evdns_base_new(data->base, 0/* init name servers */);
+		tt_assert(dns_base);
+
+		/** XXX: Hack the port to make timeout after resolving */
+		if (type & NO_NS)
+			++portnum;
+
+		evutil_snprintf(address, sizeof(address), "127.0.0.1:%d", portnum);
+		evdns_base_nameserver_ip_add(dns_base, address);
+
+		char *timeout = (type & NS_TIMEOUT) ? "6" : "3";
+		evdns_base_set_option(dns_base, "timeout:", timeout);
+		evdns_base_set_option(dns_base, "initial-probe-timeout:", timeout);
+		evdns_base_set_option(dns_base, "attempts:", "1");
+	}
 
 	exit_base = data->base;
 
@@ -1309,9 +1405,29 @@ http_cancel_test(void *arg)
 
 	http = http_setup(&port, data->base, 0);
 
-	evcon = evhttp_connection_base_new(data->base, NULL, "127.0.0.1", port);
+	if (type & INACTIVE_SERVER) {
+		port = 0;
+		inactive_base = event_base_new();
+		inactive_http = http_setup(&port, inactive_base, 0);
+	}
+
+	if (type & SERVER_TIMEOUT)
+		evcons = http_fill_backlog(inactive_base ?: data->base, port);
+
+	evcon = evhttp_connection_base_new(
+		data->base, dns_base,
+		type & BY_HOST ? "localhost" : "127.0.0.1",
+		port);
+	if (type & INACTIVE_SERVER)
+		evhttp_connection_set_timeout(evcon, 5);
 	tt_assert(evcon);
 
+	bufev = evhttp_connection_get_bufferevent(evcon);
+	/* Guarantee that we stack in connect() not after waiting EV_READ after
+	 * write() */
+	if (type & SERVER_TIMEOUT)
+		evbuffer_add_cb(bufferevent_get_output(bufev), http_no_write, NULL);
+
 	/*
 	 * At this point, we want to schedule a request to the HTTP
 	 * server using our make request method.
@@ -1335,12 +1451,24 @@ http_cancel_test(void *arg)
 
 	event_base_dispatch(data->base);
 
-	tt_int_op(test_ok, ==, 3);
+	if (type & NO_NS || type & INACTIVE_SERVER)
+		tt_int_op(test_ok, ==, 2); /** no servers responses */
+	else
+		tt_int_op(test_ok, ==, 3);
 
 	/* try to make another request over the same connection */
 	test_ok = 0;
 
-	req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
+	http_free_evcons(evcons);
+	if (type & SERVER_TIMEOUT)
+		evcons = http_fill_backlog(inactive_base ?: data->base, port);
+
+	if (!(type & NO_NS) && (type & SERVER_TIMEOUT))
+		req = evhttp_request_new(http_timed_out_request_done, data->base);
+	else if ((type & INACTIVE_SERVER) || (type & NO_NS))
+		req = evhttp_request_new(http_failed_request_done, data->base);
+	else
+		req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
 
 	/* Add the information that we care about */
 	evhttp_add_header(evhttp_request_get_output_headers(req), "Host", "somehost");
@@ -1354,7 +1482,16 @@ http_cancel_test(void *arg)
 	/* make another request: request empty reply */
 	test_ok = 0;
 
-	req = evhttp_request_new(http_request_empty_done, data->base);
+	http_free_evcons(evcons);
+	if (type & SERVER_TIMEOUT)
+		evcons = http_fill_backlog(inactive_base ?: data->base, port);
+
+	if (!(type & NO_NS) && (type & SERVER_TIMEOUT))
+		req = evhttp_request_new(http_timed_out_request_done, data->base);
+	else if ((type & INACTIVE_SERVER) || (type & NO_NS))
+		req = evhttp_request_new(http_failed_request_done, data->base);
+	else
+		req = evhttp_request_new(http_request_empty_done, data->base);
 
 	/* Add the information that we care about */
 	evhttp_add_header(evhttp_request_get_output_headers(req), "Empty", "itis");
@@ -1366,10 +1503,20 @@ http_cancel_test(void *arg)
 	event_base_dispatch(data->base);
 
  end:
+	http_free_evcons(evcons);
+	if (bufev)
+		evbuffer_remove_cb(bufferevent_get_output(bufev), http_no_write, NULL);
 	if (evcon)
 		evhttp_connection_free(evcon);
 	if (http)
 		evhttp_free(http);
+	if (dns_base)
+		evdns_base_free(dns_base, 0);
+	regress_clean_dnsserver();
+	if (inactive_http)
+		evhttp_free(inactive_http);
+	if (inactive_base)
+		event_base_free(inactive_base);
 }
 
 static void
@@ -3770,13 +3917,6 @@ http_large_entity_test_done(struct evhttp_request *req, void *arg)
 end:
 	event_base_loopexit(arg, NULL);
 }
-static void
-http_failed_request_done(struct evhttp_request *req, void *arg)
-{
-	tt_assert(!req);
-end:
-	event_base_loopexit(arg, NULL);
-}
 #ifndef WIN32
 static void
 http_expectation_failed_done(struct evhttp_request *req, void *arg)
@@ -3910,13 +4050,6 @@ static void http_data_length_constraints_test(void *arg)
 static void http_read_on_write_error_test(void *arg)
 { http_data_length_constraints_test_impl(arg, 1); }
 
-static void
-http_large_entity_non_lingering_test_done(struct evhttp_request *req, void *arg)
-{
-	tt_assert(!req);
-end:
-	event_base_loopexit(arg, NULL);
-}
 static void
 http_lingering_close_test_impl(void *arg, int lingering)
 {
@@ -3951,7 +4084,7 @@ http_lingering_close_test_impl(void *arg, int lingering)
 	if (lingering)
 		cb = http_large_entity_test_done;
 	else
-		cb = http_large_entity_non_lingering_test_done;
+		cb = http_failed_request_done;
 	req = evhttp_request_new(cb, data->base);
 	tt_assert(req);
 	evhttp_add_header(evhttp_request_get_output_headers(req), "Host", "somehost");
@@ -4342,8 +4475,10 @@ http_request_own_test(void *arg)
 	{ #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \
 		    http_##name##_test }
 
-#define HTTP(name) \
-	{ #name, http_##name##_test, TT_ISOLATED, &basic_setup, NULL }
+#define HTTP_CAST_ARG(a) ((void *)(a))
+#define HTTP_N(title, name, arg) \
+	{ #title, http_##name##_test, TT_ISOLATED, &basic_setup, HTTP_CAST_ARG(arg) }
+#define HTTP(name) HTTP_N(name, name, NULL)
 #define HTTPS(name) \
 	{ "https_" #name, https_##name##_test, TT_ISOLATED, &basic_setup, NULL }
 
@@ -4382,7 +4517,20 @@ struct testcase_t http_testcases[] = {
 	{ "uriencode", http_uriencode_test, 0, NULL, NULL },
 	HTTP(basic),
 	HTTP(simple),
-	HTTP(cancel),
+
+	HTTP_N(cancel, cancel, BASIC),
+	HTTP_N(cancel_by_host, cancel, BY_HOST),
+	HTTP_N(cancel_by_host_no_ns, cancel, BY_HOST | NO_NS),
+	HTTP_N(cancel_by_host_inactive_server, cancel, BY_HOST | INACTIVE_SERVER),
+	HTTP_N(cancel_inactive_server, cancel, INACTIVE_SERVER),
+	HTTP_N(cancel_by_host_no_ns_inactive_server, cancel, BY_HOST | NO_NS | INACTIVE_SERVER),
+	HTTP_N(cancel_by_host_server_timeout, cancel, BY_HOST | INACTIVE_SERVER | SERVER_TIMEOUT),
+	HTTP_N(cancel_server_timeout, cancel, INACTIVE_SERVER | SERVER_TIMEOUT),
+	HTTP_N(cancel_by_host_no_ns_server_timeout, cancel, BY_HOST | NO_NS | INACTIVE_SERVER | SERVER_TIMEOUT),
+	HTTP_N(cancel_by_host_ns_timeout, cancel, BY_HOST | NO_NS | NS_TIMEOUT),
+	HTTP_N(cancel_by_host_ns_timeout_inactive_server, cancel, BY_HOST | NO_NS | NS_TIMEOUT | INACTIVE_SERVER),
+	HTTP_N(cancel_by_host_ns_timeout_server_timeout, cancel, BY_HOST | NO_NS | NS_TIMEOUT | INACTIVE_SERVER | SERVER_TIMEOUT),
+
 	HTTP(virtual_host),
 	HTTP(post),
 	HTTP(put),

util-internal.h

@@ -357,8 +357,10 @@ typedef struct evdns_getaddrinfo_request* (*evdns_getaddrinfo_fn)(
     const char *nodename, const char *servname,
     const struct evutil_addrinfo *hints_in,
     void (*cb)(int, struct evutil_addrinfo *, void *), void *arg);
-
 void evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo_fn fn);
+typedef void (*evdns_getaddrinfo_cancel_fn)(
+    struct evdns_getaddrinfo_request *req);
+void evutil_set_evdns_getaddrinfo_cancel_fn_(evdns_getaddrinfo_cancel_fn fn);
 
 struct evutil_addrinfo *evutil_new_addrinfo_(struct sockaddr *sa,
     ev_socklen_t socklen, const struct evutil_addrinfo *hints);
@@ -368,10 +370,12 @@ void evutil_adjust_hints_for_addrconfig_(struct evutil_addrinfo *hints);
 int evutil_getaddrinfo_common_(const char *nodename, const char *servname,
     struct evutil_addrinfo *hints, struct evutil_addrinfo **res, int *portnum);
 
-int evutil_getaddrinfo_async_(struct evdns_base *dns_base,
+struct evdns_getaddrinfo_request *evutil_getaddrinfo_async_(
+    struct evdns_base *dns_base,
     const char *nodename, const char *servname,
     const struct evutil_addrinfo *hints_in,
     void (*cb)(int, struct evutil_addrinfo *, void *), void *arg);
+void evutil_getaddrinfo_cancel_async_(struct evdns_getaddrinfo_request *data);
 
 /** Return true iff sa is a looback address. (That is, it is 127.0.0.1/8, or
  * ::1). */