Merge remote branch 'github/20_getaddrinfo_cancel_v2'

evdns.c

@@ -139,7 +139,7 @@
 
 /* Persistent handle.  We keep this separate from 'struct request' since we
  * need some object to last for as long as an evdns_request is outstanding so
- * that it can be cancelled, whereas a search request can lead to multiple
+ * that it can be canceled, whereas a search request can lead to multiple
  * 'struct request' instances being created over its lifetime. */
 struct evdns_request {
 	struct request *current_req;
@@ -4052,11 +4052,17 @@ struct evdns_getaddrinfo_request {
 	/* If we have one request answered and one request still inflight,
 	 * then this field holds the answer from the first request... */
 	struct evutil_addrinfo *pending_result;
-	/* And this field holds the error code from the first request... */
-	int pending_error;
 	/* And this event is a timeout that will tell us to cancel the second
 	 * request if it's taking a long time. */
 	struct event timeout;
+
+	/* And this field holds the error code from the first request... */
+	int pending_error;
+	/* If this is set, the user canceled this request. */
+	unsigned user_canceled : 1;
+	/* If this is set, the user can no longer cancel this request; we're
+	 * just waiting for the free. */
+	unsigned request_done : 1;
 };
 
 /* Convert an evdns errors to the equivalent getaddrinfo error. */
@@ -4086,6 +4092,8 @@ getaddrinfo_merge_err(int e1, int e2)
 static void
 free_getaddrinfo_request(struct evdns_getaddrinfo_request *data)
 {
+	/* DO NOT CALL this if either of the requests is pending.  Only once
+	 * both callbacks have been invoked is it safe to free the request */
 	if (data->pending_result)
 		evutil_freeaddrinfo(data->pending_result);
 	if (data->cname_result)
@@ -4118,7 +4126,6 @@ evdns_getaddrinfo_timeout_cb(evutil_socket_t fd, short what, void *ptr)
 	/* Cancel any pending requests, and note which one */
 	if (data->ipv4_request.r) {
 		evdns_cancel_request(NULL, data->ipv4_request.r);
-		data->ipv4_request.r = NULL;
 		v4_timedout = 1;
 		EVDNS_LOCK(data->evdns_base);
 		++data->evdns_base->getaddrinfo_ipv4_timeouts;
@@ -4126,7 +4133,6 @@ evdns_getaddrinfo_timeout_cb(evutil_socket_t fd, short what, void *ptr)
 	}
 	if (data->ipv6_request.r) {
 		evdns_cancel_request(NULL, data->ipv6_request.r);
-		data->ipv6_request.r = NULL;
 		v6_timedout = 1;
 		EVDNS_LOCK(data->evdns_base);
 		++data->evdns_base->getaddrinfo_ipv6_timeouts;
@@ -4149,7 +4155,10 @@ evdns_getaddrinfo_timeout_cb(evutil_socket_t fd, short what, void *ptr)
 		data->user_cb(e, NULL, data->user_data);
 	}
 
-	free_getaddrinfo_request(data);
+	if (!v4_timedout && !v6_timedout) {
+		/* should be impossible? XXXX */
+		free_getaddrinfo_request(data);
+	}
 }
 
 static int
@@ -4159,6 +4168,13 @@ evdns_getaddrinfo_set_timeout(struct evdns_base *evdns_base,
 	return event_add(&data->timeout, &evdns_base->global_getaddrinfo_allow_skew);
 }
 
+static inline int
+evdns_result_is_answer(int result)
+{
+	return (result != DNS_ERR_NOTIMPL && result != DNS_ERR_REFUSED &&
+	    result != DNS_ERR_SERVERFAILED && result != DNS_ERR_CANCEL);
+}
+
 static void
 evdns_getaddrinfo_gotresolve(int result, char type, int count,
     int ttl, void *addresses, void *arg)
@@ -4176,33 +4192,39 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count,
 	int socklen, addrlen;
 	void *addrp;
 	int err;
-
+	int user_canceled;
-	if (result == DNS_ERR_CANCEL)
-		return;
 
 	EVUTIL_ASSERT(req->type == DNS_IPv4_A || req->type == DNS_IPv6_AAAA);
 	if (req->type == DNS_IPv4_A) {
 		data = EVUTIL_UPCAST(req, struct evdns_getaddrinfo_request, ipv4_request);
 		other_req = &data->ipv6_request;
-		if (result != DNS_ERR_NOTIMPL && result != DNS_ERR_REFUSED &&
-		    result != DNS_ERR_SERVERFAILED) {
-			EVDNS_LOCK(data->evdns_base);
-			++data->evdns_base->getaddrinfo_ipv4_answered;
-			EVDNS_UNLOCK(data->evdns_base);
-		}
 	} else {
 		data = EVUTIL_UPCAST(req, struct evdns_getaddrinfo_request, ipv6_request);
 		other_req = &data->ipv4_request;
-		if (result != DNS_ERR_NOTIMPL && result != DNS_ERR_REFUSED &&
-		    result != DNS_ERR_SERVERFAILED) {
-			EVDNS_LOCK(data->evdns_base);
-			++data->evdns_base->getaddrinfo_ipv6_answered;
-			EVDNS_UNLOCK(data->evdns_base);
-		}
 	}
 
+	EVDNS_LOCK(data->evdns_base);
+	if (evdns_result_is_answer(result)) {
+		if (req->type == DNS_IPv4_A)
+			++data->evdns_base->getaddrinfo_ipv4_answered;
+		else
+			++data->evdns_base->getaddrinfo_ipv6_answered;
+	}
+	user_canceled = data->user_canceled;
+	if (other_req->r == NULL)
+		data->request_done = 1;
+	EVDNS_UNLOCK(data->evdns_base);
+
 	req->r = NULL;
 
+	if (result == DNS_ERR_CANCEL && ! user_canceled) {
+		/* Internal cancel request from timeout or internal error.
+		 * we already answered the user. */
+		if (other_req->r == NULL)
+			free_getaddrinfo_request(data);
+		return;
+	}
+
 	if (result == DNS_ERR_NONE) {
 		if (count == 0)
 			err = EVUTIL_EAI_NODATA;
@@ -4223,8 +4245,11 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count,
 			return;
 		}
 
-		if (data->pending_result) {
+		if (user_canceled) {
-			/* If we have an answer waiting, ignore this error. */
+			data->user_cb(EVUTIL_EAI_CANCEL, NULL, data->user_data);
+		} else if (data->pending_result) {
+			/* If we have an answer waiting, and we weren't
+			 * canceled, ignore this error. */
 			add_cname_to_reply(data, data->pending_result);
 			data->user_cb(0, data->pending_result, data->user_data);
 			data->pending_result = NULL;
@@ -4236,6 +4261,16 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count,
 		}
 		free_getaddrinfo_request(data);
 		return;
+	} else if (user_canceled) {
+		if (other_req->r) {
+			/* The other request is still working; let it hit this
+			 * callback with EVUTIL_EAI_CANCEL callback and report
+			 * the failure. */
+			return;
+		}
+		data->user_cb(EVUTIL_EAI_CANCEL, NULL, data->user_data);
+		free_getaddrinfo_request(data);
+		return;
 	}
 
 	/* Looks like we got some answers. We should turn them into addrinfos
@@ -4270,12 +4305,12 @@ evdns_getaddrinfo_gotresolve(int result, char type, int count,
 		if (!ai) {
 			if (other_req->r) {
 				evdns_cancel_request(NULL, other_req->r);
-				other_req->r = NULL;
 			}
 			data->user_cb(EVUTIL_EAI_MEMORY, NULL, data->user_data);
 			evutil_freeaddrinfo(res);
 
-			free_getaddrinfo_request(data);
+			if (other_req->r == NULL)
+				free_getaddrinfo_request(data);
 			return;
 		}
 		res = evutil_addrinfo_append(res, ai);
@@ -4495,14 +4530,16 @@ evdns_getaddrinfo(struct evdns_base *dns_base,
 void
 evdns_getaddrinfo_cancel(struct evdns_getaddrinfo_request *data)
 {
+	EVDNS_LOCK(data->evdns_base);
+	if (data->request_done) {
+		EVDNS_UNLOCK(data->evdns_base);
+		return;
+	}
 	event_del(&data->timeout);
+	data->user_canceled = 1;
 	if (data->ipv4_request.r)
 		evdns_cancel_request(data->evdns_base, data->ipv4_request.r);
 	if (data->ipv6_request.r)
 		evdns_cancel_request(data->evdns_base, data->ipv6_request.r);
-	data->ipv4_request.r = data->ipv6_request.r = NULL;
+	EVDNS_UNLOCK(data->evdns_base);
-
-	data->user_cb(EVUTIL_EAI_CANCEL, NULL, data->user_data);
-
-	free_getaddrinfo_request(data);
 }

evutil.c

@@ -1348,7 +1348,7 @@ evutil_gai_strerror(int err)
 	 * conflict with the platform's native error codes. */
 	switch (err) {
 	case EVUTIL_EAI_CANCEL:
-		return "Request cancelled";
+		return "Request canceled";
 	case 0:
 		return "No error";
 

include/event2/dns.h

@@ -628,7 +628,7 @@ struct evdns_getaddrinfo_request *evdns_getaddrinfo(
     evdns_getaddrinfo_cb cb, void *arg);
 
 /* Cancel an in-progress evdns_getaddrinfo.  This MUST NOT be called after the
- * getaddrinfo's callback has been invoked.  The resolves will be cancelled,
+ * getaddrinfo's callback has been invoked.  The resolves will be canceled,
  * and the callback will be invoked with the error EVUTIL_EAI_CANCEL. */
 void evdns_getaddrinfo_cancel(struct evdns_getaddrinfo_request *req);
 

test/regress_dns.c

@@ -1543,6 +1543,136 @@ end:
 		evdns_base_free(dns_base, 0);
 }
 
+struct gaic_request_status {
+	int magic;
+	struct event_base *base;
+	struct evdns_base *dns_base;
+	struct evdns_getaddrinfo_request *request;
+	struct event cancel_event;
+	int canceled;
+};
+
+#define GAIC_MAGIC 0x1234abcd
+
+static int pending = 0;
+
+static void
+gaic_cancel_request_cb(evutil_socket_t fd, short what, void *arg)
+{
+	struct gaic_request_status *status = arg;
+
+	tt_assert(status->magic == GAIC_MAGIC);
+	status->canceled = 1;
+	evdns_getaddrinfo_cancel(status->request);
+	return;
+end:
+	event_base_loopexit(status->base, NULL);
+}
+
+static void
+gaic_server_cb(struct evdns_server_request *req, void *arg)
+{
+	ev_uint32_t answer = 0x7f000001;
+	tt_assert(req->nquestions);
+	evdns_server_request_add_a_reply(req, req->questions[0]->name, 1,
+	    &answer, 100);
+	evdns_server_request_respond(req, 0);
+	return;
+end:
+	evdns_server_request_respond(req, DNS_ERR_REFUSED);
+}
+
+
+static void
+gaic_getaddrinfo_cb(int result, struct evutil_addrinfo *res, void *arg)
+{
+	struct gaic_request_status *status = arg;
+	struct event_base *base = status->base;
+	tt_assert(status->magic == GAIC_MAGIC);
+
+	if (result == EVUTIL_EAI_CANCEL) {
+		tt_assert(status->canceled);
+	}
+	event_del(&status->cancel_event);
+
+	memset(status, 0xf0, sizeof(*status));
+	free(status);
+
+end:
+	if (--pending <= 0)
+		event_base_loopexit(base, NULL);
+}
+
+static void
+gaic_launch(struct event_base *base, struct evdns_base *dns_base)
+{
+	struct gaic_request_status *status = calloc(1,sizeof(*status));
+	struct timeval tv = { 0, 10000 };
+	status->magic = GAIC_MAGIC;
+	status->base = base;
+	status->dns_base = dns_base;
+	event_assign(&status->cancel_event, base, -1, 0, gaic_cancel_request_cb,
+	    status);
+	status->request = evdns_getaddrinfo(dns_base,
+	    "foobar.bazquux.example.com", "80", NULL, gaic_getaddrinfo_cb,
+	    status);
+	event_add(&status->cancel_event, &tv);
+	++pending;
+}
+
+static void
+test_getaddrinfo_async_cancel_stress(void *arg)
+{
+	struct basic_test_data *data = arg;
+	struct event_base *base = data->base;
+	struct evdns_base *dns_base = NULL;
+	struct evdns_server_port *server = NULL;
+	evutil_socket_t fd = -1;
+	struct sockaddr_in sin;
+	struct sockaddr_storage ss;
+	ev_socklen_t slen;
+	int i;
+
+	base = event_base_new();
+	dns_base = evdns_base_new(base, 0);
+
+	memset(&sin, 0, sizeof(sin));
+	sin.sin_family = AF_INET;
+	sin.sin_port = 0;
+	sin.sin_addr.s_addr = htonl(0x7f000001);
+	if ((fd = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
+		tt_abort_perror("socket");
+	}
+	evutil_make_socket_nonblocking(fd);
+	if (bind(fd, (struct sockaddr*)&sin, sizeof(sin))<0) {
+		tt_abort_perror("bind");
+	}
+	server = evdns_add_server_port_with_base(base, fd, 0, gaic_server_cb,
+	    base);
+
+	memset(&ss, 0, sizeof(ss));
+	slen = sizeof(ss);
+	if (getsockname(fd, (struct sockaddr*)&ss, &slen)<0) {
+		tt_abort_perror("getsockname");
+	}
+	evdns_base_nameserver_sockaddr_add(dns_base,
+	    (struct sockaddr*)&ss, slen, 0);
+
+	for (i = 0; i < 1000; ++i) {
+		gaic_launch(base, dns_base);
+	}
+
+	event_base_dispatch(base);
+
+end:
+	if (dns_base)
+		evdns_base_free(dns_base, 1);
+	if (server)
+		evdns_close_server_port(server);
+	if (fd >= 0)
+		evutil_closesocket(fd);
+}
+
 
 #define DNS_LEGACY(name, flags)					       \
 	{ #name, run_legacy_test_fn, flags|TT_LEGACY, &legacy_setup,   \
@@ -1565,6 +1695,8 @@ struct testcase_t dns_testcases[] = {
 
 	{ "getaddrinfo_async", test_getaddrinfo_async,
 	  TT_FORK|TT_NEED_BASE, &basic_setup, (char*)"" },
+	{ "getaddrinfo_cancel_stress", test_getaddrinfo_async_cancel_stress,
+	  TT_FORK|TT_NEED_BASE, &basic_setup, (char*)"" },
 
 	END_OF_TESTCASES
 };