Implement lazy buffer reallocation for HTTP server connections

Bug 4206 was caused by early buffers allocated for read I/O being
dropped while waiting for the read to actually take place.

In order to prevent future bugs in the server connection read logics we
implement the same lazy/late grow behaviour as required to fix bug 4206.

diff --git a/src/http.cc b/src/http.cc
index f5a9b3539735c5e53405cbfcf5a11f8f96bd12bf..5c3abc9a97ddf3ae2c6388280b0954ac080ba92a 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1166,6 +1166,7 @@ HttpStateData::readReply(const CommIoCbParams &io)
      * Plus, it breaks our lame *HalfClosed() detection
      */
 
+    Must(maybeMakeSpaceAvailable(true));
     CommIoCbParams rd(this); // will be expanded with ReadNow results
     rd.conn = io.conn;
     rd.size = entry->bytesWanted(Range<size_t>(0, inBuf.spaceSize()));
@@ -1533,6 +1534,28 @@ HttpStateData::maybeReadVirginBody()
     if (!Comm::IsConnOpen(serverConnection) || fd_table[serverConnection->fd].closing())
         return;
 
+    if (!maybeMakeSpaceAvailable(false))
+        return;
+
+    // XXX: get rid of the do_next_read flag
+    // check for the proper reasons preventing read(2)
+    if (!flags.do_next_read)
+        return;
+
+    flags.do_next_read = false;
+
+    // must not already be waiting for read(2) ...
+    assert(!Comm::MonitorsRead(serverConnection->fd));
+
+    // wait for read(2) to be possible.
+    typedef CommCbMemFunT<HttpStateData, CommIoCbParams> Dialer;
+    AsyncCall::Pointer call = JobCallback(11, 5, Dialer, this, HttpStateData::readReply);
+    Comm::Read(serverConnection, call);
+}
+
+bool
+HttpStateData::maybeMakeSpaceAvailable(bool doGrow)
+{
     // how much we are allowed to buffer
     const int limitBuffer = (flags.headers_parsed ? Config.readAheadGap : Config.maxReplyHeaderSize);
 
@@ -1542,7 +1565,7 @@ HttpStateData::maybeReadVirginBody()
         debugs(11, DBG_DATA, "buffer has {" << inBuf << "}");
         // Process next response from buffer
         processReply();
-        return;
+        return false;
     }
 
     // how much we want to read
@@ -1550,29 +1573,20 @@ HttpStateData::maybeReadVirginBody()
 
     if (!read_size) {
         debugs(11, 7, "wont read up to " << read_size << " into buffer (" << inBuf.length() << "/" << inBuf.spaceSize() << ") from " << serverConnection);
-        return;
+        return false;
     }
 
+    // just report whether we could grow or not, dont actually do it
+    if (doGrow)
+        return (read_size >= 2);
+
     // we may need to grow the buffer
     inBuf.reserveSpace(read_size);
     debugs(11, 8, (!flags.do_next_read ? "wont" : "may") <<
            " read up to " << read_size << " bytes info buf(" << inBuf.length() << "/" << inBuf.spaceSize() <<
            ") from " << serverConnection);
 
-    // XXX: get rid of the do_next_read flag
-    // check for the proper reasons preventing read(2)
-    if (!flags.do_next_read)
-        return;
-
-    flags.do_next_read = false;
-
-    // must not already be waiting for read(2) ...
-    assert(!Comm::MonitorsRead(serverConnection->fd));
-
-    // wait for read(2) to be possible.
-    typedef CommCbMemFunT<HttpStateData, CommIoCbParams> Dialer;
-    AsyncCall::Pointer call = JobCallback(11, 5, Dialer, this, HttpStateData::readReply);
-    Comm::Read(serverConnection, call);
+    return (inBuf.spaceSize() >= 2); // only read if there is 1+ bytes of space available
 }
 
 /// called after writing the very last request byte (body, last-chunk, etc)

diff --git a/src/http.h b/src/http.h
index b3fa6ebb613c72aa0d3b8bbe55c73b9a43118e20..53f5f60d50304f30931fa158a9a3bd9930432953 100644 (file)
--- a/src/http.h
+++ b/src/http.h
@@ -87,6 +87,17 @@ private:
     virtual void abortTransaction(const char *reason); // abnormal termination
     virtual bool mayReadVirginReplyBody() const;
 
+    /**
+     * determine if read buffer can have space made available
+     * for a read.
+     *
+     * \param grow  whether to actually expand the buffer
+     *
+     * \return whether the buffer can be grown to provide space
+     *         regardless of whether the grow actually happened.
+     */
+    bool maybeMakeSpaceAvailable(bool grow);
+
     // consuming request body
     virtual void handleMoreRequestBodyAvailable();
     virtual void handleRequestBodyProducerAborted();