Merge TASK-017: http_request container getters return const&

Author: etr

examples/args_processing.cpp

@@ -40,9 +40,11 @@ class args_resource : public httpserver::http_resource {
 
         response_body << "=== Using get_args() (supports multiple values per key) ===\n\n";
 
-        // get_args() returns a map where each key maps to an http_arg_value.
-        // http_arg_value contains a vector of values for parameters like "?id=1&id=2&id=3"
-        auto args = req.get_args();
+        // get_args() returns a const reference to a map where each key
+        // maps to an http_arg_value. http_arg_value contains a vector of
+        // values for parameters like "?id=1&id=2&id=3". The reference
+        // remains valid for the duration of this handler call.
+        const auto& args = req.get_args();
         for (const auto& [key, arg_value] : args) {
             response_body << "Key: " << key << "\n";
             // Use get_all_values() to get all values for this key

specs/tasks/M3-request/TASK-017.md

@@ -8,10 +8,10 @@
 Stop copying maps/vectors out of `http_request` on every getter call.
 
 **Action Items:**
-- [ ] Change return types of `get_args`, `get_path_pieces`, `get_files`, `get_headers`, `get_footers`, `get_cookies` from by-value to `const ContainerType&`.
-- [ ] Mark each getter `const`.
-- [ ] If a v1 caller relied on copy semantics (modifying the returned value), update it to copy explicitly at the call site.
-- [ ] Document in the header that the returned reference is valid until the request object is destroyed (typically until handler return).
+- [x] Change return types of `get_args`, `get_path_pieces`, `get_files`, `get_headers`, `get_footers`, `get_cookies` from by-value to `const ContainerType&`.
+- [x] Mark each getter `const`.
+- [x] If a v1 caller relied on copy semantics (modifying the returned value), update it to copy explicitly at the call site.
+- [x] Document in the header that the returned reference is valid until the request object is destroyed (typically until handler return).
 
 **Dependencies:**
 - Blocked by: TASK-015
@@ -27,4 +27,4 @@ Stop copying maps/vectors out of `http_request` on every getter call.
 **Related Requirements:** PRD-REQ-REQ-001
 **Related Decisions:** §4.2
 
-**Status:** Not Started
+**Status:** Done

specs/tasks/_index.md

@@ -99,7 +99,7 @@ Nominally: **13 sequential tasks**, each S–XL. Most other tasks parallelize of
 | TASK-014 | `webserver_impl` skeleton (PIMPL prep) | M3 | In Progress | TASK-002 |
 | TASK-015 | `http_request_impl` skeleton (PIMPL split) | M3 | In Progress | TASK-002, TASK-014 |
 | TASK-016 | Per-connection arena for `http_request_impl` | M3 | Done | TASK-014, TASK-015 |
-| TASK-017 | `http_request` container getters return `const&` | M3 | Not Started | TASK-015 |
+| TASK-017 | `http_request` container getters return `const&` | M3 | Done | TASK-015 |
 | TASK-018 | `http_request` single-key getters return `string_view`, all const | M3 | Not Started | TASK-015, TASK-016 |
 | TASK-019 | High-level GnuTLS accessors replacing `gnutls_session_t` | M3 | Not Started | TASK-015 |
 | TASK-020 | Final public-header backend-include sweep | M3 | Not Started | TASK-014, TASK-015, TASK-019 |

specs/unworked_review_issues/2026-05-05_203613_task-017.md

@@ -168,31 +168,61 @@ MHD_Result http_request_impl::build_request_header(void* cls, MHD_ValueKind kind
     return MHD_YES;
 }
 
-http::header_view_map http_request_impl::get_headerlike_values(MHD_ValueKind kind) const {
-    http::header_view_map headers;
+const http::header_view_map& http_request_impl::ensure_headerlike_cache(MHD_ValueKind kind) const {
+    // Pick the cache slot and build-flag matching `kind`. We resolve them
+    // up front so the cold (build) and warm (return) paths share a single
+    // reference without re-switching.
+    http::header_view_map* cache = nullptr;
+    bool* built = nullptr;
+    const http::header_map* local_fallback = nullptr;
+    switch (kind) {
+        case MHD_HEADER_KIND:
+            cache = &headers_cached_;
+            built = &headers_cache_built_;
+            local_fallback = &headers_local;
+            break;
+        case MHD_FOOTER_KIND:
+            cache = &footers_cached_;
+            built = &footers_cache_built_;
+            local_fallback = &footers_local;
+            break;
+        case MHD_COOKIE_KIND:
+            cache = &cookies_cached_;
+            built = &cookies_cache_built_;
+            local_fallback = &cookies_local;
+            break;
+        default:
+            // Unsupported kind: hand back the headers cache (kept empty)
+            // as a safe fallback; the public API never reaches here.
+            cache = &headers_cached_;
+            built = &headers_cache_built_;
+            local_fallback = &headers_local;
+            break;
+    }
+
+    if (*built) {
+        return *cache;
+    }
 
-    // Test-request path: connection_ is null, build view map from local storage.
+    // Test-request path: connection_ is null, build the view map from
+    // local owning storage (the create_test_request builder populated it).
     if (connection_ == nullptr) {
-        const auto* map = [&]() -> const http::header_map* {
-            switch (kind) {
-                case MHD_HEADER_KIND: return &headers_local;
-                case MHD_FOOTER_KIND: return &footers_local;
-                case MHD_COOKIE_KIND: return &cookies_local;
-                default:             return nullptr;
-            }
-        }();
-        if (map != nullptr) {
-            for (const auto& [k, v] : *map) {
-                headers[k] = v;
+        if (local_fallback != nullptr) {
+            for (const auto& [k, v] : *local_fallback) {
+                (*cache)[k] = v;
             }
         }
-        return headers;
+        *built = true;
+        return *cache;
     }
 
+    // Live-request path: ask MHD to enumerate values for this kind into
+    // the cache. The string_view keys/values alias MHD-owned storage that
+    // outlives the request handler.
     MHD_get_connection_values(connection_, kind, &http_request_impl::build_request_header,
-                              reinterpret_cast<void*>(&headers));
-
-    return headers;
+                              reinterpret_cast<void*>(cache));
+    *built = true;
+    return *cache;
 }
 
 MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
@@ -652,18 +682,21 @@ void http_request::set_method(const std::string& method) {
     this->method = method;
 }
 
-const std::vector<std::string> http_request::get_path_pieces() const {
+const std::vector<std::string>& http_request::get_path_pieces() const {
+    // TASK-017: lazily populate the public-typed mirror cache from the
+    // (already-built) pmr-backed `path_pieces` and return it by const&.
+    // Two caches in lockstep -- the pmr one stays arena-friendly for any
+    // future internal consumer; the public one is what the API exposes.
     impl_->ensure_path_pieces_cached(path);
-    // path_pieces is now pmr-backed; copy element-wise back into a default-
-    // allocator std::vector<std::string> for the public return type. The
-    // copy is intrinsic to the v1 API contract; TASK-017 narrows this to
-    // a const& return that aliases the impl-side storage.
-    std::vector<std::string> out;
-    out.reserve(impl_->path_pieces.size());
-    for (const auto& p : impl_->path_pieces) {
-        out.emplace_back(p.data(), p.size());
+    if (!impl_->path_pieces_public_built_) {
+        impl_->path_pieces_public_.clear();
+        impl_->path_pieces_public_.reserve(impl_->path_pieces.size());
+        for (const auto& p : impl_->path_pieces) {
+            impl_->path_pieces_public_.emplace_back(p.data(), p.size());
+        }
+        impl_->path_pieces_public_built_ = true;
     }
-    return out;
+    return impl_->path_pieces_public_;
 }
 
 const std::string http_request::get_path_piece(int index) const {
@@ -725,24 +758,24 @@ std::string_view http_request::get_header(std::string_view key) const {
     return impl_->get_connection_value(key, MHD_HEADER_KIND);
 }
 
-const http::header_view_map http_request::get_headers() const {
-    return impl_->get_headerlike_values(MHD_HEADER_KIND);
+const http::header_view_map& http_request::get_headers() const {
+    return impl_->ensure_headerlike_cache(MHD_HEADER_KIND);
 }
 
 std::string_view http_request::get_footer(std::string_view key) const {
     return impl_->get_connection_value(key, MHD_FOOTER_KIND);
 }
 
-const http::header_view_map http_request::get_footers() const {
-    return impl_->get_headerlike_values(MHD_FOOTER_KIND);
+const http::header_view_map& http_request::get_footers() const {
+    return impl_->ensure_headerlike_cache(MHD_FOOTER_KIND);
 }
 
 std::string_view http_request::get_cookie(std::string_view key) const {
     return impl_->get_connection_value(key, MHD_COOKIE_KIND);
 }
 
-const http::header_view_map http_request::get_cookies() const {
-    return impl_->get_headerlike_values(MHD_COOKIE_KIND);
+const http::header_view_map& http_request::get_cookies() const {
+    return impl_->ensure_headerlike_cache(MHD_COOKIE_KIND);
 }
 
 http_arg_value http_request::get_arg(std::string_view key) const {
@@ -772,17 +805,25 @@ std::string_view http_request::get_arg_flat(std::string_view key) const {
     return impl_->get_connection_value(key, MHD_GET_ARGUMENT_KIND);
 }
 
-const http::arg_view_map http_request::get_args() const {
+const http::arg_view_map& http_request::get_args() const {
+    // TASK-017: lazily populate the args view-map cache from the pmr-backed
+    // `unescaped_args` (which is itself populated lazily by populate_args()).
     impl_->populate_args();
-
-    http::arg_view_map arguments;
-    for (const auto& [key, value] : impl_->unescaped_args) {
-        auto& arg_values = arguments[key];
-        for (const auto& v : value) {
-            arg_values.values.push_back(v);
+    if (!impl_->args_view_cache_built_) {
+        impl_->args_view_cached_.clear();
+        for (const auto& [key, value] : impl_->unescaped_args) {
+            // The string_view keys/values alias the pmr-backed strings owned
+            // by `unescaped_args` -- same lifetime as the request.
+            auto& arg_values = impl_->args_view_cached_[
+                std::string_view(key.data(), key.size())];
+            arg_values.values.reserve(value.size());
+            for (const auto& v : value) {
+                arg_values.values.emplace_back(v.data(), v.size());
+            }
         }
+        impl_->args_view_cache_built_ = true;
     }
-    return arguments;
+    return impl_->args_view_cached_;
 }
 
 const std::map<std::string_view, std::string_view, http::arg_comparator> http_request::get_args_flat() const {
@@ -798,7 +839,7 @@ http::file_info& http_request::get_or_create_file_info(const std::string& key, c
     return impl_->files_[key][upload_file_name];
 }
 
-const std::map<std::string, std::map<std::string, http::file_info>> http_request::get_files() const {
+const std::map<std::string, std::map<std::string, http::file_info>>& http_request::get_files() const noexcept {
     return impl_->files_;
 }
 

src/http_request.cpp

@@ -176,6 +176,40 @@ class http_request_impl {
     mutable bool args_populated = false;
     mutable bool path_pieces_cached = false;
 
+    // TASK-017: per-request caches for the six container getters. These
+    // are typed in the public-API container types (default-allocator) so
+    // http_request::get_*() can return `const ContainerType&` aliasing
+    // impl-owned storage. Each is built lazily on the first getter call
+    // and reused on subsequent calls.
+    //
+    // Allocator note: the public container types embed std::string_view
+    // (header_view_map / arg_view_map) or std::string (path_pieces_public_)
+    // and use the default allocator. They cannot be made PMR without
+    // changing the public surface (TASK-017 plan, "Storage strategy").
+    // The first call therefore allocates on the global heap; subsequent
+    // calls are O(1) and zero-allocating -- a strict win over v1, which
+    // paid the allocation on every call.
+    //
+    // The header/footer/cookie caches alias MHD-owned strings via
+    // string_view, so they share the request's lifetime; the arg-view
+    // cache aliases the impl's pmr-backed `unescaped_args`, same lifetime.
+    mutable http::header_view_map headers_cached_;
+    mutable http::header_view_map footers_cached_;
+    mutable http::header_view_map cookies_cached_;
+    mutable bool headers_cache_built_ = false;
+    mutable bool footers_cache_built_ = false;
+    mutable bool cookies_cache_built_ = false;
+
+    mutable http::arg_view_map args_view_cached_;
+    mutable bool args_view_cache_built_ = false;
+
+    // Public-typed mirror of `path_pieces` (the pmr::vector<pmr::string>
+    // already kept above). Two caches in lockstep: the pmr version stays
+    // arena-friendly for any future internal consumer; the public version
+    // is what get_path_pieces() returns by const&.
+    mutable std::vector<std::string> path_pieces_public_;
+    mutable bool path_pieces_public_built_ = false;
+
 #ifdef HAVE_GNUTLS
     mutable bool client_cert_fields_cached = false;
     mutable std::pmr::string client_cert_dn;
@@ -189,7 +223,11 @@ class http_request_impl {
 
     // --- helpers (moved out of http_request public header) ---
     std::string_view get_connection_value(std::string_view key, MHD_ValueKind kind) const;
-    http::header_view_map get_headerlike_values(MHD_ValueKind kind) const;
+    // TASK-017: ensures the cache for `kind` (HEADER / FOOTER / COOKIE) is
+    // populated and returns a const reference to it. First call fills the
+    // map (test-request fallback or MHD scan); subsequent calls return
+    // the same reference in O(1).
+    const http::header_view_map& ensure_headerlike_cache(MHD_ValueKind kind) const;
     void populate_args() const;
     void ensure_path_pieces_cached(std::string_view path) const;
 

src/httpserver/detail/http_request_impl.hpp

@@ -110,6 +110,25 @@ struct http_request_impl_deleter {
  * get_pass(), get_digested_user(), get_header(), get_footer(),
  * get_cookie(), get_requestor().
  * (security-reviewer-iter1-1, CWE-416 Use After Free.)
+ *
+ * ### Container reference lifetime contract (TASK-017)
+ *
+ * The container getters `get_headers()`, `get_footers()`, `get_cookies()`,
+ * `get_args()`, `get_path_pieces()`, and `get_files()` all return a
+ * `const ContainerType&` rather than a by-value copy. The reference and
+ * any iterators / pointers / element references derived from it remain
+ * valid until the `http_request` object is destroyed (typically when the
+ * handler invocation returns).
+ *
+ * In particular, the `std::string_view` keys and values held inside the
+ * `header_view_map` and `arg_view_map` returned by these getters carry the
+ * same lifetime restriction as the standalone `std::string_view` accessors
+ * above: do not store them past the handler call frame. Copy explicitly
+ * if a longer lifetime is required.
+ *
+ * Implementation note: the first call to get_headers / get_footers /
+ * get_cookies / get_args / get_path_pieces lazily populates a per-request
+ * cache; subsequent calls are O(1) and return the same reference.
 **/
 class http_request {
  public:
@@ -155,9 +174,10 @@ class http_request {
 
      /**
       * Method used to get all pieces of the path requested; considering an url splitted by '/'.
-      * @return a vector of strings containing all pieces
+      * @return a vector of strings containing all pieces. The reference
+      *         remains valid until the http_request is destroyed.
      **/
-     const std::vector<std::string> get_path_pieces() const;
+     [[nodiscard]] const std::vector<std::string>& get_path_pieces() const;
 
      /**
       * Method used to obtain a specified piece of the path; considering an url splitted by '/'.
@@ -176,30 +196,35 @@ class http_request {
 
      /**
       * Method used to get all headers passed with the request.
-      * @param result a map<string, string> > that will be filled with all headers
-      * @result the size of the map
+      * @return a const reference to a map<string_view, string_view>
+      *         containing all headers. The reference (and the views it
+      *         holds) remain valid until the http_request is destroyed.
      **/
-     const http::header_view_map get_headers() const;
+     [[nodiscard]] const http::header_view_map& get_headers() const;
 
      /**
       * Method used to get all footers passed with the request.
-      * @param result a map<string, string> > that will be filled with all footers
-      * @result the size of the map
+      * @return a const reference to a map<string_view, string_view>
+      *         containing all footers. The reference (and the views it
+      *         holds) remain valid until the http_request is destroyed.
      **/
-     const http::header_view_map get_footers() const;
+     [[nodiscard]] const http::header_view_map& get_footers() const;
 
      /**
       * Method used to get all cookies passed with the request.
-      * @param result a map<string, string> > that will be filled with all cookies
-      * @result the size of the map
+      * @return a const reference to a map<string_view, string_view>
+      *         containing all cookies. The reference (and the views it
+      *         holds) remain valid until the http_request is destroyed.
      **/
-     const http::header_view_map get_cookies() const;
+     [[nodiscard]] const http::header_view_map& get_cookies() const;
 
      /**
       * Method used to get all args passed with the request.
-      * @result the size of the map
+      * @return a const reference to the args map. The reference (and the
+      *         string_view keys/values it holds) remain valid until the
+      *         http_request is destroyed.
      **/
-     const http::arg_view_map get_args() const;
+     [[nodiscard]] const http::arg_view_map& get_args() const;
 
      /**
       * Method used to get all args passed with the request. If any key has multiple
@@ -218,9 +243,11 @@ class http_request {
 
      /**
       * Method used to get all files passed with the request.
-      * @result result a map<std::string, map<std::string, http::file_info> > that will be filled with all files
+      * @return a const reference to a map<std::string, map<std::string,
+      *         http::file_info>> containing all files. The reference
+      *         remains valid until the http_request is destroyed.
      **/
-     const std::map<std::string, std::map<std::string, http::file_info>> get_files() const;
+     [[nodiscard]] const std::map<std::string, std::map<std::string, http::file_info>>& get_files() const noexcept;
 
      /**
       * Method used to get a specific header passed with the request.