TASK-058 step 1: canonicalize_lookup_path returns string_view

etr · claude · etr · commit e2f730dd7c76 · 2026-06-01T10:10:20.000-07:00
Refactor `canonicalize_lookup_path` (anon-ns helper in webserver_dispatch.cpp) to return a std::string_view into either: - the immutable "/" literal (empty-input case), - the caller's @p path argument (already-canonical happy path -- no allocation), or - a caller-owned scratch std::string (rewrite case -- one allocation per lookup_v2 call, bounded by the input size). Most warm-path inputs arrive canonical from MHD via modded_request::standardized_url, so the warm GET path now allocates zero heap memory for canonicalisation. The miss path still pays one std::string construction when assembling the cache_key (line 161 of webserver_dispatch.cpp); the warm-cache path never reaches that construction. LIFETIME contract documented above the helper: the returned view is valid for the duration of the call only. cache_key construction at the miss path naturally copies via std::string, so callers cannot inadvertently store a dangling view. Pinned by new test/unit/route_lookup_canonicalize_test.cpp covering: empty input -> "/", missing leading slash, trailing slash stripping, "/" identity, idempotency. All five tests pass before and after the refactor (regression net pinning behaviour). The step 4 heap profile closes the allocation-elimination side. Bench (debug build, before vs after; numbers dominated by unordered_map probe + mutex lock so the saving is mostly visible in release builds and via heap profiler): canonicalize: 620 ns -> 619 ns (within noise; the allocation removal is observable on the heap profiler, not the wall-clock median in a debug build) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
diff --git a/src/detail/webserver_dispatch.cpp b/src/detail/webserver_dispatch.cpp
@@ -123,25 +123,57 @@ void webserver_impl::invalidate_route_cache() {
 // would never share an entry. Matches the v1 dispatch path, which
 // constructs a non-registration http_endpoint at lookup time and so
 // gets the same normalization for free.
-static std::string canonicalize_lookup_path(const std::string& path) {
+//
+// TASK-058 step 1: return a string_view so the happy path (input
+// already canonical) allocates zero heap memory.  On the rewrite
+// path the canonicalised form is written into the caller-owned
+// @p scratch buffer and a view into that buffer is returned.
+//
+// LIFETIME: the returned view is valid for the duration of the
+// call chain only; it points at either the immutable "/" literal,
+// the caller's @p path argument, or the caller's @p scratch buffer.
+// Any caller storing the view must copy it into an owning string
+// first.  The cache layer (cache_key constructed below) already
+// copies via std::string, so the contract is naturally respected.
+static std::string_view canonicalize_lookup_path(
+        std::string_view path, std::string& scratch) {
     if (path.empty()) {
-        return "/";
+        // Immutable canonical root -- no scratch usage.
+        return std::string_view{"/", 1};
+    }
+    const bool has_leading_slash = (path.front() == '/');
+    const bool has_trailing_slash = (path.size() > 1 && path.back() == '/');
+    if (has_leading_slash && !has_trailing_slash) {
+        // Already canonical: return the caller's view directly.
+        return path;
     }
-    std::string out = path;
-    if (out.front() != '/') {
-        out.insert(out.begin(), '/');
+    // Rewrite path: write the canonicalised form into the caller's
+    // scratch buffer and return a view into it.
+    scratch.clear();
+    scratch.reserve(path.size() + (has_leading_slash ? 0 : 1));
+    if (!has_leading_slash) {
+        scratch.push_back('/');
     }
-    if (out.size() > 1 && out.back() == '/') {
-        out.pop_back();
+    if (has_trailing_slash) {
+        scratch.append(path.data(), path.size() - 1);
+    } else {
+        scratch.append(path.data(), path.size());
     }
-    return out;
+    return std::string_view{scratch};
 }
 
 webserver_impl::lookup_result
 webserver_impl::lookup_v2(http_method method, const std::string& path) {
     lookup_result result;
 
-    std::string lookup_path = canonicalize_lookup_path(path);
+    // TASK-058 step 1: canonicalize_lookup_path returns a string_view
+    // into either @p path (already-canonical happy path -- no
+    // allocation), the immutable "/" literal (empty-input case), or
+    // @p canonicalize_scratch (rewrite case -- single allocation,
+    // bounded by the input size).
+    std::string canonicalize_scratch;
+    std::string_view lookup_path =
+        canonicalize_lookup_path(path, canonicalize_scratch);
 
     // Step 1: cache. Cache under the canonical key so /foo and /foo/
     // share an entry. Use find_by_view to avoid copying lookup_path
@@ -155,10 +187,11 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
         result.captured_params = std::move(cached.captured_params);
         return result;
     }
-    // Construct key by moving lookup_path into the cache_key, avoiding a
-    // second heap allocation. All subsequent tier lookups use key.path,
-    // which is the same std::string now owned by the key.
-    cache_key key{method, std::move(lookup_path)};
+    // Construct key by copying lookup_path into the cache_key.  On the
+    // miss path we pay one std::string construction; the warm-cache
+    // path never reaches this line.  All subsequent tier lookups use
+    // key.path, which is the std::string now owned by the key.
+    cache_key key{method, std::string(lookup_path)};
 
     // Step 2: walk the tiers under a shared lock.
     {
diff --git a/test/Makefile.am b/test/Makefile.am
@@ -26,7 +26,7 @@ LDADD += -lcurl
 
 AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver/ -DHTTPSERVER_COMPILATION
 METASOURCES = AUTO
-check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ban_system ws_start_stop authentication deferred http_resource http_response create_webserver create_webserver_explicit new_response_types daemon_info uri_log feature_unavailable header_hygiene_iovec header_hygiene iovec_entry http_method constants body http_response_sbo http_response_factories http_response_move_sanitizer webserver_pimpl http_request_pimpl create_test_request http_request_arena http_request_const_getters http_request_tls_accessors http_request_operator_stream webserver_register_smartptr webserver_register_path_prefix webserver_on_methods webserver_route route_table lookup_pipeline route_table_concurrency routing_regression v2_dispatch_contract threadsafety_stress webserver_features webserver_ws_unavailable webserver_register_ws_smartptr webserver_dauth_unavailable consumer_fixture header_hygiene_hooks hook_api_shape hooks_no_firing hooks_accept_ctx_shape hooks_connection_lifecycle hooks_accept_decision_banned hooks_accept_decision_throwing hooks_body_chunk_ctx_shape hooks_request_received_short_circuit hooks_body_chunk_observes_progress hooks_body_chunk_short_circuit_no_leak hooks_before_handler_ctx_shape hooks_route_resolved_miss_and_hit hooks_before_handler_short_circuit hooks_alias_count hooks_alias_functional hooks_handler_exception_chain hooks_handler_exception_user_handler_throws_continues_chain hooks_handler_exception_fallback_to_hardcoded_500 hooks_handler_exception_slot hooks_response_sent_ctx_shape hooks_request_completed_ctx_shape hooks_after_handler_replaces_response hooks_after_handler_mutates_response_in_place hooks_response_sent_carries_status_bytes_timing hooks_request_completed_fires_on_early_failure hooks_log_access_alias_slot hooks_per_route_invalid_phase_throws hooks_per_route_order hooks_per_route_early_413_per_endpoint hooks_per_route_resource_destroyed_first hooks_per_route_concurrent_registration auth_handler_optional_signature auth_handler_legacy_shim
+check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ban_system ws_start_stop authentication deferred http_resource http_response create_webserver create_webserver_explicit new_response_types daemon_info uri_log feature_unavailable header_hygiene_iovec header_hygiene iovec_entry http_method constants body http_response_sbo http_response_factories http_response_move_sanitizer webserver_pimpl http_request_pimpl create_test_request http_request_arena http_request_const_getters http_request_tls_accessors http_request_operator_stream webserver_register_smartptr webserver_register_path_prefix webserver_on_methods webserver_route route_table lookup_pipeline route_table_concurrency routing_regression route_lookup_canonicalize v2_dispatch_contract threadsafety_stress webserver_features webserver_ws_unavailable webserver_register_ws_smartptr webserver_dauth_unavailable consumer_fixture header_hygiene_hooks hook_api_shape hooks_no_firing hooks_accept_ctx_shape hooks_connection_lifecycle hooks_accept_decision_banned hooks_accept_decision_throwing hooks_body_chunk_ctx_shape hooks_request_received_short_circuit hooks_body_chunk_observes_progress hooks_body_chunk_short_circuit_no_leak hooks_before_handler_ctx_shape hooks_route_resolved_miss_and_hit hooks_before_handler_short_circuit hooks_alias_count hooks_alias_functional hooks_handler_exception_chain hooks_handler_exception_user_handler_throws_continues_chain hooks_handler_exception_fallback_to_hardcoded_500 hooks_handler_exception_slot hooks_response_sent_ctx_shape hooks_request_completed_ctx_shape hooks_after_handler_replaces_response hooks_after_handler_mutates_response_in_place hooks_response_sent_carries_status_bytes_timing hooks_request_completed_fires_on_early_failure hooks_log_access_alias_slot hooks_per_route_invalid_phase_throws hooks_per_route_order hooks_per_route_early_413_per_endpoint hooks_per_route_resource_destroyed_first hooks_per_route_concurrent_registration auth_handler_optional_signature auth_handler_legacy_shim
 
 MOSTLYCLEANFILES = *.gcda *.gcno *.gcov
 
@@ -282,6 +282,15 @@ threadsafety_stress_SOURCES = integ/threadsafety_stress.cpp
 routing_regression_SOURCES = unit/routing_regression_test.cpp
 routing_regression_LDADD = $(LDADD) -lmicrohttpd
 
+# route_lookup_canonicalize (TASK-058 step 1): pins lookup_v2's
+# canonicalisation contract behind a behaviour-only test (empty, leading-
+# slash insertion, trailing-slash stripping, "/" identity, idempotency).
+# Same -lmicrohttpd dependency as routing_regression: the webserver
+# constructor pulls libhttpserver in, which transitively touches
+# libmicrohttpd headers.
+route_lookup_canonicalize_SOURCES = unit/route_lookup_canonicalize_test.cpp
+route_lookup_canonicalize_LDADD = $(LDADD) -lmicrohttpd
+
 # v2_dispatch_contract: TASK-053. End-to-end safety net pinning the
 # observable invariants the dispatch path must satisfy AFTER finalize_answer
 # is cut over from resolve_resource_for_request (v1 maps) to
diff --git a/test/unit/route_lookup_canonicalize_test.cpp b/test/unit/route_lookup_canonicalize_test.cpp
@@ -0,0 +1,164 @@
+/*
+     This file is part of libhttpserver
+     Copyright (C) 2011-2026 Sebastiano Merlino
+
+     This library is free software; you can redistribute it and/or
+     modify it under the terms of the GNU Lesser General Public
+     License as published by the Free Software Foundation; either
+     version 2.1 of the License, or (at your option) any later version.
+
+     This library is distributed in the hope that it will be useful,
+     but WITHOUT ANY WARRANTY; without even the implied warranty of
+     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+     Lesser General Public License for more details.
+
+     You should have received a copy of the GNU Lesser General Public
+     License along with this library; if not, write to the Free Software
+     Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301
+     USA
+*/
+
+// TASK-058 step 1: pin canonicalize_lookup_path's observable contract
+// behind lookup_v2.  The static helper itself is anonymous-namespaced
+// inside webserver_dispatch.cpp and not linkable from tests, so we pin
+// its observable effect: two lookups under different spellings of the
+// same canonical path must share a cache entry (the second call hits
+// tier_hit::cache).  Also covers the boundary inputs:
+//   - empty string -> "/"
+//   - "foo" without leading slash -> "/foo"
+//   - "/foo/" with trailing slash -> "/foo"
+//   - "/" identity
+//
+// These tests must pass both BEFORE and AFTER the string_view refactor:
+// they pin behavior, not internal allocation.  The heap-profile gate in
+// step 4 closes the loop on the allocation-elimination side.
+
+#include <memory>
+#include <string>
+
+#include "./httpserver.hpp"
+#include "./httpserver/detail/webserver_impl.hpp"
+#include "./littletest.hpp"
+
+namespace ht = httpserver;
+
+namespace {
+
+class noop_resource : public ht::http_resource {
+ public:
+    ht::http_response render_get(const ht::http_request&) override {
+        return ht::http_response::string("ok");
+    }
+};
+
+ht::detail::webserver_impl& impl_of(ht::webserver& ws) {
+    return *ht::webserver_test_access::impl(ws);
+}
+
+}  // namespace
+
+LT_BEGIN_SUITE(route_lookup_canonicalize_suite)
+    void set_up() {}
+    void tear_down() {}
+LT_END_SUITE(route_lookup_canonicalize_suite)
+
+// ---------------------------------------------------------------------
+// Empty input canonicalises to "/" -> the same entry as "/" itself.
+// ---------------------------------------------------------------------
+LT_BEGIN_AUTO_TEST(route_lookup_canonicalize_suite, empty_path_maps_to_root)
+    ht::webserver ws{ht::create_webserver(8080)
+        .start_method(ht::http::http_utils::INTERNAL_SELECT)};
+    ws.register_path("/", std::make_shared<noop_resource>());
+
+    auto a = impl_of(ws).lookup_v2(ht::http_method::get, std::string(""));
+    LT_CHECK(a.found);
+
+    // Second call under the canonical spelling must hit the cache the
+    // empty-string canonicalisation populated.
+    auto b = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/"));
+    LT_CHECK(b.found);
+    LT_CHECK(b.tier == ht::detail::webserver_impl::tier_hit::cache);
+LT_END_AUTO_TEST(empty_path_maps_to_root)
+
+// ---------------------------------------------------------------------
+// Leading-slash insertion: "foo" canonicalises to "/foo".
+// ---------------------------------------------------------------------
+LT_BEGIN_AUTO_TEST(route_lookup_canonicalize_suite,
+                   missing_leading_slash_is_prepended)
+    ht::webserver ws{ht::create_webserver(8080)
+        .start_method(ht::http::http_utils::INTERNAL_SELECT)};
+    ws.register_path("/foo", std::make_shared<noop_resource>());
+
+    auto a = impl_of(ws).lookup_v2(ht::http_method::get, std::string("foo"));
+    LT_CHECK(a.found);
+    LT_CHECK(a.entry.methods == ht::method_set{}.set_all());
+
+    // The second lookup under the canonical spelling shares the cache
+    // entry installed by the first call -- proves both spellings hash
+    // to the same canonical key.
+    auto b = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/foo"));
+    LT_CHECK(b.found);
+    LT_CHECK(b.tier == ht::detail::webserver_impl::tier_hit::cache);
+LT_END_AUTO_TEST(missing_leading_slash_is_prepended)
+
+// ---------------------------------------------------------------------
+// Trailing-slash stripping: "/foo/" canonicalises to "/foo".  This is
+// the case routing_regression_test::exact_path_normalization_aliases
+// already covers end-to-end; we pin the cache-sharing property too.
+// ---------------------------------------------------------------------
+LT_BEGIN_AUTO_TEST(route_lookup_canonicalize_suite,
+                   trailing_slash_is_stripped)
+    ht::webserver ws{ht::create_webserver(8080)
+        .start_method(ht::http::http_utils::INTERNAL_SELECT)};
+    ws.register_path("/foo", std::make_shared<noop_resource>());
+
+    auto a = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/foo/"));
+    LT_CHECK(a.found);
+
+    auto b = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/foo"));
+    LT_CHECK(b.found);
+    LT_CHECK(b.tier == ht::detail::webserver_impl::tier_hit::cache);
+LT_END_AUTO_TEST(trailing_slash_is_stripped)
+
+// ---------------------------------------------------------------------
+// "/" identity: lookup_v2("/") on a registered "/" route hits exact
+// the first time, cache the second.  Pins that the canonicalisation
+// of "/" does NOT pop its single character.
+// ---------------------------------------------------------------------
+LT_BEGIN_AUTO_TEST(route_lookup_canonicalize_suite, root_identity)
+    ht::webserver ws{ht::create_webserver(8080)
+        .start_method(ht::http::http_utils::INTERNAL_SELECT)};
+    ws.register_path("/", std::make_shared<noop_resource>());
+
+    auto a = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/"));
+    LT_CHECK(a.found);
+    LT_CHECK(a.tier == ht::detail::webserver_impl::tier_hit::exact);
+
+    auto b = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/"));
+    LT_CHECK(b.found);
+    LT_CHECK(b.tier == ht::detail::webserver_impl::tier_hit::cache);
+LT_END_AUTO_TEST(root_identity)
+
+// ---------------------------------------------------------------------
+// Already-canonical input takes the no-allocation happy path after
+// step 1.  Behaviorally observable here: same-shape inputs share the
+// cache entry the prior call installed.  (The no-allocation property
+// is closed by the step 4 heap-profile gate.)
+// ---------------------------------------------------------------------
+LT_BEGIN_AUTO_TEST(route_lookup_canonicalize_suite,
+                   already_canonical_is_idempotent)
+    ht::webserver ws{ht::create_webserver(8080)
+        .start_method(ht::http::http_utils::INTERNAL_SELECT)};
+    ws.register_path("/foo", std::make_shared<noop_resource>());
+
+    auto a = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/foo"));
+    LT_CHECK(a.found);
+
+    auto b = impl_of(ws).lookup_v2(ht::http_method::get, std::string("/foo"));
+    LT_CHECK(b.found);
+    LT_CHECK(b.tier == ht::detail::webserver_impl::tier_hit::cache);
+LT_END_AUTO_TEST(already_canonical_is_idempotent)
+
+LT_BEGIN_AUTO_TEST_ENV()
+    AUTORUN_TESTS()
+LT_END_AUTO_TEST_ENV()
