Fix #88 Header views on requests could be moved on a re-allocation

CMakeLists.txt

@@ -30,7 +30,7 @@ set(LIBLIFT_SOURCE_FILES
     inc/lift/Escape.hpp src/Escape.cpp
     inc/lift/EventLoop.hpp src/EventLoop.cpp
     inc/lift/Executor.hpp src/Executor.cpp
-    inc/lift/HeaderView.hpp src/HeaderView.cpp
+    inc/lift/Header.hpp src/Header.cpp
     inc/lift/Lift.hpp src/Lift.cpp
     inc/lift/LiftStatus.hpp src/LiftStatus.cpp
     inc/lift/MimeField.hpp src/MimeField.cpp

inc/lift/Executor.hpp

@@ -44,7 +44,7 @@ class Executor {
     /// The mime handle if present.
     curl_mime* m_mime_handle { nullptr };
     /// The HTTP curl request headers.
-    std::vector<curl_slist> m_curl_request_headers {};
+    curl_slist* m_curl_request_headers { nullptr };
     /// The HTTP curl resolve hosts.
     curl_slist* m_curl_resolve_hosts { nullptr };
 

inc/lift/Header.hpp

@@ -0,0 +1,65 @@
+#pragma once
+
+#include <string>
+#include <string_view>
+
+namespace lift {
+
+class Executor;
+
+class Header {
+    friend Executor;
+
+public:
+    /**
+     * Creates an owned header.
+     * @param name The name of the header.
+     * @param value The value of the header.
+     */
+    explicit Header(
+        std::string_view name,
+        std::string_view value);
+
+    /**
+     * Creates an owned header.
+     * @param header_full The full "<name>: <value>" header field.
+     */
+    explicit Header(
+        std::string header_full);
+
+    /**
+     * @return The entire header, e.g. "Connection: Keep-Alive"
+     */
+    [[nodiscard]] auto HeaderFull() const -> const std::string& { return m_header; }
+
+    /**
+     * @return The header's name.
+     */
+    [[nodiscard]] auto Name() const -> std::string_view
+    {
+        std::string_view name = m_header;
+        name = name.substr(0, m_colon_pos);
+        return name;
+    }
+
+    /**
+     * @return The header's value or empty if it doesn't have a value.
+     */
+    [[nodiscard]] auto Value() const -> std::string_view
+    {
+        std::string_view value = m_header;
+        // Remove the name from the value. We know we made this string with ": " so skip two chars.
+        value.remove_prefix(m_colon_pos + 2);
+        return value;
+    }
+
+private:
+    /// The full header data.
+    std::string m_header {};
+    std::size_t m_colon_pos { 0 };
+
+    // Executor requires a char* to pass into the curlslist.
+    [[nodiscard]] auto headerFull() -> std::string& { return m_header; }
+};
+
+} // lift

inc/lift/Request.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include "lift/Header.hpp"
 #include "lift/Http.hpp"
 #include "lift/MimeField.hpp"
 #include "lift/ResolveHost.hpp"
@@ -188,12 +189,16 @@ class Request {
         std::string_view name,
         std::string_view value) -> void;
 
-    auto Headers() const -> const std::vector<HeaderView>& { return m_request_headers_idx; }
-    auto ClearHeaders() -> void
-    {
-        m_request_headers.clear();
-        m_request_headers_idx.clear();
-    }
+    /**
+     * @return The current list of headers added to this request.  Note that if more headers are added
+     *         the Header classes Name() and Value() string_views might become invalidated.
+     */
+    auto Headers() const -> const std::vector<lift::Header>& { return m_request_headers; }
+
+    /**
+     * Clears the current set of headers for this request.
+     */
+    auto ClearHeaders() -> void { m_request_headers.clear(); }
 
     auto Data() const -> const std::string& { return m_request_data; }
     /**
@@ -240,10 +245,8 @@ class Request {
     std::optional<std::vector<std::string>> m_accept_encodings {};
     /// A set of host:port to ip addresses that will be resolved before DNS.
     std::vector<lift::ResolveHost> m_resolve_hosts {};
-    /// The request headers buffer to quickly append into without allocating memory.
-    std::string m_request_headers {};
-    /// The request headers index.  Used to generate the curl slist.
-    std::vector<HeaderView> m_request_headers_idx {};
+    /// The request headers preformatted into the curl "Header: value\0" format.
+    std::vector<lift::Header> m_request_headers {};
     /// The POST request body data, mutually exclusive with MimeField requests.
     bool m_request_data_set { false };
     std::string m_request_data {};

inc/lift/Response.hpp

@@ -1,6 +1,6 @@
 #pragma once
 
-#include "lift/HeaderView.hpp"
+#include "lift/Header.hpp"
 #include "lift/Http.hpp"
 #include "lift/LiftStatus.hpp"
 
@@ -46,7 +46,7 @@ class Response {
     /**
      * @return The HTTP response headers.
      */
-    [[nodiscard]] auto Headers() const -> const std::vector<HeaderView>& { return m_headers_idx; }
+    [[nodiscard]] auto Headers() const -> const std::vector<Header>& { return m_headers; }
 
     /**
      * @return The HTTP download payload.
@@ -72,9 +72,7 @@ class Response {
     /// The status of this HTTP request.
     lift::LiftStatus m_lift_status { lift::LiftStatus::BUILDING };
     /// The response headers.
-    std::string m_headers {};
-    /// Views into each header.
-    std::vector<HeaderView> m_headers_idx {};
+    std::vector<Header> m_headers {};
     /// The response data if any.
     std::string m_data {};
     /// The HTTP response status code.

src/EventLoop.cpp

@@ -387,8 +387,14 @@ auto EventLoop::completeRequestTimeout(
         // per loop iteration to be removed if there are multiple items with the same timesup value.
         // removeTimeout(executor);
 
+        // IMPORTANT! Copying here is required _OR_ shared ownership must be added as libcurl
+        // maintains char* type pointers into the request data structure.  There is no guarantee
+        // after moving into user land that it will stay alive long enough until curl finishes its
+        // own timeout.
+        auto copy_ptr = std::make_unique<Request>(*executor.m_request_async);
+
         on_complete_handler(
-            std::move(executor.m_request_async),
+            std::move(copy_ptr),
             std::move(executor.m_response));
     }
 

src/Executor.cpp

@@ -41,6 +41,11 @@ Executor::Executor(
 
 Executor::~Executor()
 {
+    if (m_curl_request_headers != nullptr) {
+        curl_slist_free_all(m_curl_request_headers);
+        m_curl_request_headers = nullptr;
+    }
+
     if (m_curl_resolve_hosts != nullptr) {
         curl_slist_free_all(m_curl_resolve_hosts);
         m_curl_resolve_hosts = nullptr;
@@ -194,30 +199,18 @@ auto Executor::prepare() -> void
     }
 
     // Headers
-    // Make sure we've got enough items allocated.
-    if (m_curl_request_headers.size() < m_request->m_request_headers_idx.size()) {
-        m_curl_request_headers.resize(m_request->m_request_headers_idx.size());
+    if (m_curl_request_headers != nullptr) {
+        curl_slist_free_all(m_curl_request_headers);
+        m_curl_request_headers = nullptr;
     }
 
-    curl_slist* prev = nullptr;
-    for (std::size_t i = 0; i < m_request->m_request_headers_idx.size(); ++i) {
-        const auto& header = m_request->m_request_headers_idx[i];
-
-        auto& item = m_curl_request_headers[i];
-        // WOOF! curl shouldn't edit this...
-        // TODO c++20 use span which allows for mutable string_views.
-        item.data = const_cast<char*>(header.Header().data());
-        item.next = nullptr;
-
-        if (prev != nullptr) {
-            prev->next = &item;
-        }
-
-        prev = &item;
+    for (auto& header : m_request->m_request_headers) {
+        m_curl_request_headers = curl_slist_append(
+            m_curl_request_headers, header.headerFull().data());
     }
 
-    if (!m_curl_request_headers.empty()) {
-        curl_easy_setopt(m_curl_handle, CURLOPT_HTTPHEADER, &m_curl_request_headers.front());
+    if (m_curl_request_headers != nullptr) {
+        curl_easy_setopt(m_curl_handle, CURLOPT_HTTPHEADER, m_curl_request_headers);
     } else {
         curl_easy_setopt(m_curl_handle, CURLOPT_HTTPHEADER, nullptr);
     }
@@ -368,25 +361,7 @@ auto curl_write_header(
         data_view.remove_suffix(rm_size);
     }
 
-    const auto cleaned_up_length = data_view.length();
-
-    size_t capacity = response.m_headers.capacity();
-    size_t total_len = response.m_headers.size() + cleaned_up_length;
-    if (capacity < total_len) {
-        do {
-            capacity *= 2;
-        } while (capacity < total_len);
-        response.m_headers.reserve(capacity);
-    }
-
-    // Append the entire header into the full header buffer.
-    response.m_headers.append(data_view.data(), cleaned_up_length);
-
-    // Calculate and append the Header view object.
-    const char* start = response.m_headers.c_str();
-    auto total_length = response.m_headers.length();
-    std::string_view request_data_view { (start + total_length) - cleaned_up_length, cleaned_up_length };
-    response.m_headers_idx.emplace_back(request_data_view);
+    response.m_headers.emplace_back(std::string { data_view.data(), data_view.length() });
 
     return data_length; // return original size for curl to continue processing
 }

src/Header.cpp

@@ -0,0 +1,35 @@
+#include "lift/Header.hpp"
+
+#include <string>
+
+namespace lift {
+
+Header::Header(
+    std::string_view name,
+    std::string_view value)
+{
+    m_header.reserve(name.length() + value.length() + 2);
+    m_header.append(name.data(), name.length());
+    m_header.append(": ");
+    m_header.append(value.data(), value.length());
+
+    m_colon_pos = name.length();
+}
+
+Header::Header(
+    std::string header_full)
+    : m_header(std::move(header_full))
+{
+    m_colon_pos = m_header.find(":");
+    // class assumes the two bytes ": " always exist, enforce that.
+    if (m_colon_pos == std::string::npos) {
+        m_colon_pos = m_header.length();
+        m_header.append(": ");
+    } else if (m_colon_pos == m_header.length() - 1) {
+        m_header.append(" ");
+    } else if (m_header[m_colon_pos + 1] != ' ') {
+        m_header.insert(m_colon_pos + 1, 1, ' ');
+    }
+}
+
+} // lift