modify reference counting within netmgr

- isc__nmhandle_get() now attaches to the sock in the nmhandle object.
  the caller is responsible for dereferencing the original socket
  pointer when necessary.
- tcpdns listener sockets attach sock->outer to the outer tcp listener
  socket. tcpdns connected sockets attach sock->outerhandle to the handle
  for the tcp connected socket.
- only listener sockets need to be attached/detached directly. connected
  sockets should only be accessed and reference-counted via their
  associated handles.

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 981f4aa635fe797d84344679c12425b82aa49333..d066198bd012f2c72a18545cad997e607674f960 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -405,6 +405,7 @@ struct isc_nmsocket {
        int nchildren;
        isc_nmiface_t *iface;
        isc_nmhandle_t *tcphandle;
+       isc_nmhandle_t *outerhandle;
 
        /*% Extra data allocated at the end of each isc_nmhandle_t */
        size_t extrahandlesize;
@@ -589,6 +590,9 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer,
  *
  * If 'local' is not NULL, set the handle's local address to 'local',
  * otherwise set it to 'sock->iface->addr'.
+ *
+ * 'sock' will be attached to 'handle->sock'. The caller may need
+ * to detach the socket afterward.
  */
 
 isc__nm_uvreq_t *

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index dd589788924944e49826ddb40daf83a671b5bb47..1a4a82c831a102caaeefe10b49a7d456d3606c7c 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -736,9 +736,15 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree) {
                isc__nm_decstats(sock->mgr, sock->statsindex[STATID_ACTIVE]);
        }
 
-       if (sock->tcphandle != NULL) {
-               isc_nmhandle_unref(sock->tcphandle);
-               sock->tcphandle = NULL;
+       sock->tcphandle = NULL;
+
+       if (sock->outerhandle != NULL) {
+               isc_nmhandle_unref(sock->outerhandle);
+               sock->outerhandle = NULL;
+       }
+
+       if (sock->outer != NULL) {
+               isc__nmsocket_detach(&sock->outer);
        }
 
        while ((handle = isc_astack_pop(sock->inactivehandles)) != NULL) {
@@ -1050,7 +1056,8 @@ isc__nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer,
                isc_refcount_increment0(&handle->references);
        }
 
-       handle->sock = sock;
+       isc__nmsocket_attach(sock, &handle->sock);
+
        if (peer != NULL) {
                memcpy(&handle->peer, peer, sizeof(isc_sockaddr_t));
        } else {
@@ -1160,7 +1167,7 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) {
 
 void
 isc_nmhandle_unref(isc_nmhandle_t *handle) {
-       isc_nmsocket_t *sock = NULL, *tmp = NULL;
+       isc_nmsocket_t *sock = NULL;
 
        REQUIRE(VALID_NMHANDLE(handle));
 
@@ -1182,7 +1189,6 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
         * be deleted by another thread while we're deactivating the
         * handle.
         */
-       isc__nmsocket_attach(sock, &tmp);
        nmhandle_deactivate(sock, handle);
 
        /*
@@ -1206,7 +1212,7 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
                }
        }
 
-       isc__nmsocket_detach(&tmp);
+       isc__nmsocket_detach(&sock);
 }
 
 void *

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index 5a6ac45735c30f0cd12ee83146f6ca1af709c6e7..d879fceeafb2fb6eaf7a020fe6b905726a495086 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -128,14 +128,15 @@ static void
 tcp_connect_cb(uv_connect_t *uvreq, int status) {
        isc__nm_uvreq_t *req = (isc__nm_uvreq_t *)uvreq->data;
        isc_nmsocket_t *sock = NULL;
+
        sock = uv_handle_get_data((uv_handle_t *)uvreq->handle);
 
        REQUIRE(VALID_UVREQ(req));
 
        if (status == 0) {
                isc_result_t result;
-               isc_nmhandle_t *handle = NULL;
                struct sockaddr_storage ss;
+               isc_nmhandle_t *handle = NULL;
 
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
                uv_tcp_getpeername(&sock->uv_handle.tcp, (struct sockaddr *)&ss,
@@ -146,6 +147,19 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) {
 
                handle = isc__nmhandle_get(sock, NULL, NULL);
                req->cb.connect(handle, ISC_R_SUCCESS, req->cbarg);
+
+               isc__nm_uvreq_put(&req, sock);
+
+               /*
+                * The sock is now attached to the handle.
+                */
+               isc__nmsocket_detach(&sock);
+
+               /*
+                * If the connect callback wants to hold on to the handle,
+                * it needs to attach to it.
+                */
+               isc_nmhandle_unref(handle);
        } else {
                /*
                 * TODO:
@@ -154,9 +168,8 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) {
                isc__nm_incstats(sock->mgr,
                                 sock->statsindex[STATID_CONNECTFAIL]);
                req->cb.connect(NULL, isc__nm_uverr2result(status), req->cbarg);
+               isc__nm_uvreq_put(&req, sock);
        }
-
-       isc__nm_uvreq_put(&req, sock);
 }
 
 isc_result_t
@@ -170,8 +183,8 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb,
 
        nsock = isc_mem_get(mgr->mctx, sizeof(*nsock));
        isc__nmsocket_init(nsock, mgr, isc_nm_tcplistener, iface);
-       nsock->rcb.accept = cb;
-       nsock->rcbarg = cbarg;
+       nsock->accept_cb.accept = cb;
+       nsock->accept_cbarg = cbarg;
        nsock->extrahandlesize = extrahandlesize;
        nsock->backlog = backlog;
        nsock->result = ISC_R_SUCCESS;
@@ -383,10 +396,20 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) {
 
        handle = isc__nmhandle_get(csock, NULL, &local);
 
-       INSIST(ssock->rcb.accept != NULL);
+       INSIST(ssock->accept_cb.accept != NULL);
        csock->read_timeout = ssock->mgr->init;
-       ssock->rcb.accept(handle, ISC_R_SUCCESS, ssock->rcbarg);
+       ssock->accept_cb.accept(handle, ISC_R_SUCCESS, ssock->accept_cbarg);
+
+       /*
+        * csock is now attached to the handle.
+        */
        isc__nmsocket_detach(&csock);
+
+       /*
+        * If the accept callback wants to hold on to the handle,
+        * it needs to attach to it.
+        */
+       isc_nmhandle_unref(handle);
        return;
 
 error:
@@ -915,7 +938,9 @@ timer_close_cb(uv_handle_t *uvhandle) {
 
        REQUIRE(VALID_NMSOCK(sock));
 
-       isc__nmsocket_detach(&sock->server);
+       if (sock->server != NULL) {
+               isc__nmsocket_detach(&sock->server);
+       }
        uv_close(&sock->uv_handle.handle, tcp_close_cb);
 }
 

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 0f7819fc231c7ec0887767d9177771f138d4d4e3..6ae5c111cf5725fa71216bffa4f25b1f6add9a7e 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -82,7 +82,6 @@ static void
 timer_close_cb(uv_handle_t *handle) {
        isc_nmsocket_t *sock = (isc_nmsocket_t *)uv_handle_get_data(handle);
        INSIST(VALID_NMSOCK(sock));
-       isc__nmsocket_detach(&sock);
 }
 
 static void
@@ -123,7 +122,10 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 
        dnssock->extrahandlesize = dnslistensock->extrahandlesize;
        isc__nmsocket_attach(dnslistensock, &dnssock->listener);
-       isc__nmsocket_attach(handle->sock, &dnssock->outer);
+
+       dnssock->outerhandle = handle;
+       isc_nmhandle_ref(dnssock->outerhandle);
+
        dnssock->peer = handle->sock->peer;
        dnssock->read_timeout = handle->sock->mgr->init;
        dnssock->tid = isc_nm_tid();
@@ -136,7 +138,11 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
        uv_timer_start(&dnssock->timer, dnstcp_readtimeout,
                       dnssock->read_timeout, 0);
 
-       isc_nm_read(handle, dnslisten_readcb, dnssock);
+       isc_nmhandle_ref(handle);
+       result = isc_nm_read(handle, dnslisten_readcb, dnssock);
+       if (result != ISC_R_SUCCESS) {
+               isc_nmhandle_unref(handle);
+       }
 }
 
 /*
@@ -188,6 +194,11 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) {
                                dnssock->buf_len);
                }
 
+               /*
+                * dnssock is now attached to dnshandle
+                */
+               isc__nmsocket_detach(&dnssock);
+
                *handlep = dnshandle;
                return (ISC_R_SUCCESS);
        }
@@ -244,7 +255,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
                /*
                 * We have a packet: stop timeout timers
                 */
-               atomic_store(&dnssock->outer->processing, true);
+               atomic_store(&dnssock->outerhandle->sock->processing, true);
                if (dnssock->timer_initialized) {
                        uv_timer_stop(&dnssock->timer);
                }
@@ -255,7 +266,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
                         * one packet, so we're done until the next read
                         * completes.
                         */
-                       isc_nm_pauseread(dnssock->outer);
+                       isc_nm_pauseread(dnssock->outerhandle->sock);
                        done = true;
                } else {
                        /*
@@ -267,7 +278,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
                         */
                        if (atomic_load(&dnssock->ah) >=
                            TCPDNS_CLIENTS_PER_CONN) {
-                               isc_nm_pauseread(dnssock->outer);
+                               isc_nm_pauseread(dnssock->outerhandle->sock);
                                done = true;
                        }
                }
@@ -326,7 +337,7 @@ isc__nm_tcpdns_stoplistening(isc_nmsocket_t *sock) {
        sock->rcbarg = NULL;
 
        if (sock->outer != NULL) {
-               isc_nm_stoplistening(sock->outer);
+               isc__nm_tcp_stoplistening(sock->outer);
                isc__nmsocket_detach(&sock->outer);
        }
 }
@@ -336,7 +347,8 @@ isc_nm_tcpdns_sequential(isc_nmhandle_t *handle) {
        REQUIRE(VALID_NMHANDLE(handle));
 
        if (handle->sock->type != isc_nm_tcpdnssocket ||
-           handle->sock->outer == NULL) {
+           handle->sock->outerhandle == NULL)
+       {
                return;
        }
 
@@ -348,7 +360,7 @@ isc_nm_tcpdns_sequential(isc_nmhandle_t *handle) {
         * closehandle_cb callback, called whenever a handle
         * is released.
         */
-       isc_nm_pauseread(handle->sock->outer);
+       isc_nm_pauseread(handle->sock->outerhandle->sock);
        atomic_store(&handle->sock->sequential, true);
 }
 
@@ -357,12 +369,13 @@ isc_nm_tcpdns_keepalive(isc_nmhandle_t *handle) {
        REQUIRE(VALID_NMHANDLE(handle));
 
        if (handle->sock->type != isc_nm_tcpdnssocket ||
-           handle->sock->outer == NULL) {
+           handle->sock->outerhandle == NULL)
+       {
                return;
        }
 
        atomic_store(&handle->sock->keepalive, true);
-       atomic_store(&handle->sock->outer->keepalive, true);
+       atomic_store(&handle->sock->outerhandle->sock->keepalive, true);
 }
 
 typedef struct tcpsend {
@@ -382,13 +395,13 @@ resume_processing(void *arg) {
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_nm_tid());
 
-       if (sock->type != isc_nm_tcpdnssocket || sock->outer == NULL) {
+       if (sock->type != isc_nm_tcpdnssocket || sock->outerhandle == NULL) {
                return;
        }
 
        if (atomic_load(&sock->ah) == 0) {
                /* Nothing is active; sockets can timeout now */
-               atomic_store(&sock->outer->processing, false);
+               atomic_store(&sock->outerhandle->sock->processing, false);
                if (sock->timer_initialized) {
                        uv_timer_start(&sock->timer, dnstcp_readtimeout,
                                       sock->read_timeout, 0);
@@ -404,13 +417,14 @@ resume_processing(void *arg) {
 
                result = processbuffer(sock, &handle);
                if (result == ISC_R_SUCCESS) {
-                       atomic_store(&sock->outer->processing, true);
+                       atomic_store(&sock->outerhandle->sock->processing,
+                                    true);
                        if (sock->timer_initialized) {
                                uv_timer_stop(&sock->timer);
                        }
                        isc_nmhandle_unref(handle);
-               } else if (sock->outer != NULL) {
-                       isc_nm_resumeread(sock->outer);
+               } else if (sock->outerhandle != NULL) {
+                       isc_nm_resumeread(sock->outerhandle->sock);
                }
 
                return;
@@ -428,8 +442,8 @@ resume_processing(void *arg) {
                        /*
                         * Nothing in the buffer; resume reading.
                         */
-                       if (sock->outer != NULL) {
-                               isc_nm_resumeread(sock->outer);
+                       if (sock->outerhandle != NULL) {
+                               isc_nm_resumeread(sock->outerhandle->sock);
                        }
 
                        break;
@@ -438,7 +452,7 @@ resume_processing(void *arg) {
                if (sock->timer_initialized) {
                        uv_timer_stop(&sock->timer);
                }
-               atomic_store(&sock->outer->processing, true);
+               atomic_store(&sock->outerhandle->sock->processing, true);
                isc_nmhandle_unref(dnshandle);
        } while (atomic_load(&sock->ah) < TCPDNS_CLIENTS_PER_CONN);
 }
@@ -447,13 +461,13 @@ static void
 tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
        tcpsend_t *ts = (tcpsend_t *)cbarg;
 
-       UNUSED(handle);
-
        ts->cb(ts->orighandle, result, ts->cbarg);
        isc_mem_put(ts->mctx, ts->region.base, ts->region.length);
 
        isc_nmhandle_unref(ts->orighandle);
        isc_mem_putanddetach(&ts->mctx, ts, sizeof(*ts));
+
+       isc_nmhandle_unref(handle);
 }
 
 /*
@@ -471,7 +485,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->type == isc_nm_tcpdnssocket);
 
-       if (sock->outer == NULL) {
+       if (sock->outerhandle == NULL) {
                /* The socket is closed */
                return (ISC_R_NOTCONNECTED);
        }
@@ -480,7 +494,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
        *t = (tcpsend_t){
                .cb = cb,
                .cbarg = cbarg,
-               .handle = handle->sock->outer->tcphandle,
+               .handle = handle->sock->outerhandle,
        };
 
        isc_mem_attach(sock->mgr->mctx, &t->mctx);
@@ -515,14 +529,16 @@ tcpdns_close_direct(isc_nmsocket_t *sock) {
                 * At this point we're certain that there are no external
                 * references, we can close everything.
                 */
-               if (sock->outer != NULL) {
-                       sock->outer->rcb.recv = NULL;
-                       isc__nmsocket_detach(&sock->outer);
+               if (sock->outerhandle != NULL) {
+                       sock->outerhandle->sock->rcb.recv = NULL;
+                       isc_nmhandle_unref(sock->outerhandle);
+                       sock->outerhandle = NULL;
                }
                if (sock->listener != NULL) {
                        isc__nmsocket_detach(&sock->listener);
                }
                atomic_store(&sock->closed, true);
+               isc__nmsocket_prep_destroy(sock);
        }
 }
 

diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c
index bd40fa38119261b66b2638f34ffb3825cf387326..d37c1c47653cbdb68a3cb1c98a668f6972e74ca3 100644 (file)
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -317,12 +317,17 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
        isc_result_t result;
        isc_nmhandle_t *nmhandle = NULL;
        isc_sockaddr_t sockaddr;
-       isc_nmsocket_t *sock = uv_handle_get_data((uv_handle_t *)handle);
+       isc_nmsocket_t *sock = NULL;
        isc_region_t region;
        uint32_t maxudp;
        bool free_buf = true;
 
-       REQUIRE(VALID_NMSOCK(sock));
+       /*
+        * Even though destruction of the socket can only happen from the
+        * network thread that we're in, we still attach to the socket here
+        * to ensure it won't be destroyed by the recv callback.
+        */
+       isc__nmsocket_attach(uv_handle_get_data((uv_handle_t *)handle), &sock);
 
 #ifdef UV_UDP_MMSG_CHUNK
        free_buf = ((flags & UV_UDP_MMSG_CHUNK) == 0);
@@ -338,6 +343,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
                if (free_buf) {
                        isc__nm_free_uvbuf(sock, buf);
                }
+               isc__nmsocket_detach(&sock);
                return;
        }
 
@@ -347,6 +353,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
         */
        maxudp = atomic_load(&sock->mgr->maxudp);
        if (maxudp != 0 && (uint32_t)nrecv > maxudp) {
+               isc__nmsocket_detach(&sock);
                return;
        }
 
@@ -362,6 +369,11 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
                isc__nm_free_uvbuf(sock, buf);
        }
 
+       /*
+        * The sock is now attached to the handle, we can detach our ref.
+        */
+       isc__nmsocket_detach(&sock);
+
        /*
         * If the recv callback wants to hold on to the handle,
         * it needs to attach to it.