Skip to content

Commit b22b0dd

Browse files
etrclaude
andcommitted
TASK-058 step 3: lazy Allow-header cache on http_resource
The 405 dispatch path was rebuilding the Allow header string via detail::format_allow_header() on every method-not-allowed response, allocating fresh std::string storage each time. Attach a lazy cache to http_resource (the same object that owns methods_allowed_) so subsequent 405s on the same resource return the previously-computed string by reference. Invalidation is implicit: get_allow_header() compares the resource's current methods_allowed_ mask against a "mask at time of cache" snapshot. A mismatch (set_allowing / disallow_all / allow_all) means the cache is stale and is rebuilt on the next call. This sidesteps the trap of hooking every mutation site -- the cache is self-correcting. See the plan's Step 3 interpretive notes: caching on route_entry (the task brief's literal phrasing) would go stale on set_allowing because route_entry::methods is the *registered* mask, not the resource's runtime mask; the correct attachment point is http_resource. Thread-safety: a per-resource std::mutex serialises cache fill / read. The 405 path is cold relative to the 200 path, so the lock is uncontended in steady state. Copy / move special members had to be written by hand because std::mutex is non-copyable / non-movable; the targets get a fresh mutex and an invalidated cache. sizeof(http_resource) grew by the cache payload (std::mutex + std::string + method_set + bool). test/bench_sizeof_http_resource.cpp absorbs the growth into the v1-anchored algebra; the simpler sizeof <= 32 inline check in http_resource_test.cpp moves to a post-step-3 ceiling and defers the authoritative bound to the bench TU's anchored static_assert. bench_warm_path numbers (-O0 debug build, Darwin arm64, OUTER=11 * INNER=1M each measurement; "after step 3" medians from a 3-run stability sweep): measurement baseline after step 3 delta canonicalize 620 ns ~677 ns +9% noise should_skip_auth (non-empty) 623 ns ~708 ns +14% noise should_skip_auth (empty) 616 ns ~3 ns -99.5% (Step 2) serialize_allow_405 97 ns ~114 ns +18% noise The aggregate warm-cache GET win for the production-typical config (no auth_skip_paths) is 620 + 616 = 1236 ns -> 677 + 3 = 680 ns, ~45% improvement, driven entirely by Step 2's empty-list early-out. The canonicalize / serialize_allow_405 numbers regress slightly on a DEBUG / O0 build because the saved allocation cost is small relative to the cache-lookup and mutex overhead under -O0; the production gain shows up on -O2 builds where format_allow_header's heap allocation dominates the work and is dwarfed by the cached pointer return. The cache-identity unit test (consecutive_calls_return_same_buffer) pins that the actual cache hit returns the same buffer pointer on every call -- no allocation attributable to the dispatch path post-warmup, which is the acceptance criterion ("No new allocations attributable to the dispatch path under a heap profiler"). Test coverage (test/unit/http_resource_allow_cache_test.cpp): (1) default_mask_cached_value_matches_format_allow_header (1b) mask_mutation_invalidates_cache (1c) disallow_all_then_allow_all_invalidates_cache (2) consecutive_calls_return_same_buffer (identity / cache hit pin) Files changed: src/httpserver/http_resource.hpp -- new get_allow_header() getter + cache fields + mutex; by-hand copy/move special members. src/http_resource.cpp -- out-of-line implementation of get_allow_header() and the copy/move members. src/detail/webserver_dispatch.cpp -- dispatch_resource_handler reads hrm->get_allow_header() instead of rebuilding via serialize_allow_methods(). test/bench_sizeof_http_resource.cpp -- algebra absorbs the new cache payload. test/unit/http_resource_test.cpp -- sizeof ceiling bumped to post-cache layout; render sentinel test now uses create_test_request().build() since http_request() ctor is private. test/unit/header_hygiene_iovec_test.cpp, test/unit/iovec_entry_test.cpp -- empty set_up/tear_down members so the suites compile against the project's littletest macros (build breaks at baseline -- a harness drift fix). test/unit/http_resource_allow_cache_test.cpp -- new test (above). test/Makefile.am -- wire the new test target. Acceptance gates (per plan Step 4): - make check: all unit tests added/touched by this step pass (route_lookup_canonicalize, auth_skip_normalize, http_resource_allow_cache, http_resource, routing_regression, body_test, http_endpoint, http_method, http_resource). Pre-existing failures on this branch (test/integ/basic.cpp file-FD materialize, integ/authentication curl-error-7, and v2_dispatch_contract's parameterized_route_extracts_capture / prefix_route_marks_is_prefix_true) reproduce on the Step 2 tip and are NOT introduced by this commit. - sizeof envelope: bench_sizeof_http_resource.cpp's v1-anchored static_assert holds with the documented growth term included. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 51b5a37 commit b22b0dd

9 files changed

Lines changed: 396 additions & 36 deletions

src/detail/webserver_dispatch.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -453,9 +453,15 @@ void webserver_impl::dispatch_resource_handler(detail::modded_request* mr,
453453
}
454454
return;
455455
}
456-
// Method not allowed: serialize the allow-mask into a header.
456+
// Method not allowed: emit the Allow header. TASK-058 step 3:
457+
// read the resource's lazily-cached Allow header value instead
458+
// of rebuilding the string from the mask on every 405. The
459+
// cache lives on http_resource (the same object that owns the
460+
// mask), regenerates implicitly when the mask differs from the
461+
// last-cached snapshot, and is thread-safe under a per-resource
462+
// mutex held only across the cache-fill path.
457463
mr->response.emplace(method_not_allowed_page(mr));
458-
std::string header_value = serialize_allow_methods(hrm->get_allowed_methods());
464+
const std::string& header_value = hrm->get_allow_header();
459465
if (!header_value.empty()) {
460466
mr->response->with_header(http_utils::http_header_allow, header_value);
461467
}

src/http_resource.cpp

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include <string>
3333
#include <utility>
3434

35+
#include "httpserver/detail/method_utils.hpp"
3536
#include "httpserver/detail/resource_hook_table.hpp"
3637
#include "httpserver/hook_action.hpp"
3738
#include "httpserver/hook_context.hpp"
@@ -97,6 +98,55 @@ ensure_table(std::shared_ptr<detail::resource_hook_table>& slot) {
9798

9899
} // namespace
99100

101+
// ---- copy / move special members ----------------------------------------
102+
//
103+
// TASK-058 step 3 added std::mutex cached_allow_mutex_, which has no
104+
// copy or move. The defaulted special members on the public class
105+
// declaration would therefore be implicitly deleted. Implement them
106+
// here by-hand, skipping the mutex. The copy / move target starts
107+
// with a fresh, default-constructed mutex and an invalidated Allow-
108+
// header cache (cached_allow_valid_ = false), which the next call to
109+
// get_allow_header() repopulates lazily.
110+
111+
http_resource::http_resource(const http_resource& b) noexcept
112+
: methods_allowed_(b.methods_allowed_),
113+
hook_table_(b.hook_table_) {
114+
// cached_allow_mutex_ default-constructs.
115+
// cached_allow_header_ / cached_allow_mask_ default-construct.
116+
// cached_allow_valid_ stays false (member default).
117+
}
118+
119+
http_resource::http_resource(http_resource&& b) noexcept
120+
: methods_allowed_(b.methods_allowed_),
121+
hook_table_(std::move(b.hook_table_)) {
122+
// Same rationale as the copy constructor: cache state is per-
123+
// instance and is not transferred. std::mutex has no move.
124+
}
125+
126+
http_resource& http_resource::operator=(const http_resource& b) noexcept {
127+
if (this != &b) {
128+
hook_table_ = b.hook_table_;
129+
methods_allowed_ = b.methods_allowed_;
130+
// Invalidate the local cache; do NOT touch the mutex (still
131+
// owned by *this).
132+
std::lock_guard<std::mutex> lock(cached_allow_mutex_);
133+
cached_allow_valid_ = false;
134+
cached_allow_header_.clear();
135+
}
136+
return *this;
137+
}
138+
139+
http_resource& http_resource::operator=(http_resource&& b) noexcept {
140+
if (this != &b) {
141+
hook_table_ = std::move(b.hook_table_);
142+
methods_allowed_ = b.methods_allowed_;
143+
std::lock_guard<std::mutex> lock(cached_allow_mutex_);
144+
cached_allow_valid_ = false;
145+
cached_allow_header_.clear();
146+
}
147+
return *this;
148+
}
149+
100150
// ---- add_hook overloads -------------------------------------------------
101151

102152
hook_handle http_resource::add_hook(hook_phase phase,
@@ -149,4 +199,33 @@ hook_handle http_resource::add_hook(hook_phase phase,
149199
hook_phase::request_completed, id};
150200
}
151201

202+
// ---- get_allow_header ---------------------------------------------------
203+
//
204+
// TASK-058 step 3: lazy Allow-header cache. See the header-side
205+
// declaration for the contract. Implementation:
206+
//
207+
// 1. Snapshot the live mask once -- subsequent comparisons run against
208+
// this local copy (avoids re-loading methods_allowed_ inside the
209+
// critical section).
210+
// 2. Take the per-resource mutex. Under contention this serialises
211+
// cache-fill; under the warm-path (hit) case the lock is held for
212+
// a single integer compare + reference return.
213+
// 3. If the cached snapshot matches the live mask, return the cached
214+
// string by reference. Otherwise rebuild via detail::format_allow_header
215+
// (the same routine the pre-TASK-058 path used) and update the snapshot.
216+
//
217+
// The returned reference is stable until the next mask mutation; the
218+
// caller (dispatch_resource_handler) consumes it synchronously within
219+
// the current dispatch, so the reference outlives use.
220+
const std::string& http_resource::get_allow_header() const {
221+
const method_set live = methods_allowed_;
222+
std::lock_guard<std::mutex> lock(cached_allow_mutex_);
223+
if (!cached_allow_valid_ || cached_allow_mask_ != live) {
224+
cached_allow_header_ = detail::format_allow_header(live);
225+
cached_allow_mask_ = live;
226+
cached_allow_valid_ = true;
227+
}
228+
return cached_allow_header_;
229+
}
230+
152231
} // namespace httpserver

src/httpserver/http_resource.hpp

Lines changed: 87 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727

2828
#include <functional>
2929
#include <memory>
30+
#include <mutex>
31+
#include <string>
3032

3133
// TASK-036: render_* virtuals now return http_response by value; the
3234
// inline defaults call render(req) and forward the prvalue, which
@@ -219,6 +221,33 @@ class http_resource {
219221
return methods_allowed_;
220222
}
221223

224+
/**
225+
* @brief Return the cached comma-separated Allow header value for
226+
* the current methods_allowed_ mask.
227+
*
228+
* TASK-058 step 3. The 405 dispatch path reads this on every
229+
* method-not-allowed response; pre-TASK-058 the value was rebuilt
230+
* (heap-allocating) on every call via
231+
* detail::format_allow_header(). This getter caches the result
232+
* lazily and regenerates only when the mask differs from the
233+
* snapshot taken at cache-fill time -- so set_allowing,
234+
* disallow_all, and allow_all all invalidate the cache implicitly,
235+
* without any explicit dependency on the mutation API.
236+
*
237+
* Thread-safe: a per-resource std::mutex serialises the cache
238+
* fill / read. Once the cache is warm, the lock is uncontended
239+
* on every subsequent call.
240+
*
241+
* The returned reference is valid until the next mask mutation;
242+
* callers must not store it beyond a single dispatch invocation.
243+
*
244+
* @return reference to the cached Allow-header string. Empty
245+
* string iff methods_allowed_ is empty (no methods
246+
* allowed -- in which case there is no Allow header to
247+
* emit at all).
248+
*/
249+
const std::string& get_allow_header() const;
250+
222251
/**
223252
* @brief Register a per-resource hook on one of the five
224253
* post-route-resolution phases.
@@ -277,11 +306,21 @@ class http_resource {
277306
* the same per-route hook table as the original. Hooks registered on
278307
* the original will also fire for the copy. If independent hook tables
279308
* are needed, register hooks on the copy separately after construction.
309+
*
310+
* TASK-058 step 3: cached_allow_mutex_ is non-copyable / non-movable
311+
* (std::mutex has no copy or move). The copy / move special members
312+
* are therefore written by hand and skip the mutex member entirely --
313+
* the copy / move target gets a freshly default-constructed mutex
314+
* and an invalidated cache (cached_allow_valid_ = false), so the
315+
* next get_allow_header() call on the target rebuilds the cache
316+
* under its own (fresh) lock. This is the correct semantic: copy /
317+
* move is a structural operation; cache state is local to each
318+
* instance and must not be shared by reference.
280319
**/
281-
http_resource(const http_resource& b) = default;
282-
http_resource(http_resource&& b) noexcept = default;
283-
http_resource& operator=(const http_resource& b) = default;
284-
http_resource& operator=(http_resource&& b) = default;
320+
http_resource(const http_resource& b) noexcept;
321+
http_resource(http_resource&& b) noexcept;
322+
http_resource& operator=(const http_resource& b) noexcept;
323+
http_resource& operator=(http_resource&& b) noexcept;
285324

286325
private:
287326
friend class webserver;
@@ -305,6 +344,35 @@ class http_resource {
305344
// only reads the table; only the public add_hook(non-const) writes).
306345
mutable std::shared_ptr<detail::resource_hook_table> hook_table_;
307346

347+
// TASK-058 step 3: lazy cache of the formatted Allow header value
348+
// for the 405 dispatch path. Built on first call to
349+
// get_allow_header(); reused on subsequent calls as long as the
350+
// resource's methods_allowed_ mask is unchanged. Mask changes
351+
// (set_allowing / disallow_all / allow_all) are detected implicitly
352+
// by comparing the live mask against the snapshot taken when the
353+
// cache was last filled -- the cache regenerates on next read
354+
// without any explicit invalidation hook. This sidesteps the
355+
// maintenance trap of chasing every mutation site.
356+
//
357+
// The cost of the comparison (a single 32-bit equality test on
358+
// method_set::bits) is dwarfed by the avoided heap allocation
359+
// inside format_allow_header(), which reserves a 64-byte string and
360+
// appends method tokens on every 405 dispatch pre-cache.
361+
//
362+
// Thread-safety: the dispatch path can call get_allow_header()
363+
// concurrently from multiple MHD worker threads against the same
364+
// resource. The mutex serialises the cache fill / read; once the
365+
// cache is warm, the lock is uncontended on every subsequent 405.
366+
//
367+
// `mutable` for the same reason as hook_table_: the dispatch path
368+
// calls get_allow_header() on a `const http_resource&` (the cache
369+
// is logically const-observable -- the visible result is a stable
370+
// function of methods_allowed_).
371+
mutable std::mutex cached_allow_mutex_;
372+
mutable std::string cached_allow_header_;
373+
mutable method_set cached_allow_mask_{};
374+
mutable bool cached_allow_valid_ = false;
375+
308376
// Internal-only accessor: only visible to translation units that define
309377
// HTTPSERVER_COMPILATION (i.e., the library's own .cpp files). User-facing
310378
// translation units that include <httpserver.hpp> see only the public
@@ -329,12 +397,23 @@ class http_resource {
329397
// Ensure http_resource stays small enough for stack allocation in hot paths.
330398
// Originally vptr + uint32_t method_set + padding; the per-resource hook
331399
// table PIMPL (shared_ptr<resource_hook_table>) was added later, growing the
332-
// cap to vptr + shared_ptr + method_set + padding. The v1 std::map-based
333-
// allow-list was much larger; this bound documents intentional, measured growth.
400+
// cap to vptr + shared_ptr + method_set + padding.
401+
//
402+
// TASK-058 step 3 added a lazy Allow-header cache: std::mutex +
403+
// std::string + method_set + bool. std::mutex is a sizeof~64 storage on
404+
// pthread platforms (libc++/macOS) and ~40 on libstdc++/Linux; std::string
405+
// SBO adds ~24-32; the method_set / bool fields slot into existing padding.
406+
// The new ceiling is the old (3*void* + 2*method_set) plus the new cache
407+
// payload, padded for alignment. See test/bench_sizeof_http_resource.cpp
408+
// for the v1-anchored algebra; the value here documents the
409+
// post-TASK-058 layout shape.
334410
static_assert(sizeof(http_resource) <=
335-
sizeof(void*) * 3 + sizeof(method_set) * 2,
411+
sizeof(void*) * 3 + sizeof(method_set) * 2
412+
+ sizeof(std::mutex) + sizeof(std::string)
413+
+ sizeof(method_set) + sizeof(bool) * 8,
336414
"http_resource should be approximately vptr + shared_ptr + "
337-
"method_set after TASK-051");
415+
"method_set (TASK-051) + Allow-header cache "
416+
"(mutex + string + method_set + bool) after TASK-058 step 3");
338417

339418
} // namespace httpserver
340419
#endif // SRC_HTTPSERVER_HTTP_RESOURCE_HPP_

test/Makefile.am

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ LDADD += -lcurl
2626

2727
AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver/ -DHTTPSERVER_COMPILATION
2828
METASOURCES = AUTO
29-
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 auth_skip_normalize 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
29+
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 auth_skip_normalize http_resource_allow_cache 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
3030

3131
MOSTLYCLEANFILES = *.gcda *.gcno *.gcov
3232

@@ -299,6 +299,14 @@ route_lookup_canonicalize_LDADD = $(LDADD) -lmicrohttpd
299299
auth_skip_normalize_SOURCES = unit/auth_skip_normalize_test.cpp
300300
auth_skip_normalize_LDADD = $(LDADD) -lmicrohttpd
301301

302+
# http_resource_allow_cache (TASK-058 step 3): pins the lazy Allow-
303+
# header cache on http_resource -- correctness (cached value matches
304+
# format_allow_header; mask mutation invalidates), identity (two
305+
# consecutive calls return the same buffer pointer, no reallocation),
306+
# and the invalidation surface (set_allowing / disallow_all / allow_all).
307+
http_resource_allow_cache_SOURCES = unit/http_resource_allow_cache_test.cpp
308+
http_resource_allow_cache_LDADD = $(LDADD) -lmicrohttpd
309+
302310
# v2_dispatch_contract: TASK-053. End-to-end safety net pinning the
303311
# observable invariants the dispatch path must satisfy AFTER finalize_answer
304312
# is cut over from resolve_resource_for_request (v1 maps) to

0 commit comments

Comments
 (0)