Skip to content

Commit 1b97f4d

Browse files
etrclaude
andcommitted
TASK-053 step 2: cut dispatch over to lookup_v2 behind flag
Adds resolve_resource_for_request_v2 alongside the v1 resolver and routes finalize_answer through it by default (use_lookup_v2_ flag, true by default; transitional — removed in step 3). The new resolver: * Preserves the parent->single_resource fast path verbatim (it reads registered_resources, which v1 also did — that data store is orthogonal to the route-table tier choice). * Otherwise delegates to lookup_v2 (cache -> exact -> radix -> regex), extracts the shared_ptr<http_resource> arm of route_entry::handler, replays captured_params into mr->dhr via set_arg, and populates matched_path_template / matched_is_prefix for the hook ctx (gated on need_path_template like the v1 resolver). Two correctness fixes wired in alongside the cutover: 1. lookup_v2 was moving result.entry into the cache_value before returning result to the caller. Pre-TASK-053 no caller read result.entry.handler (lookup_pipeline_test only checks .found / .tier), so the bug was latent. Once the resolver started calling std::get_if<shared_ptr<...>> on the returned entry, every dispatch saw a moved-from variant and 404'd. Copy on insert instead — one shared_ptr ref-count bump and one captures-vector copy on the cache-miss path, no effect on cache hits. 2. route_cache::find_by_view crashed on a fresh cache (bucket_count() == 0 on libc++; cbegin(0)/cend(0) dereferences a null bucket-list pointer). Pre-TASK-053 this path was test-only, so the UB was dormant; the dispatch cutover puts find_by_view on the request hot path, so the first request against a fresh server reliably reproduced the crash. Early-out when bucket_count() == 0. All v2_dispatch_contract, routing_regression, lookup_pipeline, route_table_concurrency, hooks_route_resolved_miss_and_hit, webserver_register_path_prefix, webserver_on_methods, webserver_route suites pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 24f3f9c commit 1b97f4d

5 files changed

Lines changed: 144 additions & 8 deletions

File tree

src/detail/webserver_dispatch.cpp

Lines changed: 100 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -203,14 +203,17 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
203203
}
204204
} // table_lock released.
205205

206-
// Step 3: install into cache (cache mutex only). Move result.entry and
207-
// result.captured_params into the cache_value to avoid a second copy
208-
// of the shared_ptr ref-count bump and captured vector on the miss path.
209-
// result is not used after this point.
206+
// Step 3: install into cache (cache mutex only). Copy result.entry and
207+
// result.captured_params into the cache_value — the caller (the v2
208+
// resolver) consumes `result` after this returns, so a move-out of
209+
// `result.entry` here would leave the caller's `std::get_if` reading a
210+
// moved-from variant (the shared_ptr arm is null after a move, causing
211+
// a false-negative 404). The copy is one shared_ptr ref-count bump and
212+
// one std::vector copy on the miss path — cache hits are unaffected.
210213
if (result.found) {
211214
cache_value v;
212-
v.entry = std::move(result.entry);
213-
v.captured_params = std::move(result.captured_params);
215+
v.entry = result.entry;
216+
v.captured_params = result.captured_params;
214217
route_cache_v2.insert(key, std::move(v));
215218
}
216219

@@ -293,6 +296,97 @@ void webserver_impl::apply_extracted_params(detail::modded_request* mr,
293296
}
294297
}
295298

299+
// TASK-053 step 2: v2 lookup-backed resolver. Routes finalize_answer
300+
// through lookup_v2 (the 3-tier v2 table fronted by route_cache_v2)
301+
// instead of the v1 registered_resources* maps. See header comment in
302+
// webserver_impl_dispatch.hpp for the full contract.
303+
//
304+
// The dispatch path used to do:
305+
// v1: hash registered_resources_str -> walk registered_resources_regex
306+
// (with LRU front-end route_cache_list / route_cache_map).
307+
// This path now does:
308+
// v2: lookup_v2() walks route_cache_v2 -> exact_routes_ ->
309+
// param_and_prefix_routes_ -> regex_routes_, returning a
310+
// route_entry whose handler arm is always shared_ptr<http_resource>
311+
// (the on_*/route entry points wrap user lambdas in lambda_resource
312+
// before storing; the variant's lambda_handler arm is dead code).
313+
//
314+
// The parent->single_resource fast path is intentionally preserved
315+
// here: it reads the single registered endpoint directly from
316+
// registered_resources rather than falling through to lookup_v2 (which
317+
// would also work because single_resource installs a radix prefix at
318+
// "/"). Reading registered_resources avoids the captured-params /
319+
// route_entry plumbing for a configuration that has no parameters.
320+
bool webserver_impl::resolve_resource_for_request_v2(detail::modded_request* mr,
321+
std::shared_ptr<http_resource>& hrm) {
322+
// matched_path_template + matched_is_prefix feed the route_resolved
323+
// and before_handler hook ctxs. Skip the heap allocation when no
324+
// hook in either phase is registered. Mirrors the v1 resolver.
325+
const bool need_path_template =
326+
has_hooks_for(hook_phase::route_resolved) ||
327+
has_hooks_for(hook_phase::before_handler);
328+
329+
// single_resource fast path: the one registered endpoint serves
330+
// every URL. Read it directly from registered_resources to mirror
331+
// the v1 resolver's behaviour. registered_resources_mutex protects
332+
// a registration-time data store; under dispatch we still need a
333+
// shared lock to make the read-then-copy atomic with respect to a
334+
// concurrent unregister_path. (NOTE: single_resource webservers do
335+
// not support runtime register/unregister in practice, but the lock
336+
// is cheap when uncontended and the safety guarantee is worth it.)
337+
if (parent->single_resource) {
338+
std::shared_lock registered_resources_lock(registered_resources_mutex);
339+
if (registered_resources.empty()) return false;
340+
const auto& only = *registered_resources.begin();
341+
hrm = only.second;
342+
if (need_path_template) {
343+
mr->matched_path_template = only.first.get_url_complete();
344+
mr->matched_is_prefix = only.first.is_family_url();
345+
}
346+
return true;
347+
}
348+
349+
// v2 lookup pipeline: cache -> exact -> radix -> regex.
350+
lookup_result result = lookup_v2(mr->method_enum, mr->standardized_url);
351+
if (!result.found) return false;
352+
353+
// Extract the shared_ptr<http_resource> arm of the route_entry's
354+
// variant. The on_* / route entry points wrap user lambdas in a
355+
// lambda_resource shim and store the shim as shared_ptr<http_resource>,
356+
// so this arm is the only one populated in practice; treat the
357+
// lambda_handler arm as unreachable (defensive nullptr check below).
358+
auto* hp = std::get_if<std::shared_ptr<http_resource>>(&result.entry.handler);
359+
if (hp == nullptr || *hp == nullptr) {
360+
// Unreachable today: lookup_v2 only returns entries whose
361+
// handler is a shared_ptr<http_resource>. If a future task
362+
// stores a lambda_handler directly we will need to grow this
363+
// path; until then, treat as a not-found miss.
364+
return false;
365+
}
366+
hrm = *hp;
367+
368+
// Replay captured URL parameters into the request. This is the v2
369+
// equivalent of apply_extracted_params (which the v1 resolver calls
370+
// on regex-tier hits). Per-name set_arg matches the v1 behaviour
371+
// exactly — duplicates with later wins, etc.
372+
if (mr->dhr != nullptr) {
373+
for (const auto& [name, value] : result.captured_params) {
374+
mr->dhr->set_arg(name, value);
375+
}
376+
}
377+
378+
// Populate the hook ctx scratch slots when at least one hook is
379+
// registered for the phases that read them. v2 cache_value does
380+
// not store the matched URL template; fall back to standardized_url
381+
// (matches the v1 cache-hit path; see resolve_resource_for_request
382+
// ll. 348-353).
383+
if (need_path_template) {
384+
mr->matched_path_template = mr->standardized_url;
385+
mr->matched_is_prefix = result.entry.is_prefix;
386+
}
387+
return true;
388+
}
389+
296390
bool webserver_impl::resolve_resource_for_request(detail::modded_request* mr,
297391
std::shared_ptr<http_resource>& hrm) {
298392
// TASK-048 perf: matched_path_template is only consumed by

src/detail/webserver_request.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,12 @@ MHD_Result webserver_impl::finalize_answer(MHD_Connection* connection,
350350
// alive until our local shared_ptr drops at the end of
351351
// finalize_answer.
352352
std::shared_ptr<http_resource> hrm;
353-
bool found = resolve_resource_for_request(mr, hrm);
353+
// TASK-053 step 2: route through the v2 3-tier table (default) or
354+
// the legacy v1 maps (use_lookup_v2_=false). Step 3 deletes the
355+
// branch and the v1 resolver body together.
356+
bool found = use_lookup_v2_
357+
? resolve_resource_for_request_v2(mr, hrm)
358+
: resolve_resource_for_request(mr, hrm);
354359

355360
// TASK-051: capture the resolved resource on the request so the
356361
// tail-end firing helpers (fire_response_sent_gated /

src/httpserver/detail/route_cache.hpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,16 @@ class route_cache {
138138
// Walk the bucket manually via equal_range on the raw bucket index.
139139
// unordered_map::bucket() + bucket_begin()/bucket_end() lets us
140140
// scan the correct bucket without constructing a full cache_key.
141-
std::size_t b = map_.bucket_count() ? bucket_hash % map_.bucket_count() : 0;
141+
// TASK-053: empty-cache early-out. On libc++ a default-constructed
142+
// unordered_map has bucket_count() == 0 and calling cbegin(0) /
143+
// cend(0) dereferences a null bucket-list pointer (UB). Before
144+
// TASK-053 this path was only hit by tests; the TASK-053 dispatch
145+
// cutover puts find_by_view on the request hot path, so the first
146+
// request against a fresh server reliably reproduced the crash.
147+
if (map_.bucket_count() == 0) {
148+
return false;
149+
}
150+
std::size_t b = bucket_hash % map_.bucket_count();
142151
for (auto it = map_.cbegin(b), end = map_.cend(b); it != end; ++it) {
143152
if (it->first.method == method && it->first.path == path) {
144153
// Promote: splice requires a mutable iterator from the main map.

src/httpserver/detail/webserver_impl.hpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,16 @@ class webserver_impl {
229229
// TASK-027 LRU front-end. 256 entries per architecture spec.
230230
route_cache route_cache_v2{ROUTE_CACHE_MAX_SIZE};
231231

232+
// TASK-053 step 2: transitional flag controlling whether
233+
// finalize_answer routes its lookup through the v2 3-tier table
234+
// (resolve_resource_for_request_v2 -> lookup_v2) or the legacy v1
235+
// maps (resolve_resource_for_request). Default: v2.
236+
//
237+
// Step 3 of TASK-053 deletes this flag and the v1 resolver body in
238+
// a separate commit so the diff stays reviewable in isolation. The
239+
// flag exists only across step 2 / step 3.
240+
bool use_lookup_v2_ = true;
241+
232242
// tier_hit identifies which tier of the v2 route table answered a
233243
// lookup. Returned alongside the route_entry copy from lookup_v2()
234244
// so the dispatch site (and tests) can pin the lookup pipeline.

src/httpserver/detail/webserver_impl_dispatch.hpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,24 @@ struct regex_route_lookup {
299299
bool resolve_resource_for_request(modded_request* mr,
300300
std::shared_ptr<::httpserver::http_resource>& hrm);
301301

302+
// TASK-053 step 2: v2 lookup-backed resolver. Same observable
303+
// contract as resolve_resource_for_request (returns true + sets
304+
// @p hrm on hit, sets @p mr->matched_path_template + matched_is_prefix
305+
// when the hook ctx needs them; sets URL captures via mr->dhr->set_arg
306+
// for parameterized routes), but consults lookup_v2() and the v2 3-tier
307+
// route table (exact_routes_ / param_and_prefix_routes_ / regex_routes_)
308+
// fronted by route_cache_v2 instead of the v1 maps (registered_resources*
309+
// + route_cache_list). Selected over the v1 path at finalize_answer
310+
// per the use_lookup_v2_ flag on webserver_impl (default: true).
311+
//
312+
// Note: the parent->single_resource fast path is preserved here exactly
313+
// as in the v1 resolver — it reads registered_resources directly because
314+
// the configuration shape (one registered endpoint serves every URL) is
315+
// orthogonal to the route-table tier choice. (Folding it into lookup_v2
316+
// itself is cleaner long-term but would balloon the diff; deferred.)
317+
bool resolve_resource_for_request_v2(modded_request* mr,
318+
std::shared_ptr<::httpserver::http_resource>& hrm);
319+
302320
// LRU cache hit path: returns the cached match (hrm + url_pars +
303321
// chunks) and promotes the entry to the front of the list. Caller
304322
// must hold registered_resources_mutex (shared).

0 commit comments

Comments
 (0)