refactor dispatch cancellation

Renamed some functions for clarity and readability:

- dns_dispatch_addresponse() -> dns_dispatch_add()
- dns_dispatch_removeresponse() -> dns_dispatch_done()

The dns_dispatch_cancel() function now calls dns_dispatch_done()
directly, so it is no longer ever necessary to call both functions.

dns_dispatch_cancel() is used to terminate dispatch connections
that are still pending, while dns_dispatch_done() is used when they
are complete.

lib/dns/dispatch.c

@@ -1288,11 +1288,11 @@ dns_dispatch_detach(dns_dispatch_t **dispp) {
 }
 
 isc_result_t
-dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options,
+dns_dispatch_add(dns_dispatch_t *disp, unsigned int options,
 		 unsigned int timeout, const isc_sockaddr_t *dest,
 		 dispatch_cb_t connected, dispatch_cb_t sent,
-			 dispatch_cb_t response, void *arg,
+		 dispatch_cb_t response, void *arg, dns_messageid_t *idp,
-			 dns_messageid_t *idp, dns_dispentry_t **resp) {
+		 dns_dispentry_t **resp) {
 	dns_dispentry_t *res = NULL;
 	dns_qid_t *qid = NULL;
 	in_port_t localport = 0;
@@ -1476,7 +1476,65 @@ dns_dispatch_getnext(dns_dispentry_t *resp) {
 }
 
 void
-dns_dispatch_removeresponse(dns_dispentry_t **respp) {
+dns_dispatch_cancel(dns_dispentry_t **respp) {
+	dns_dispentry_t *resp = NULL;
+
+	REQUIRE(respp != NULL);
+
+	resp = *respp;
+
+	REQUIRE(VALID_RESPONSE(resp));
+
+	resp->canceled = true;
+
+	/* Connected UDP. */
+	if (resp->handle != NULL) {
+		isc_nm_cancelread(resp->handle);
+		goto done;
+	}
+
+	/* TCP pending connection. */
+	if (ISC_LINK_LINKED(resp, plink)) {
+		dns_dispentry_t *copy = resp;
+
+		ISC_LIST_UNLINK(resp->disp->pending, resp, plink);
+		if (resp->connected != NULL) {
+			resp->connected(ISC_R_CANCELED, NULL, resp->arg);
+		}
+
+		/*
+		 * We need to detach twice if we were pending
+		 * connection - once to take the place of the
+		 * detach in tcp_connected() or udp_connected()
+		 * that we won't reach, and again later in
+		 * dns_dispatch_done().
+		 */
+		dispentry_detach(&copy);
+		goto done;
+	}
+
+	/*
+	 * Connected TCP, or unconnected UDP.
+	 *
+	 * If TCP, we don't want to cancel the dispatch
+	 * unless this is the last resp waiting.
+	 */
+	if (ISC_LINK_LINKED(resp, alink)) {
+		ISC_LIST_UNLINK(resp->disp->active, resp, alink);
+		if (ISC_LIST_EMPTY(resp->disp->active) &&
+		    resp->disp->handle != NULL) {
+			isc_nm_cancelread(resp->disp->handle);
+		} else if (resp->response != NULL) {
+			resp->response(ISC_R_CANCELED, NULL, resp->arg);
+		}
+	}
+
+done:
+	dns_dispatch_done(respp);
+}
+
+void
+dns_dispatch_done(dns_dispentry_t **respp) {
 	dns_dispatchmgr_t *mgr = NULL;
 	dns_dispatch_t *disp = NULL;
 	dns_dispentry_t *resp = NULL;
@@ -1762,45 +1820,6 @@ dns_dispatch_send(dns_dispentry_t *resp, isc_region_t *r, isc_dscp_t dscp) {
 	isc_nm_send(handle, r, send_done, resp);
 }
 
-void
-dns_dispatch_cancel(dns_dispentry_t *resp) {
-	REQUIRE(VALID_RESPONSE(resp));
-
-	resp->canceled = true;
-
-	/* Connected UDP */
-	if (resp->handle != NULL) {
-		isc_nm_cancelread(resp->handle);
-		return;
-	}
-
-	/* TCP pending connection. */
-	if (ISC_LINK_LINKED(resp, plink)) {
-		ISC_LIST_UNLINK(resp->disp->pending, resp, plink);
-		if (resp->connected != NULL) {
-			resp->connected(ISC_R_CANCELED, NULL, resp->arg);
-			dispentry_detach(&resp);
-		}
-		return;
-	}
-
-	/*
-	 * Connected TCP, or unconnected UDP.
-	 *
-	 * If TCP, we don't want to cancel the dispatch
-	 * unless this is the last resp waiting.
-	 */
-	if (ISC_LINK_LINKED(resp, alink)) {
-		ISC_LIST_UNLINK(resp->disp->active, resp, alink);
-		if (ISC_LIST_EMPTY(resp->disp->active) &&
-		    resp->disp->handle != NULL) {
-			isc_nm_cancelread(resp->disp->handle);
-		} else if (resp->response != NULL) {
-			resp->response(ISC_R_CANCELED, NULL, resp->arg);
-		}
-	}
-}
-
 isc_result_t
 dns_dispatch_getlocaladdress(dns_dispatch_t *disp, isc_sockaddr_t *addrp) {
 	REQUIRE(VALID_DISPATCH(disp));

lib/dns/include/dns/dispatch.h

@@ -25,7 +25,7 @@
  * MP:
  *
  *\li	All locking is performed internally to each dispatch.
- *	Restrictions apply to dns_dispatch_removeresponse().
+ *	Restrictions apply to dns_dispatch_done().
  *
  * Reliability:
  *
@@ -225,17 +225,7 @@ isc_result_t
 dns_dispatch_connect(dns_dispentry_t *resp);
 /*%<
  * Connect to the remote server configured in 'resp' and run the
- * connect callback that was set up via dns_dispatch_addresponse().
+ * connect callback that was set up via dns_dispatch_add().
- *
- * Requires:
- *\li	'resp' is valid.
- */
-
-void
-dns_dispatch_cancel(dns_dispentry_t *resp);
-/*%<
- * Cancel pending connects in 'resp', by setting a flag so that
- * a read is not started when the connect handler runs.
  *
  * Requires:
  *\li	'resp' is valid.
@@ -274,11 +264,11 @@ typedef void (*dispatch_cb_t)(isc_result_t eresult, isc_region_t *region,
 			      void *cbarg);
 
 isc_result_t
-dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options,
+dns_dispatch_add(dns_dispatch_t *disp, unsigned int options,
 		 unsigned int timeout, const isc_sockaddr_t *dest,
 		 dispatch_cb_t connected, dispatch_cb_t sent,
-			 dispatch_cb_t response, void *arg,
+		 dispatch_cb_t response, void *arg, dns_messageid_t *idp,
-			 dns_messageid_t *idp, dns_dispentry_t **resp);
+		 dns_dispentry_t **resp);
 /*%<
  * Add a response entry for this dispatch.
  *
@@ -316,13 +306,27 @@ dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options,
  */
 
 void
-dns_dispatch_removeresponse(dns_dispentry_t **resp);
+dns_dispatch_done(dns_dispentry_t **respp);
 /*%<
- * Stops the flow of responses for the provided id and destination.
+ * Disconnects a dispatch response entry from its dispatch and shuts it
+ * down. This is called when the dispatch is complete; use
+ * dns_dispatch_cancel() if it is still pending.
  *
  * Requires:
  *\li	"resp" != NULL and "*resp" contain a value previously allocated
- *	by dns_dispatch_addresponse();
+ *	by dns_dispatch_add();
+ */
+
+void
+dns_dispatch_cancel(dns_dispentry_t **respp);
+/*%<
+ * Cancel all pending connects and reads in a dispatch entry,
+ * then call dns_dispatch_done(). This is used when the caller
+ * cancels a dispatch response before it has completed.
+ *
+ * Requires:
+ *\li	"resp" != NULL and "*resp" contain a value previously allocated
+ *	by dns_dispatch_add();
  */
 
 isc_result_t

lib/dns/request.c

@@ -559,9 +559,9 @@ again:
 	}
 
 	req_attach(request, &rclone);
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(request->dispatch, dispopt, request->timeout,
-		request->dispatch, dispopt, request->timeout, destaddr,
+				  destaddr, req_connected, req_senddone,
-		req_connected, req_senddone, req_response, request, &id,
+				  req_response, request, &id,
 				  &request->dispentry);
 	if (result != ISC_R_SUCCESS) {
 		if ((options & DNS_REQUESTOPT_FIXEDID) != 0 && !newtcp) {
@@ -722,7 +722,7 @@ use_tcp:
 	}
 
 	req_attach(request, &rclone);
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(
 		request->dispatch, 0, request->timeout, destaddr, req_connected,
 		req_senddone, req_response, request, &id, &request->dispentry);
 	if (result != ISC_R_SUCCESS) {
@@ -737,7 +737,7 @@ use_tcp:
 		 */
 		req_detach(&rclone);
 		dns_message_renderreset(message);
-		dns_dispatch_removeresponse(&request->dispentry);
+		dns_dispatch_done(&request->dispentry);
 		dns_dispatch_detach(&request->dispatch);
 		options |= DNS_REQUESTOPT_TCP;
 		tcp = true;
@@ -901,8 +901,7 @@ request_cancel(dns_request_t *request) {
 		request->flags &= ~DNS_REQUEST_F_CONNECTING;
 
 		if (request->dispentry != NULL) {
-			dns_dispatch_cancel(request->dispentry);
+			dns_dispatch_cancel(&request->dispentry);
-			dns_dispatch_removeresponse(&request->dispentry);
 		}
 
 		dns_dispatch_detach(&request->dispatch);
@@ -977,8 +976,6 @@ dns_request_destroy(dns_request_t **requestp) {
 	LOCK(&request->requestmgr->lock);
 	LOCK(&request->requestmgr->locks[request->hash]);
 	ISC_LIST_UNLINK(request->requestmgr->requests, request, link);
-	INSIST(!DNS_REQUEST_CONNECTING(request));
-	INSIST(!DNS_REQUEST_SENDING(request));
 	UNLOCK(&request->requestmgr->locks[request->hash]);
 	UNLOCK(&request->requestmgr->lock);
 
@@ -986,7 +983,6 @@ dns_request_destroy(dns_request_t **requestp) {
 	 * These should have been cleaned up before the completion
 	 * event was sent.
 	 */
-	INSIST(!ISC_LINK_LINKED(request, link));
 	INSIST(request->dispentry == NULL);
 	INSIST(request->dispatch == NULL);
 
@@ -1010,7 +1006,7 @@ req_connected(isc_result_t eresult, isc_region_t *region, void *arg) {
 	request->flags &= ~DNS_REQUEST_F_CONNECTING;
 
 	if (eresult == ISC_R_TIMEDOUT) {
-		dns_dispatch_removeresponse(&request->dispentry);
+		dns_dispatch_done(&request->dispentry);
 		dns_dispatch_detach(&request->dispatch);
 		req_sendevent(request, eresult);
 	} else if (DNS_REQUEST_CANCELED(request)) {
@@ -1103,7 +1099,7 @@ done:
 	 * Cleanup.
 	 */
 	if (request->dispentry != NULL) {
-		dns_dispatch_removeresponse(&request->dispentry);
+		dns_dispatch_done(&request->dispentry);
 	}
 	request_cancel(request);
 
@@ -1192,7 +1188,7 @@ req_destroy(dns_request_t *request) {
 		isc_event_free((isc_event_t **)&request->event);
 	}
 	if (request->dispentry != NULL) {
-		dns_dispatch_removeresponse(&request->dispentry);
+		dns_dispatch_done(&request->dispentry);
 	}
 	if (request->dispatch != NULL) {
 		dns_dispatch_detach(&request->dispatch);

lib/dns/resolver.c

@@ -1142,7 +1142,7 @@ resquery_destroy(resquery_t *query) {
 	}
 
 	if (query->dispentry != NULL) {
-		dns_dispatch_removeresponse(&query->dispentry);
+		dns_dispatch_done(&query->dispentry);
 	}
 
 	if (query->dispatch != NULL) {
@@ -1229,12 +1229,12 @@ fctx_cancelquery(resquery_t **queryp, isc_time_t *finish, bool no_response,
 	query = *queryp;
 	fctx = query->fctx;
 
-	FCTXTRACE("cancelquery");
-
 	if (RESQUERY_CANCELED(query)) {
 		return;
 	}
 
+	FCTXTRACE("cancelquery");
+
 	query->attributes |= RESQUERY_ATTR_CANCELED;
 
 	/*
@@ -1397,8 +1397,7 @@ fctx_cancelquery(resquery_t **queryp, isc_time_t *finish, bool no_response,
 	 * exist, cancel them.
 	 */
 	if (query->dispentry != NULL) {
-		dns_dispatch_cancel(query->dispentry);
+		dns_dispatch_cancel(&query->dispentry);
-		dns_dispatch_removeresponse(&query->dispentry);
 	}
 
 	if (ISC_LINK_LINKED(query, link)) {
@@ -2126,7 +2125,7 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 	UNLOCK(&res->buckets[fctx->bucketnum].lock);
 
 	/* Set up the dispatch and set the query ID */
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(
 		query->dispatch, 0, isc_interval_ms(&fctx->interval),
 		&query->addrinfo->sockaddr, resquery_connected,
 		resquery_senddone, resquery_response, query, &query->id,
@@ -2733,7 +2732,7 @@ cleanup_message:
 	/*
 	 * Stop the dispatcher from listening.
 	 */
-	dns_dispatch_removeresponse(&query->dispentry);
+	dns_dispatch_done(&query->dispentry);
 
 cleanup_temps:
 	if (qname != NULL) {
@@ -4390,7 +4389,7 @@ fctx_doshutdown(isc_task_t *task, isc_event_t *event) {
 	fctx_cleanup(fctx);
 
 	LOCK(&res->buckets[bucketnum].lock);
-	fctx_decreference(fctx);
+	RUNTIME_CHECK(!fctx_decreference(fctx));
 
 	FCTX_ATTR_SET(fctx, FCTX_ATTR_SHUTTINGDOWN);
 

lib/dns/tests/dispatch_test.c

@@ -456,10 +456,10 @@ dispatch_timeout_tcp_connect(void **state) {
 	region.base = rbuf;
 	region.length = sizeof(rbuf);
 
-	result = dns_dispatch_addresponse(dispatch, 0, T_CLIENT_CONNECT,
+	result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
 				  &tcp_server_addr, timeout_connected,
-					  client_senddone, response, &region,
+				  client_senddone, response, &region, &id,
-					  &id, &dispentry);
+				  &dispentry);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	memset(message, 0, sizeof(message));
@@ -473,7 +473,7 @@ dispatch_timeout_tcp_connect(void **state) {
 
 	uv_sem_wait(&sem);
 
-	dns_dispatch_removeresponse(&dispentry);
+	dns_dispatch_done(&dispentry);
 
 	dns_dispatch_detach(&dispatch);
 	dns_dispatchmgr_detach(&dispatchmgr);
@@ -517,9 +517,9 @@ dispatch_timeout_tcp_response(void **state __attribute__((unused))) {
 	region.base = rbuf;
 	region.length = sizeof(rbuf);
 
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
-		dispatch, 0, T_CLIENT_CONNECT, &tcp_server_addr, connected,
+				  &tcp_server_addr, connected, client_senddone,
-		client_senddone, response_timeout, &region, &id, &dispentry);
+				  response_timeout, &region, &id, &dispentry);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	memset(message, 0, sizeof(message));
@@ -539,7 +539,7 @@ dispatch_timeout_tcp_response(void **state __attribute__((unused))) {
 	isc_nmsocket_close(&sock);
 	assert_null(sock);
 
-	dns_dispatch_removeresponse(&dispentry);
+	dns_dispatch_done(&dispentry);
 
 	dns_dispatch_detach(&dispatch);
 	dns_dispatchmgr_detach(&dispatchmgr);
@@ -573,9 +573,9 @@ dispatch_tcp_response(void **state __attribute__((unused))) {
 	region.base = rbuf;
 	region.length = sizeof(rbuf);
 
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
-		dispatch, 0, T_CLIENT_CONNECT, &tcp_server_addr, connected,
+				  &tcp_server_addr, connected, client_senddone,
-		client_senddone, response, &region, &id, &dispentry);
+				  response, &region, &id, &dispentry);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	memset(message, 0, sizeof(message));
@@ -598,7 +598,7 @@ dispatch_tcp_response(void **state __attribute__((unused))) {
 	isc_nmsocket_close(&sock);
 	assert_null(sock);
 
-	dns_dispatch_removeresponse(&dispentry);
+	dns_dispatch_done(&dispentry);
 
 	dns_dispatch_detach(&dispatch);
 	dns_dispatchmgr_detach(&dispatchmgr);
@@ -632,9 +632,9 @@ dispatch_timeout_udp_response(void **state __attribute__((unused))) {
 	region.base = rbuf;
 	region.length = sizeof(rbuf);
 
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
-		dispatch, 0, T_CLIENT_CONNECT, &udp_server_addr, connected,
+				  &udp_server_addr, connected, client_senddone,
-		client_senddone, response_timeout, &region, &id, &dispentry);
+				  response_timeout, &region, &id, &dispentry);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	memset(message, 0, sizeof(message));
@@ -654,7 +654,7 @@ dispatch_timeout_udp_response(void **state __attribute__((unused))) {
 	isc_nmsocket_close(&sock);
 	assert_null(sock);
 
-	dns_dispatch_removeresponse(&dispentry);
+	dns_dispatch_done(&dispentry);
 
 	dns_dispatch_detach(&dispatch);
 	dns_dispatchmgr_detach(&dispatchmgr);
@@ -688,9 +688,9 @@ dispatch_getnext(void **state) {
 
 	region.base = rbuf;
 	region.length = sizeof(rbuf);
-	result = dns_dispatch_addresponse(
+	result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
-		dispatch, 0, T_CLIENT_CONNECT, &udp_server_addr, connected,
+				  &udp_server_addr, connected, client_senddone,
-		client_senddone, response_getnext, &region, &id, &dispentry);
+				  response_getnext, &region, &id, &dispentry);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	memset(message, 0, sizeof(message));
@@ -711,7 +711,7 @@ dispatch_getnext(void **state) {
 	isc_nmsocket_close(&sock);
 	assert_null(sock);
 
-	dns_dispatch_removeresponse(&dispentry);
+	dns_dispatch_done(&dispentry);
 	dns_dispatch_detach(&dispatch);
 	dns_dispatchmgr_detach(&dispatchmgr);
 }