Skip to content

Commit 41f5b49

Browse files
etrclaude
andcommitted
TASK-053 step 3: remove v1 fallback + rename route_cache_v2
finalize_answer now unconditionally routes through the v2 resolver (no flag). The v1 resolver body and its four helpers (lookup_route_cache, scan_regex_routes, store_route_cache, apply_extracted_params) are deleted together with the v1 LRU cache (route_cache_list / route_cache_map / route_cache_mutex_) and the regex_route_lookup / regex_route_scan_hit carrier structs. The v2 resolver is renamed in place from resolve_resource_for_request_v2 to resolve_resource_for_request — there is now a single dispatch resolver. invalidate_route_cache() collapses to a single route_lru_cache.clear() since the v1 cache is gone. The lock-order comments in webserver_register.cpp / webserver_routes.cpp are refreshed: the order is now registration mutex (registered_resources_mutex) -> route_table_mutex_ -> route_lru_cache's internal mutex. The route_cache_v2 field is renamed to route_lru_cache, dropping the TODO(Cycle K) marker — this was the rename the v2.0 plan deferred to the dispatch cutover, and the cutover landed here. The class itself stays named `route_cache` (only the field rename is in scope per the plan). The five v1-side registration-time maps (registered_resources / _str / _regex + registered_resources_mutex) are intentionally retained: prepare_or_create_lambda_shim reads registered_resources for lambda+class conflict detection and the WebSocket dispatch path uses registered_resources_mutex. Both are non-dispatch concerns; their removal is its own follow-up task (see TASK-053 §11). The header block comment is updated to make the "registration-only after TASK-053" status explicit. 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 1b97f4d commit 41f5b49

7 files changed

Lines changed: 104 additions & 326 deletions

File tree

src/detail/webserver_dispatch.cpp

Lines changed: 26 additions & 180 deletions
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,10 @@ namespace detail {
9595
// route_resolved and before_handler firing sites landed.
9696

9797
void webserver_impl::invalidate_route_cache() {
98-
// Clear both the v1 and v2 caches. v1's cache is keyed on
99-
// standardized_url only; v2's is keyed on (method, path). A
100-
// registration may invalidate both, so clear both atomically.
101-
{
102-
std::lock_guard<std::mutex> lock(route_cache_mutex_);
103-
route_cache_list.clear();
104-
route_cache_map.clear();
105-
}
106-
route_cache_v2.clear();
98+
// Clear the v2 LRU cache. (The v1 dispatch cache — route_cache_list
99+
// / route_cache_map / route_cache_mutex_ — was removed in TASK-053
100+
// step 3 along with the v1 resolver.)
101+
route_lru_cache.clear();
107102
}
108103

109104
// TASK-027: 3-tier route lookup. Pipeline:
@@ -153,7 +148,7 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
153148
// into a cache_key on every call, including warm-cache hits.
154149
// cache_key is only constructed when an insert is needed (miss path).
155150
cache_value cached;
156-
if (route_cache_v2.find_by_view(method, lookup_path, cached)) {
151+
if (route_lru_cache.find_by_view(method, lookup_path, cached)) {
157152
result.found = true;
158153
result.tier = tier_hit::cache;
159154
result.entry = std::move(cached.entry);
@@ -214,7 +209,7 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
214209
cache_value v;
215210
v.entry = result.entry;
216211
v.captured_params = result.captured_params;
217-
route_cache_v2.insert(key, std::move(v));
212+
route_lru_cache.insert(key, std::move(v));
218213
}
219214

220215
return result;
@@ -224,88 +219,15 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
224219

225220
namespace detail {
226221

227-
std::optional<webserver_impl::regex_route_lookup>
228-
webserver_impl::lookup_route_cache(const std::string& key) {
229-
std::lock_guard<std::mutex> cache_lock(route_cache_mutex_);
230-
auto cache_it = route_cache_map.find(key);
231-
if (cache_it == route_cache_map.end()) {
232-
return std::nullopt;
233-
}
234-
// Cache hit — promote to LRU front and copy the match data while
235-
// still under the cache lock, so a concurrent invalidation can't
236-
// free the cached endpoint out from under us.
237-
// The shared_ptr copy (cached.resource) performs an atomic refcount bump
238-
// while holding route_cache_mutex_ exclusively. This is the hottest
239-
// point in the dispatch path; the atomic op is unavoidable as long as
240-
// callers need the resource to survive a concurrent unregister_resource.
241-
route_cache_list.splice(route_cache_list.begin(), route_cache_list, cache_it->second);
242-
const route_cache_entry& cached = cache_it->second->second;
243-
return regex_route_lookup{
244-
cached.resource,
245-
cached.matched_endpoint.get_url_pars(),
246-
cached.matched_endpoint.get_chunk_positions(),
247-
};
248-
}
249-
250-
std::optional<webserver_impl::regex_route_scan_hit>
251-
webserver_impl::scan_regex_routes(const detail::http_endpoint& target) {
252-
// Longest-match-wins tie-breaking: prefer the endpoint with more
253-
// url-pieces; among equals, prefer the longer url_complete string.
254-
bool found = false;
255-
size_t len = 0;
256-
size_t tot_len = 0;
257-
map<detail::http_endpoint, std::shared_ptr<http_resource>>::iterator found_endpoint;
258-
for (auto it = registered_resources_regex.begin();
259-
it != registered_resources_regex.end(); ++it) {
260-
size_t endpoint_pieces_len = it->first.get_url_pieces().size();
261-
size_t endpoint_tot_len = it->first.get_url_complete().size();
262-
if (found && endpoint_pieces_len <= len
263-
&& !(endpoint_pieces_len == len && endpoint_tot_len > tot_len)) {
264-
continue;
265-
}
266-
if (!it->first.match(target)) continue;
267-
found = true;
268-
len = endpoint_pieces_len;
269-
tot_len = endpoint_tot_len;
270-
found_endpoint = it;
271-
}
272-
if (!found) return std::nullopt;
273-
return regex_route_scan_hit{found_endpoint->first, found_endpoint->second};
274-
}
275-
276-
void webserver_impl::store_route_cache(const std::string& key,
277-
const detail::http_endpoint& matched,
278-
std::shared_ptr<http_resource> hrm) {
279-
std::lock_guard<std::mutex> cache_lock(route_cache_mutex_);
280-
route_cache_list.emplace_front(key, route_cache_entry{matched, std::move(hrm)});
281-
route_cache_map[key] = route_cache_list.begin();
282-
if (route_cache_map.size() > ROUTE_CACHE_MAX_SIZE) {
283-
route_cache_map.erase(route_cache_list.back().first);
284-
route_cache_list.pop_back();
285-
}
286-
}
287-
288-
void webserver_impl::apply_extracted_params(detail::modded_request* mr,
289-
const detail::http_endpoint& target,
290-
const std::vector<std::string>& url_pars,
291-
const std::vector<int>& chunks) {
292-
const auto& url_pieces = target.get_url_pieces();
293-
for (unsigned int i = 0; i < url_pars.size(); i++) {
294-
if (chunks[i] < 0 || static_cast<size_t>(chunks[i]) >= url_pieces.size()) continue;
295-
mr->dhr->set_arg(url_pars[i], url_pieces[chunks[i]]);
296-
}
297-
}
298-
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.
222+
// TASK-053: v2 lookup-backed resolver. Routes finalize_answer through
223+
// lookup_v2 (the 3-tier v2 table fronted by route_lru_cache) — the v1
224+
// resolver and its helpers (lookup_route_cache, scan_regex_routes,
225+
// store_route_cache, apply_extracted_params) were deleted in TASK-053
226+
// step 3. See header comment in webserver_impl_dispatch.hpp for the
227+
// full contract.
303228
//
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_ ->
229+
// The dispatch path now does:
230+
// lookup_v2() walks route_lru_cache -> exact_routes_ ->
309231
// param_and_prefix_routes_ -> regex_routes_, returning a
310232
// route_entry whose handler arm is always shared_ptr<http_resource>
311233
// (the on_*/route entry points wrap user lambdas in lambda_resource
@@ -317,22 +239,22 @@ void webserver_impl::apply_extracted_params(detail::modded_request* mr,
317239
// would also work because single_resource installs a radix prefix at
318240
// "/"). Reading registered_resources avoids the captured-params /
319241
// route_entry plumbing for a configuration that has no parameters.
320-
bool webserver_impl::resolve_resource_for_request_v2(detail::modded_request* mr,
242+
bool webserver_impl::resolve_resource_for_request(detail::modded_request* mr,
321243
std::shared_ptr<http_resource>& hrm) {
322244
// matched_path_template + matched_is_prefix feed the route_resolved
323245
// and before_handler hook ctxs. Skip the heap allocation when no
324-
// hook in either phase is registered. Mirrors the v1 resolver.
246+
// hook in either phase is registered.
325247
const bool need_path_template =
326248
has_hooks_for(hook_phase::route_resolved) ||
327249
has_hooks_for(hook_phase::before_handler);
328250

329251
// 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
252+
// every URL. Read it directly from registered_resources.
253+
// registered_resources_mutex protects a registration-time data
254+
// store; under dispatch we still need a shared lock to make the
255+
// read-then-copy atomic with respect to a concurrent
256+
// unregister_path. (NOTE: single_resource webservers do not
257+
// support runtime register/unregister in practice, but the lock
336258
// is cheap when uncontended and the safety guarantee is worth it.)
337259
if (parent->single_resource) {
338260
std::shared_lock registered_resources_lock(registered_resources_mutex);
@@ -366,9 +288,9 @@ bool webserver_impl::resolve_resource_for_request_v2(detail::modded_request* mr,
366288
hrm = *hp;
367289

368290
// 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.
291+
// equivalent of the v1-era apply_extracted_params (removed in
292+
// TASK-053 step 3). Per-name set_arg matches v1 behaviour exactly —
293+
// duplicates with later wins, etc.
372294
if (mr->dhr != nullptr) {
373295
for (const auto& [name, value] : result.captured_params) {
374296
mr->dhr->set_arg(name, value);
@@ -378,90 +300,14 @@ bool webserver_impl::resolve_resource_for_request_v2(detail::modded_request* mr,
378300
// Populate the hook ctx scratch slots when at least one hook is
379301
// registered for the phases that read them. v2 cache_value does
380302
// 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).
303+
// (matches the v1 cache-hit path the legacy resolver used).
383304
if (need_path_template) {
384305
mr->matched_path_template = mr->standardized_url;
385306
mr->matched_is_prefix = result.entry.is_prefix;
386307
}
387308
return true;
388309
}
389310

390-
bool webserver_impl::resolve_resource_for_request(detail::modded_request* mr,
391-
std::shared_ptr<http_resource>& hrm) {
392-
// TASK-048 perf: matched_path_template is only consumed by
393-
// fire_route_resolved_gated and the before_handler firing site, both
394-
// of which are gated by their respective any_hooks_ entries. Skip the
395-
// heap allocation on every matched request when no hooks are registered.
396-
const bool need_path_template =
397-
has_hooks_for(hook_phase::route_resolved) ||
398-
has_hooks_for(hook_phase::before_handler);
399-
400-
std::shared_lock registered_resources_lock(registered_resources_mutex);
401-
402-
if (parent->single_resource) {
403-
hrm = registered_resources.begin()->second;
404-
// single_resource: the one registered endpoint serves every URL.
405-
// Capture its key for the route_resolved/before_handler hook ctx.
406-
if (need_path_template) {
407-
const auto& only = *registered_resources.begin();
408-
mr->matched_path_template = only.first.get_url_complete();
409-
mr->matched_is_prefix = only.first.is_family_url();
410-
}
411-
return true;
412-
}
413-
414-
auto exact_it = registered_resources_str.find(mr->standardized_url.c_str());
415-
if (exact_it != registered_resources_str.end()) {
416-
// Copy the shared_ptr (atomic refcount bump) while the shared_lock
417-
// on registered_resources_mutex is still held. The copy extends the
418-
// resource's lifetime past a concurrent unregister_path that might
419-
// erase this slot — without the copy, dispatch_resource_handler
420-
// could hold a dangling pointer. The single atomic op is the
421-
// unavoidable cost of the use-after-unregister safety guarantee.
422-
hrm = exact_it->second;
423-
// Exact-match: the registration key equals the standardized URL.
424-
// Copy into modded_request so the hook context's string_view is
425-
// safe across hook calls even if a concurrent unregister_path
426-
// erases the slot.
427-
if (need_path_template) {
428-
mr->matched_path_template = exact_it->first;
429-
mr->matched_is_prefix = false;
430-
}
431-
return true;
432-
}
433-
434-
if (!parent->regex_checking) return false;
435-
436-
detail::http_endpoint endpoint(mr->standardized_url.c_str(), false, false, false);
437-
438-
if (auto cached = lookup_route_cache(mr->standardized_url)) {
439-
hrm = cached->hrm;
440-
apply_extracted_params(mr, endpoint, cached->url_pars, cached->chunks);
441-
if (need_path_template) {
442-
// Cache layer dropped the matched endpoint at its API boundary;
443-
// fall back to the requested URL as a stable approximation of
444-
// the path_template (used by the route_resolved hook ctx only).
445-
mr->matched_path_template = mr->standardized_url;
446-
mr->matched_is_prefix = false;
447-
}
448-
return true;
449-
}
450-
451-
auto scan_hit = scan_regex_routes(endpoint);
452-
if (!scan_hit) return false;
453-
454-
hrm = scan_hit->hrm;
455-
store_route_cache(mr->standardized_url, scan_hit->endpoint, hrm);
456-
apply_extracted_params(mr, endpoint, scan_hit->endpoint.get_url_pars(),
457-
scan_hit->endpoint.get_chunk_positions());
458-
if (need_path_template) {
459-
mr->matched_path_template = scan_hit->endpoint.get_url_complete();
460-
mr->matched_is_prefix = scan_hit->endpoint.is_family_url();
461-
}
462-
return true;
463-
}
464-
465311
std::string webserver_impl::serialize_allow_methods(method_set allowed) const {
466312
// TASK-048 review cleanup (findings 3 & 4): delegate to the shared free
467313
// function format_allow_header in detail/method_utils.hpp. The member

src/detail/webserver_register.cpp

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -142,18 +142,17 @@ void webserver::register_impl_(const std::string& resource,
142142
// shared_ptr copies are cheaper than the regex scan they help avoid.
143143
impl_->registered_resources_regex.insert({idx, res});
144144
}
145-
// Release the write lock before invalidating the cache.
146-
// invalidate_route_cache() acquires route_cache_mutex_ independently;
147-
// holding registered_resources_mutex across that call would invert the
148-
// lock order documented in webserver_impl.hpp (cache mutex is always
149-
// acquired AFTER table mutex, never while table mutex is held).
145+
// Release the registration mutex before clearing the LRU cache.
146+
// route_lru_cache.clear() takes the cache's internal mutex; the
147+
// lock order documented in webserver_impl.hpp acquires the table
148+
// mutex BEFORE the cache mutex and never both at once, so the
149+
// registration mutex (which gates the registration-side maps,
150+
// distinct from the v2 table mutex) is also released first as a
151+
// matter of discipline.
150152
registered_resources_lock.unlock();
151-
// Unlocking before invalidate_route_cache() is intentional: it preserves
152-
// lock-order discipline (registered_resources_mutex released before
153-
// route_cache_mutex_ is acquired inside invalidate_route_cache).
154-
// A reader can transiently see the new entry without a warm cache, causing
155-
// one extra regex scan on the first hit. This is a harmless read-stale
156-
// effect — the resource is already visible to readers under the shared lock.
153+
// A reader can transiently see the new entry without a warm cache,
154+
// causing one extra tier walk on the first hit — harmless read-stale
155+
// effect: the resource is already visible under the shared lock.
157156

158157
impl_->register_v2_route(idx, std::move(res), family);
159158
impl_->invalidate_route_cache();
@@ -236,11 +235,9 @@ void webserver::unregister_impl_(const string& resource, bool family) {
236235

237236
// TASK-027: mirror the erasure into the v2 3-tier table.
238237
// Lock ordering: registered_resources_mutex (released above) ->
239-
// route_table_mutex_ -> route_cache_mutex_ (inside invalidate_route_cache).
240-
// Do not acquire route_cache_mutex_ without holding route_table_mutex_ or
241-
// registered_resources_mutex first.
242-
// This matches the pattern used in register_impl_ / on_methods_:
243-
// table lock released before cache is cleared.
238+
// route_table_mutex_ -> route_lru_cache's internal mutex
239+
// (inside invalidate_route_cache). Table lock released before the
240+
// LRU cache is cleared, matching register_impl_ / on_methods_.
244241
{
245242
std::unique_lock table_lock(impl_->route_table_mutex_);
246243
const std::string& key = he.get_url_complete();

src/detail/webserver_request.cpp

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -350,12 +350,7 @@ 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-
// 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);
353+
bool found = resolve_resource_for_request(mr, hrm);
359354

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

src/detail/webserver_routes.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -334,10 +334,11 @@ void webserver::on_methods_(method_set methods,
334334
std::shared_ptr<detail::lambda_resource> shim;
335335
{
336336
// Scoped block: registered_resources_mutex is released at the
337-
// closing brace, before upsert_v2_table_entry (which takes its own
338-
// route_table_mutex_) and before invalidate_route_cache (which takes
339-
// route_cache_mutex_). This lock-ordering — v1 mutex released before
340-
// v2/cache mutexes are acquired — prevents deadlock.
337+
// closing brace, before upsert_v2_table_entry (which takes its
338+
// own route_table_mutex_) and before invalidate_route_cache
339+
// (which takes the route_lru_cache internal mutex). This
340+
// lock-ordering — registration mutex released before
341+
// table/cache mutexes are acquired — prevents deadlock.
341342
std::unique_lock registered_resources_lock(impl_->registered_resources_mutex);
342343
shim = impl_->prepare_or_create_lambda_shim(idx, methods, is_new_entry);
343344
impl_->commit_handlers_to_shim(*shim, methods, std::move(handler));

src/httpserver/detail/route_cache.hpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,12 @@
2626
// the key so a path served by both on_get and on_post warms two distinct
2727
// cache entries.
2828
//
29-
// Concurrency: the cache uses its own plain std::mutex (route_cache_mutex_
30-
// in webserver_impl) — *not* a shared_mutex — because every cache touch,
29+
// Concurrency: the cache uses its own plain std::mutex (an internal
30+
// member of route_cache itself, owned by `route_lru_cache` on
31+
// webserver_impl) — *not* a shared_mutex — because every cache touch,
3132
// including the LRU promotion on a hit, is a write (std::list::splice).
3233
// Lock-order discipline: route_table_mutex_ is always acquired before
33-
// route_cache_mutex_ when both are held.
34+
// the cache's internal mutex when both are held.
3435
//
3536
// Internal header — only reachable when compiling libhttpserver.
3637
#if !defined(HTTPSERVER_COMPILATION)

0 commit comments

Comments
 (0)