DoH: Simplify http_do_bio()

This commit significantly simplifies the code flow in the
http_do_bio() function, which is responsible for processing incoming
and outgoing HTTP/2 data. It seems that the way it was structured
before was indirectly caused by the presence of the missing callback
calls bug, fixed in 8b8f4d500d9c1d41d95d34a79c8935823978114c.

The change introduced by this commit is known to remove a bottleneck
and allows reproducible and measurable performance improvement for
long runs (>= 1h) of "stress:long:rpz:doh+udp:linux:*" tests.

Additionally, it fixes a similar issue with potentially missing send
callback calls processing and hardens the code against use-after-free
errors related to the session object (they can potentially occur).

(cherry picked from commit 0956fb9b9ec4d07f1b744a829dacfe7a1251a58e)

diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c
index 33ce20809cdfea8ec55a088e3d313c850ba7ce93..3e469d4eaefef2aa943f7e8dfada85dd88664d32 100644 (file)
--- a/lib/isc/netmgr/http.c
+++ b/lib/isc/netmgr/http.c
@@ -1287,7 +1287,10 @@ http_readcb(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region,
                }
                isc_buffer_putmem(session->buf, region->base + readlen,
                                  unread_size);
-               isc_nm_pauseread(session->handle);
+               if (session->handle != NULL) {
+                       INSIST(VALID_NMHANDLE(session->handle));
+                       isc_nm_pauseread(session->handle);
+               }
                http_do_bio_async(session);
        } else {
                /* We might have something to receive or send, do IO */
@@ -1578,10 +1581,12 @@ http_too_many_active_streams(isc_nm_http_session_t *session) {
 static void
 http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle,
            isc_nm_cb_t send_cb, void *send_cbarg) {
+       isc__nm_uvreq_t *req = NULL;
+       size_t remaining = 0;
        REQUIRE(VALID_HTTP2_SESSION(session));
 
        if (session->closed) {
-               return;
+               goto cancel;
        } else if (session->closing) {
                /*
                 * There might be leftover callbacks waiting to be received
@@ -1589,23 +1594,24 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle,
                if (session->sending == 0) {
                        finish_http_session(session);
                }
-               return;
-       }
-
-       if (send_cb != NULL) {
-               INSIST(VALID_NMHANDLE(send_httphandle));
-               http_send_outgoing(session, send_httphandle, send_cb,
-                                  send_cbarg);
-               return;
+               goto cancel;
+       } else if (nghttp2_session_want_read(session->ngsession) == 0 &&
+                  nghttp2_session_want_write(session->ngsession) == 0 &&
+                  session->pending_write_data == NULL)
+       {
+               session->closing = true;
+               if (session->handle != NULL) {
+                       isc_nm_pauseread(session->handle);
+               }
+               if (session->sending == 0) {
+                       finish_http_session(session);
+               }
+               goto cancel;
        }
 
-       INSIST(send_httphandle == NULL);
-       INSIST(send_cb == NULL);
-       INSIST(send_cbarg == NULL);
-
-       if (session->pending_write_data != NULL && session->sending == 0) {
-               http_send_outgoing(session, NULL, NULL, NULL);
-               return;
+       else if (session->buf != NULL)
+       {
+               remaining = isc_buffer_remaininglength(session->buf);
        }
 
        if (nghttp2_session_want_read(session->ngsession) != 0) {
@@ -1614,9 +1620,7 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle,
                        isc__nmsocket_timer_start(session->handle->sock);
                        isc_nm_read(session->handle, http_readcb, session);
                        session->reading = true;
-               } else if (session->buf != NULL) {
-                       size_t remaining =
-                               isc_buffer_remaininglength(session->buf);
+               } else if (session->buf != NULL && remaining > 0) {
                        /* Leftover data in the buffer, use it */
                        size_t remaining_after = 0;
                        ssize_t readlen = 0;
@@ -1640,8 +1644,12 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle,
                                http_log_flooding_peer(session);
                                failed_read_cb(ISC_R_RANGE, session);
                        } else if ((size_t)readlen == remaining) {
-                               isc_buffer_free(&session->buf);
-                               http_do_bio(session, NULL, NULL, NULL);
+                               isc_buffer_clear(session->buf);
+                               isc_buffer_compact(session->buf);
+                               http_do_bio(session, send_httphandle, send_cb,
+                                           send_cbarg);
+                               isc__nm_httpsession_detach(&tmpsess);
+                               return;
                        } else if (remaining_after > 0 &&
                                   remaining_after < remaining)
                        {
@@ -1658,37 +1666,36 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle,
                                 * it and that could overwhelm the server.
                                 */
                                http_do_bio_async(session);
-                       } else {
-                               http_send_outgoing(session, NULL, NULL, NULL);
                        }
-
                        isc__nm_httpsession_detach(&tmpsess);
-                       return;
-               } else {
+               } else if (session->handle != NULL) {
+                       INSIST(VALID_NMHANDLE(session->handle));
                        /* Resume reading, it's idempotent, wait for more */
                        isc_nm_resumeread(session->handle);
                        isc__nmsocket_timer_start(session->handle->sock);
                }
-       } else {
+       } else if (session->handle != NULL) {
+               INSIST(VALID_NMHANDLE(session->handle));
                /* We don't want more data, stop reading for now */
                isc_nm_pauseread(session->handle);
        }
 
        /* we might have some data to send after processing */
-       http_send_outgoing(session, NULL, NULL, NULL);
+       http_send_outgoing(session, send_httphandle, send_cb, send_cbarg);
 
-       if (nghttp2_session_want_read(session->ngsession) == 0 &&
-           nghttp2_session_want_write(session->ngsession) == 0 &&
-           session->pending_write_data == NULL)
-       {
-               session->closing = true;
-               isc_nm_pauseread(session->handle);
-               if (session->sending == 0) {
-                       finish_http_session(session);
-               }
+       return;
+
+cancel:
+       if (send_cb == NULL) {
+               return;
        }
+       req = isc__nm_uvreq_get(send_httphandle->sock->mgr,
+                               send_httphandle->sock);
 
-       return;
+       req->cb.send = send_cb;
+       req->cbarg = send_cbarg;
+       isc_nmhandle_attach(send_httphandle, &req->handle);
+       isc__nm_sendcb(send_httphandle->sock, req, ISC_R_CANCELED, true);
 }
 
 static void