Merge pull request #12543 from vespa-engine/balder/do-not-aggregate-header-over-many-calls

Do not aggregate header info over multiple calls

2 files changed, 67 insertions, 94 deletions

diff --git a/fbench/src/httpclient/httpclient.cpp b/fbench/src/httpclient/httpclient.cpp
index 9615a6e6df7..2feed087b43 100644
--- a/fbench/src/httpclient/httpclient.cpp
+++ b/fbench/src/httpclient/httpclient.cpp
@@ -37,7 +37,6 @@ HTTPClient::HTTPClient(vespalib::CryptoEngine::SP engine, const char *hostname,
     _buf(new char[_bufsize]),
     _bufused(0),
     _bufpos(0),
-    _headerinfo(),
     _isOpen(false),
     _httpVersion(0),
     _requestStatus(0),
@@ -111,8 +110,7 @@ HTTPClient::ReadLine(char *buf, size_t bufsize)
 bool
 HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen)
 {
-    char tmp[4096];
-    char *req = NULL;
+    std::unique_ptr<char[]> req;
     uint32_t req_max = 0;
     uint32_t url_len = strlen(url);
     uint32_t host_len = _hostname.size();
@@ -127,14 +125,8 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen
         headers += "X-Yahoo-Vespa-Benchmarkdata-Coverage: true\r\n";
     }
 
-    if (url_len + host_len + headers.length() + FIXED_REQ_MAX < sizeof(tmp)) {
-        req = tmp;
-        req_max = sizeof(tmp);
-    } else {
-        req_max = url_len + host_len + headers.length() + FIXED_REQ_MAX;
-        req = new char[req_max];
-        assert(req != NULL);
-    }
+    req_max = url_len + host_len + headers.length() + FIXED_REQ_MAX;
+    req = std::make_unique<char []>(req_max);
 
     if (!_keepAlive) {
         headers += "Connection: close\r\n";
@@ -143,7 +135,7 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen
 
     // create request
     if (usePost) {
-        snprintf(req, req_max,
+        snprintf(req.get(), req_max,
                  "POST %s HTTP/1.1\r\n"
                  "Host: %s\r\n"
                  "Content-Length: %d\r\n"
@@ -151,7 +143,7 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen
                  "\r\n",
                  url, _host_header_value.c_str(), cLen, headers.c_str());
     } else {
-        snprintf(req, req_max,
+        snprintf(req.get(), req_max,
                  "GET %s HTTP/1.1\r\n"
                  "Host: %s\r\n"
                  "%s"
@@ -160,18 +152,16 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen
     }
 
     // try to reuse connection if keep-alive is enabled
+    ssize_t reqLen = strlen(req.get());
     if (_keepAlive
         && _socket
-        && _socket->write(req, strlen(req)) == (ssize_t)strlen(req)
+        && _socket->write(req.get(), reqLen) == reqLen
         && (!usePost || _socket->write(content, cLen) == (ssize_t)cLen)
         && FillBuffer() > 0) {
 
         // DEBUG
         // printf("Socket Connection reused!\n");
         _reuseCount++;
-        if (req != tmp) {
-            delete [] req;
-        }
         return true;
     } else {
         _socket.reset();
@@ -180,25 +170,14 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen
 
     // try to open new connection to server
     if (connect_socket()
-        && _socket->write(req, strlen(req)) == (ssize_t)strlen(req)
+        && _socket->write(req.get(), reqLen) == reqLen
         && (!usePost || _socket->write(content, cLen) == (ssize_t)cLen))
     {
-
-        // DEBUG
-        // printf("New Socket connection!\n");
-        if (req != tmp) {
-            delete [] req;
-        }
         return true;
     } else {
         _socket.reset();
     }
 
-    // DEBUG
-    // printf("Connect FAILED!\n");
-    if (req != tmp) {
-        delete [] req;
-    }
     return false;
 }
 
@@ -215,11 +194,11 @@ HTTPClient::SplitString(char *input, int &argc, char **argv, int maxargs)
         }
     if (*(argv[argc]) != '\0')
         argc++;
-    return NULL;                    // COMPLETE
+    return nullptr;                    // COMPLETE
 }
 
 bool
-HTTPClient::ReadHTTPHeader()
+HTTPClient::ReadHTTPHeader(std::string & headerinfo)
 {
     int     lineLen;
     char    line[4096];
@@ -242,8 +221,7 @@ HTTPClient::ReadHTTPHeader()
     if (argc >= 2) {
         if (strncmp(argv[0], "HTTP/", 5) != 0)
             return false;
-        _httpVersion = (strncmp(argv[0], "HTTP/1.0", 8) == 0) ?
-                       0 : 1;
+        _httpVersion = (strncmp(argv[0], "HTTP/1.0", 8) == 0) ? 0 : 1;
         _requestStatus = atoi(argv[1]);
     } else {
         return false;
@@ -268,8 +246,8 @@ HTTPClient::ReadHTTPHeader()
             }
 
             // Make sure to have enough memory in _headerinfo
-            _headerinfo += benchmark_data;
-            _headerinfo += "\n";
+            headerinfo += benchmark_data;
+            headerinfo += "\n";
         }
 
         SplitString(line, argc, argv, 32);
@@ -320,7 +298,7 @@ HTTPClient::ReadChunkHeader()
     char c;
     int  i;
 
-    if (_chunkSeq++ > 0 && ReadLine(NULL, 0) != 0)
+    if (_chunkSeq++ > 0 && ReadLine(nullptr, 0) != 0)
         return false;                  // no CRLF(/LF) after data block
 
     assert(_chunkLeft == 0);
@@ -345,7 +323,7 @@ HTTPClient::ReadChunkHeader()
     // printf("CHUNK: Length: %d\n", _chunkLeft);
 
     if (_chunkLeft == 0) {
-        while ((lineLen = ReadLine(NULL, 0)) > 0);   // skip trailer
+        while ((lineLen = ReadLine(nullptr, 0)) > 0);   // skip trailer
         if (lineLen < 0)
             return false;                              // data error
         _dataDone = true;                            // got last chunk
@@ -354,7 +332,7 @@ HTTPClient::ReadChunkHeader()
 }
 
 bool
-HTTPClient::Open(const char *url, bool usePost, const char *content, int cLen)
+HTTPClient::Open(std::string & headerinfo, const char *url, bool usePost, const char *content, int cLen)
 {
     if (_isOpen)
         Close();
@@ -363,7 +341,7 @@ HTTPClient::Open(const char *url, bool usePost, const char *content, int cLen)
     _dataRead  = 0;
     _dataDone  = false;
     _isOpen    = Connect(url, usePost, content, cLen);
-    if(!_isOpen || !ReadHTTPHeader()) {
+    if(!_isOpen || !ReadHTTPHeader(headerinfo)) {
         Close();
         return false;
     }
@@ -532,24 +510,24 @@ HTTPClient::Fetch(const char *url, std::ostream *file,
     ssize_t readRes  = 0;
     ssize_t written  = 0;
 
-    if (!Open(url, usePost, content, contentLen)) {
+    std::string headerinfo;
+    if (!Open(headerinfo, url, usePost, content, contentLen)) {
         return FetchStatus(false, _requestStatus, _totalHitCount, 0);
     }
 
     // Write headerinfo
     if (file) {
-        file->write(_headerinfo.c_str(), _headerinfo.length());
+        file->write(headerinfo.c_str(), headerinfo.length());
         if (file->fail()) {
             Close();
             return FetchStatus(false, _requestStatus, _totalHitCount, 0);
         }
         file->write("\r\n", 2);
         // Reset header data.
-        _headerinfo = "";
     }
 
     while((readRes = Read(buf, buflen)) > 0) {
-        if(file != NULL) {
+        if(file != nullptr) {
             if (!file->write(buf, readRes)) {
                 Close();
                 return FetchStatus(false, _requestStatus, _totalHitCount, written);
diff --git a/fbench/src/httpclient/httpclient.h b/fbench/src/httpclient/httpclient.h
index cad01826db7..5b96daa2441 100644
--- a/fbench/src/httpclient/httpclient.h
+++ b/fbench/src/httpclient/httpclient.h
@@ -95,23 +95,20 @@ protected:
     vespalib::SocketAddress        _address;
     vespalib::SyncCryptoSocket::UP _socket;
 
-  std::string      _hostname;
-  int              _port;
-  bool             _keepAlive;
-  bool             _headerBenchmarkdataCoverage;
-  std::string      _extraHeaders;
+  const std::string    _hostname;
+  int                  _port;
+  bool                 _keepAlive;
+  bool                 _headerBenchmarkdataCoverage;
+  const std::string    _extraHeaders;
   vespalib::SocketSpec _sni_spec;
-  std::string      _host_header_value;
-  uint64_t         _reuseCount;
+  std::string          _host_header_value;
+  uint64_t             _reuseCount;
 
   size_t           _bufsize;
   char            *_buf;
   ssize_t          _bufused;
   ssize_t          _bufpos;
 
-  std::string      _headerinfo;
-  unsigned int     _headerinfoPos;
-
   bool             _isOpen;
   unsigned int     _httpVersion;
   unsigned int     _requestStatus;
@@ -206,15 +203,14 @@ protected:
    * @param argv the argument array.
    * @param maxargs the size of 'argv'.
    **/
-  static char *SplitString(char *input, int &argc, char **argv,
-				       int maxargs);
+  static char *SplitString(char *input, int &argc, char **argv, int maxargs);
 
   /**
    * Read and parse the HTTP Header.
    *
    * @return success(true)/failure(fail)
    **/
-  bool ReadHTTPHeader();
+  bool ReadHTTPHeader(std::string & headerinfo);
 
   /**
    * Read and parse a chunk header. Only used with chunked encoding.
@@ -223,6 +219,43 @@ protected:
    **/
   bool ReadChunkHeader();
 
+  /**
+   * Connect to the given url and read the response HTTP header. Note
+   * that this method will fail if the host returns a status code
+   * other than 200. This is done in order to make the interface as
+   * simple as possible.
+   *
+   * @return success(true)/failure(false)
+   * @param url the url you want to connect to
+   * @param usePost whether to use POST in the request
+   * @param content if usePost is true, the content to post
+   * @param cLen length of content in bytes
+   **/
+  bool Open(std::string & headerinfo, const char *url, bool usePost = false, const char *content = 0, int cLen = 0);
+  
+  /**
+   * Close the connection to the url we are currently reading
+   * from. Will also close the physical connection if keepAlive is not
+   * enabled or if all the url content was not read. This is done
+   * because skipping will probably be more expencive than creating a
+   * new connection.
+   *
+   * @return success(true)/failure(false)
+   **/
+  bool Close();
+
+  /**
+   * Read data from the url we are currently connected to. This method
+   * should be called repeatedly until it returns 0 in order to
+   * completely read the URL content. If @ref Close is called before
+   * all URL content is read the physical connection will be closed
+   * even if keepAlive is enabled.
+   *
+   * @return bytes read or -1 on failure.
+   * @param buf where to store the incoming data.
+   * @param len length of buf.
+   **/
+  ssize_t Read(void *buf, size_t len);
 public:
 
   /**
@@ -255,44 +288,6 @@ public:
   }
 
   /**
-   * Connect to the given url and read the response HTTP header. Note
-   * that this method will fail if the host returns a status code
-   * other than 200. This is done in order to make the interface as
-   * simple as possible.
-   *
-   * @return success(true)/failure(false)
-   * @param url the url you want to connect to
-   * @param usePost whether to use POST in the request
-   * @param content if usePost is true, the content to post
-   * @param cLen length of content in bytes
-   **/
-  bool Open(const char *url, bool usePost = false, const char *content = 0, int cLen = 0);
-
-  /**
-   * Read data from the url we are currently connected to. This method
-   * should be called repeatedly until it returns 0 in order to
-   * completely read the URL content. If @ref Close is called before
-   * all URL content is read the physical connection will be closed
-   * even if keepAlive is enabled.
-   *
-   * @return bytes read or -1 on failure.
-   * @param buf where to store the incoming data.
-   * @param len length of buf.
-   **/
-  ssize_t Read(void *buf, size_t len);
-
-  /**
-   * Close the connection to the url we are currently reading
-   * from. Will also close the physical connection if keepAlive is not
-   * enabled or if all the url content was not read. This is done
-   * because skipping will probably be more expencive than creating a
-   * new connection.
-   *
-   * @return success(true)/failure(false)
-   **/
-  bool Close();
-
-  /**
    * Class that provides status about the executed fetch method.
   **/
   class FetchStatus final