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.

diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c
index 41751ed890058776109ad469df107cb81d551d30..c702656431a88b35974d0e2f0e6d9583aa2fb90a 100644 (file)
--- a/lib/dns/dispatch.c
+++ b/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,
-                        unsigned int timeout, const isc_sockaddr_t *dest,
-                        dispatch_cb_t connected, dispatch_cb_t sent,
-                        dispatch_cb_t response, void *arg,
-                        dns_messageid_t *idp, dns_dispentry_t **resp) {
+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, dns_messageid_t *idp,
+                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));

diff --git a/lib/dns/include/dns/dispatch.h b/lib/dns/include/dns/dispatch.h
index 93f676f5f94bbeed9b12242c2a21924fd46d3d7b..572fa95d19d600181c079f92dc6047bc9c824047 100644 (file)
--- a/lib/dns/include/dns/dispatch.h
+++ b/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().
- *
- * 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.
+ * connect callback that was set up via dns_dispatch_add().
  *
  * 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,
-                        unsigned int timeout, const isc_sockaddr_t *dest,
-                        dispatch_cb_t connected, dispatch_cb_t sent,
-                        dispatch_cb_t response, void *arg,
-                        dns_messageid_t *idp, dns_dispentry_t **resp);
+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, dns_messageid_t *idp,
+                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);
+/*%<
+ * 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_add();
+ */
+
+void
+dns_dispatch_cancel(dns_dispentry_t **respp);
 /*%<
- * Stops the flow of responses for the provided id and destination.
+ * 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_addresponse();
+ *     by dns_dispatch_add();
  */
 
 isc_result_t

diff --git a/lib/dns/request.c b/lib/dns/request.c
index 2085575661fcde921c586e285ad0513461b70450..0e3619d93a74215ac2dff0a9eeadef83301d8702 100644 (file)
--- a/lib/dns/request.c
+++ b/lib/dns/request.c
@@ -559,10 +559,10 @@ again:
        }
 
        req_attach(request, &rclone);
-       result = dns_dispatch_addresponse(
-               request->dispatch, dispopt, request->timeout, destaddr,
-               req_connected, req_senddone, req_response, request, &id,
-               &request->dispentry);
+       result = dns_dispatch_add(request->dispatch, dispopt, request->timeout,
+                                 destaddr, req_connected, req_senddone,
+                                 req_response, request, &id,
+                                 &request->dispentry);
        if (result != ISC_R_SUCCESS) {
                if ((options & DNS_REQUESTOPT_FIXEDID) != 0 && !newtcp) {
                        newtcp = true;
@@ -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_removeresponse(&request->dispentry);
+                       dns_dispatch_cancel(&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);

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 4ca5d8ab546c9639b995bb85b448b496b2b9958f..02c09aad34437794b9d313014e1550a57f64b3e5 100644 (file)
--- a/lib/dns/resolver.c
+++ b/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_removeresponse(&query->dispentry);
+               dns_dispatch_cancel(&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);
 

diff --git a/lib/dns/tests/dispatch_test.c b/lib/dns/tests/dispatch_test.c
index 8b64c82ce023542d1a4dbf4f5803ef8a974e0c0f..93eac9f5ae350ee1f7f15d15cd8afc78b6b35435 100644 (file)
--- a/lib/dns/tests/dispatch_test.c
+++ b/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,
-                                         &tcp_server_addr, timeout_connected,
-                                         client_senddone, response, &region,
-                                         &id, &dispentry);
+       result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
+                                 &tcp_server_addr, timeout_connected,
+                                 client_senddone, response, &region, &id,
+                                 &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(
-               dispatch, 0, T_CLIENT_CONNECT, &tcp_server_addr, connected,
-               client_senddone, response_timeout, &region, &id, &dispentry);
+       result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
+                                 &tcp_server_addr, connected, client_senddone,
+                                 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(
-               dispatch, 0, T_CLIENT_CONNECT, &tcp_server_addr, connected,
-               client_senddone, response, &region, &id, &dispentry);
+       result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
+                                 &tcp_server_addr, connected, client_senddone,
+                                 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(
-               dispatch, 0, T_CLIENT_CONNECT, &udp_server_addr, connected,
-               client_senddone, response_timeout, &region, &id, &dispentry);
+       result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
+                                 &udp_server_addr, connected, client_senddone,
+                                 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(
-               dispatch, 0, T_CLIENT_CONNECT, &udp_server_addr, connected,
-               client_senddone, response_getnext, &region, &id, &dispentry);
+       result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT,
+                                 &udp_server_addr, connected, client_senddone,
+                                 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);
 }