Skip to content

Commit dd9df4d

Browse files
committed
ci: drop all CCN-gate offenders below the project ceiling
Eight functions tripped the lizard CCN-10 gate on the lint lane. Refactor each one so the parent stays inside the ceiling while preserving the existing behaviour byte-for-byte: * hook_phase::to_string: replace the 12-case switch with a constexpr std::string_view[] lookup keyed on the underlying value. Codegens to the same jump table on modern optimisers; CCN 13 -> 2. * webserver_impl::phase_hook_count: split the 12-case phase fanout into two private helpers (lifecycle / handler), each with 6 arms. CCN 13 -> 3 (parent) + 7 (each helper). * hook_handle::remove: hoist the typed per-phase erase switch into two anonymous-namespace templates (lifecycle / handler) and a thin dispatcher; remove() itself now just builds the erase_and_reset lambda and calls erase_slot_for_phase. CCN 18 -> 5. * webserver_impl::resolve_resource_for_request: extract the single_resource fast-path into a private helper resolve_single_resource_. CCN 11 -> 9. * webserver::install_default_alias_hooks_: extract the auth / method_not_allowed / not_found alias installations into per-handler private helpers. The parent function is now a four-line orchestrator that calls them in order; CCN 12 -> 5. * webserver::on_methods_: extract the four-shape input-validation guard (validate_on_methods_inputs_) and the catch-arm rollback (rollback_on_methods_fresh_entry_). CCN 12 -> 6. * webserver::register_impl_: extract the input-validation guard (validate_register_inputs_) and the catch-arm rollback (rollback_register_). CCN 13 -> 8. * radix_tree::find: extract pop_next_segment_, step_to_child_, and try_consume_exact_terminus_ so the per-segment loop body is three function calls + two cheap branches. CCN 15 -> 9. All eight helpers are private to their owning class (or anonymous- namespace in the .cpp). No public API changed; the only signature movement is the `class http_endpoint` forward-declaration newly visible in webserver.hpp so the two rollback helpers can take it by reference. Local lizard run reports zero offenders under src/.
1 parent e870063 commit dd9df4d

11 files changed

Lines changed: 453 additions & 332 deletions

src/detail/webserver_aliases.cpp

Lines changed: 149 additions & 145 deletions
Large diffs are not rendered by default.

src/detail/webserver_dispatch.cpp

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,28 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
265265
// would also work because single_resource installs a radix prefix at
266266
// "/"). Reading registered_resources avoids the captured-params /
267267
// route_entry plumbing for a configuration that has no parameters.
268+
// single_resource fast path: the one registered endpoint serves every
269+
// URL. Read it directly from registered_resources.
270+
// registered_resources_mutex protects a registration-time data store;
271+
// under dispatch we still need a shared lock to make the read-then-copy
272+
// atomic with respect to a concurrent unregister_path. (single_resource
273+
// webservers do not support runtime register/unregister in practice,
274+
// but the lock is cheap when uncontended and the safety guarantee is
275+
// worth it.) Returns false when no resource is registered yet.
276+
bool webserver_impl::resolve_single_resource_(detail::modded_request* mr,
277+
std::shared_ptr<http_resource>& hrm,
278+
bool need_path_template) {
279+
std::shared_lock registered_resources_lock(registered_resources_mutex);
280+
if (registered_resources.empty()) return false;
281+
const auto& only = *registered_resources.begin();
282+
hrm = only.second;
283+
if (need_path_template) {
284+
mr->matched_path_template = only.first.get_url_complete();
285+
mr->matched_is_prefix = only.first.is_family_url();
286+
}
287+
return true;
288+
}
289+
268290
bool webserver_impl::resolve_resource_for_request(detail::modded_request* mr,
269291
std::shared_ptr<http_resource>& hrm) {
270292
// matched_path_template + matched_is_prefix feed the route_resolved
@@ -274,24 +296,8 @@ bool webserver_impl::resolve_resource_for_request(detail::modded_request* mr,
274296
has_hooks_for(hook_phase::route_resolved) ||
275297
has_hooks_for(hook_phase::before_handler);
276298

277-
// single_resource fast path: the one registered endpoint serves
278-
// every URL. Read it directly from registered_resources.
279-
// registered_resources_mutex protects a registration-time data
280-
// store; under dispatch we still need a shared lock to make the
281-
// read-then-copy atomic with respect to a concurrent
282-
// unregister_path. (NOTE: single_resource webservers do not
283-
// support runtime register/unregister in practice, but the lock
284-
// is cheap when uncontended and the safety guarantee is worth it.)
285299
if (parent->single_resource) {
286-
std::shared_lock registered_resources_lock(registered_resources_mutex);
287-
if (registered_resources.empty()) return false;
288-
const auto& only = *registered_resources.begin();
289-
hrm = only.second;
290-
if (need_path_template) {
291-
mr->matched_path_template = only.first.get_url_complete();
292-
mr->matched_is_prefix = only.first.is_family_url();
293-
}
294-
return true;
300+
return resolve_single_resource_(mr, hrm, need_path_template);
295301
}
296302

297303
// v2 lookup pipeline: cache -> exact -> radix -> regex.

src/detail/webserver_register.cpp

Lines changed: 35 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -101,16 +101,40 @@ using detail::route_tier_result;
101101
// this private helper, which carries the validation and insertion logic.
102102
// Keeping the work in one place prevents drift between the two registration
103103
// kinds.
104-
void webserver::register_impl_(const std::string& resource,
105-
std::shared_ptr<http_resource> res,
106-
bool family) {
104+
// Input-validation guard for register_impl_. Extracted so register_impl_
105+
// stays inside the project-wide CCN gate.
106+
void webserver::validate_register_inputs_(const std::string& resource,
107+
const std::shared_ptr<http_resource>& res, bool family) const {
107108
if (res == nullptr) {
108109
throw std::invalid_argument("The http_resource pointer cannot be null");
109110
}
110-
111111
if (single_resource && ((resource != "" && resource != "/") || !family)) {
112-
throw std::invalid_argument("The resource should be '' or '/' and be registered via register_prefix when using a single_resource server");
112+
throw std::invalid_argument(
113+
"The resource should be '' or '/' and be registered via "
114+
"register_prefix when using a single_resource server");
113115
}
116+
}
117+
118+
// Roll back the v1-side inserts performed by register_impl_ when
119+
// register_v2_route throws. Locks are reacquired briefly for the rollback;
120+
// the registration was visible to readers in the window between, which
121+
// is a harmless read-stale effect (same as the cache-invalidation window).
122+
void webserver::rollback_register_(const detail::http_endpoint& idx,
123+
bool is_plain_path) {
124+
std::unique_lock rollback_lock(impl_->registered_resources_mutex);
125+
impl_->registered_resources.erase(idx);
126+
if (is_plain_path) {
127+
impl_->registered_resources_str.erase(idx.get_url_complete());
128+
}
129+
if (idx.is_regex_compiled()) {
130+
impl_->registered_resources_regex.erase(idx);
131+
}
132+
}
133+
134+
void webserver::register_impl_(const std::string& resource,
135+
std::shared_ptr<http_resource> res,
136+
bool family) {
137+
validate_register_inputs_(resource, res, family);
114138

115139
detail::http_endpoint idx(resource, family, true, regex_checking);
116140

@@ -143,36 +167,16 @@ void webserver::register_impl_(const std::string& resource,
143167
impl_->registered_resources_regex.insert({idx, res});
144168
}
145169
// 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.
152170
registered_resources_lock.unlock();
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.
156-
157-
// TASK-056: register_v2_route may throw std::invalid_argument when a
158-
// prefix-vs-exact terminus collision is detected on the canonical
159-
// path. If it does, undo the v1 inserts above so the table stays
160-
// consistent with the caller's mental model ("the call threw, so
161-
// nothing was registered"). Locks are reacquired briefly for the
162-
// rollback; the registration was visible to readers in the window
163-
// between, which is the same harmless read-stale effect documented
164-
// for the cache invalidation comment above.
171+
172+
// TASK-056: register_v2_route may throw on a prefix-vs-exact terminus
173+
// collision; roll back the v1 inserts above so the table stays
174+
// consistent with the caller's "the call threw, so nothing was
175+
// registered" mental model.
165176
try {
166177
impl_->register_v2_route(idx, std::move(res), family);
167178
} catch (...) {
168-
std::unique_lock rollback_lock(impl_->registered_resources_mutex);
169-
impl_->registered_resources.erase(idx);
170-
if (is_plain_path) {
171-
impl_->registered_resources_str.erase(idx.get_url_complete());
172-
}
173-
if (idx.is_regex_compiled()) {
174-
impl_->registered_resources_regex.erase(idx);
175-
}
179+
rollback_register_(idx, is_plain_path);
176180
throw;
177181
}
178182
impl_->invalidate_route_cache();

src/detail/webserver_routes.cpp

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -343,9 +343,11 @@ void webserver_impl::upsert_v2_table_entry(const detail::http_endpoint& idx,
343343

344344
} // namespace detail
345345

346-
void webserver::on_methods_(method_set methods,
347-
const std::string& path,
348-
std::function<http_response(const http_request&)> handler) {
346+
// Input-validation guard for on_methods_. Extracted so on_methods_ stays
347+
// inside the project-wide CCN gate.
348+
void webserver::validate_on_methods_inputs_(method_set methods,
349+
const std::string& path,
350+
const std::function<http_response(const http_request&)>& handler) const {
349351
if (methods.empty()) {
350352
throw std::invalid_argument(
351353
"route(method_set, ...) requires at least one method bit set");
@@ -382,6 +384,28 @@ void webserver::on_methods_(method_set methods,
382384
throw std::invalid_argument(
383385
"Route path must not contain embedded null bytes");
384386
}
387+
}
388+
389+
// Roll back the v1-side inserts performed by on_methods_ when
390+
// upsert_v2_table_entry throws. Only the fresh-entry case is meaningful
391+
// here -- existing entries are pre-rejected by prepare_or_create_lambda_shim
392+
// before any mutation, so no rollback is needed.
393+
void webserver::rollback_on_methods_fresh_entry_(
394+
const detail::http_endpoint& idx) {
395+
std::unique_lock rollback_lock(impl_->registered_resources_mutex);
396+
impl_->registered_resources.erase(idx);
397+
if (idx.get_url_pars().empty()) {
398+
impl_->registered_resources_str.erase(idx.get_url_complete());
399+
}
400+
if (idx.is_regex_compiled()) {
401+
impl_->registered_resources_regex.erase(idx);
402+
}
403+
}
404+
405+
void webserver::on_methods_(method_set methods,
406+
const std::string& path,
407+
std::function<http_response(const http_request&)> handler) {
408+
validate_on_methods_inputs_(methods, path, handler);
385409

386410
detail::http_endpoint idx(path, /*family=*/false,
387411
/*registration=*/true, regex_checking);
@@ -392,37 +416,17 @@ void webserver::on_methods_(method_set methods,
392416
// Scoped block: registered_resources_mutex is released at the
393417
// closing brace, before upsert_v2_table_entry (which takes its
394418
// own route_table_mutex_) and before invalidate_route_cache
395-
// (which takes the route_lru_cache internal mutex). This
396-
// lock-ordering — registration mutex released before
397-
// table/cache mutexes are acquired — prevents deadlock.
419+
// (which takes the route_lru_cache internal mutex).
398420
std::unique_lock registered_resources_lock(impl_->registered_resources_mutex);
399421
shim = impl_->prepare_or_create_lambda_shim(idx, methods, is_new_entry);
400422
impl_->commit_handlers_to_shim(*shim, methods, std::move(handler));
401423
if (is_new_entry) impl_->insert_fresh_v1_entries(idx, shim);
402-
} // registered_resources_lock released here
403-
404-
// TASK-056: upsert_v2_table_entry may throw std::invalid_argument
405-
// when a prefix-vs-exact terminus collision is detected. Roll back
406-
// the v1 inserts above on throw so the table stays consistent with
407-
// the caller's mental model. The fresh-entry rollback is the only
408-
// case that needs work: for the existing-entry path, on_methods_'s
409-
// prepare_or_create_lambda_shim atomicity pre-check would have
410-
// rejected duplicates BEFORE any mutation, so we never get here.
424+
}
425+
411426
try {
412427
impl_->upsert_v2_table_entry(idx, methods, shim, is_new_entry);
413428
} catch (...) {
414-
if (is_new_entry) {
415-
std::unique_lock rollback_lock(
416-
impl_->registered_resources_mutex);
417-
impl_->registered_resources.erase(idx);
418-
if (idx.get_url_pars().empty()) {
419-
impl_->registered_resources_str.erase(
420-
idx.get_url_complete());
421-
}
422-
if (idx.is_regex_compiled()) {
423-
impl_->registered_resources_regex.erase(idx);
424-
}
425-
}
429+
if (is_new_entry) rollback_on_methods_fresh_entry_(idx);
426430
throw;
427431
}
428432
impl_->invalidate_route_cache();

src/hook_handle.cpp

Lines changed: 80 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,82 @@ static void remove_per_route(std::weak_ptr<detail::resource_hook_table>& weak,
101101
}
102102
}
103103

104+
namespace {
105+
106+
// erase_slot_for_phase: route the (impl, phase, slot_id) tuple to the
107+
// correctly-typed per-phase vector and invoke `erase_and_reset` (a generic
108+
// lambda) on it. Split out of hook_handle::remove so the parent function
109+
// stays under the project-wide CCN gate; the per-phase typed dispatch is
110+
// intrinsically one-arm-per-phase so the CCN cost lives here instead.
111+
// The dispatch is split into two helpers (`_lifecycle` / `_handler`) so
112+
// each helper stays inside the CCN ceiling.
113+
114+
template <class EraseFn>
115+
void erase_slot_for_lifecycle_phase_(detail::webserver_impl* impl,
116+
hook_phase phase,
117+
const EraseFn& erase_and_reset) {
118+
switch (phase) {
119+
case hook_phase::connection_opened:
120+
erase_and_reset(impl->hooks_connection_opened_); break;
121+
case hook_phase::accept_decision:
122+
erase_and_reset(impl->hooks_accept_decision_); break;
123+
case hook_phase::request_received:
124+
erase_and_reset(impl->hooks_request_received_); break;
125+
case hook_phase::body_chunk:
126+
erase_and_reset(impl->hooks_body_chunk_); break;
127+
case hook_phase::route_resolved:
128+
erase_and_reset(impl->hooks_route_resolved_); break;
129+
default:
130+
break;
131+
}
132+
}
133+
134+
template <class EraseFn>
135+
void erase_slot_for_handler_phase_(detail::webserver_impl* impl,
136+
hook_phase phase,
137+
const EraseFn& erase_and_reset) {
138+
switch (phase) {
139+
case hook_phase::before_handler:
140+
erase_and_reset(impl->hooks_before_handler_); break;
141+
case hook_phase::handler_exception:
142+
// Finding #6 (forward-looking): if a future task adds a runtime
143+
// re-registration path for handler_exception_alias_, remove()
144+
// will also need a path to clear that slot and re-evaluate
145+
// any_hooks_. Currently any_hooks_ remains true while the alias
146+
// is wired; removing only a user-vector entry does not clear it
147+
// if the alias is still set. Add that handling alongside the
148+
// runtime setter.
149+
erase_and_reset(impl->hooks_handler_exception_); break;
150+
case hook_phase::after_handler:
151+
erase_and_reset(impl->hooks_after_handler_); break;
152+
case hook_phase::response_sent:
153+
erase_and_reset(impl->hooks_response_sent_); break;
154+
case hook_phase::request_completed:
155+
erase_and_reset(impl->hooks_request_completed_); break;
156+
case hook_phase::connection_closed:
157+
erase_and_reset(impl->hooks_connection_closed_); break;
158+
default:
159+
break;
160+
}
161+
}
162+
163+
template <class EraseFn>
164+
void erase_slot_for_phase(detail::webserver_impl* impl,
165+
hook_phase phase,
166+
const EraseFn& erase_and_reset) {
167+
// The switch arms in the helpers below cannot collapse into a table
168+
// lookup because each phase vector is a differently-typed
169+
// phase_entry<Sig>.
170+
if (static_cast<int>(phase) <=
171+
static_cast<int>(hook_phase::route_resolved)) {
172+
erase_slot_for_lifecycle_phase_(impl, phase, erase_and_reset);
173+
return;
174+
}
175+
erase_slot_for_handler_phase_(impl, phase, erase_and_reset);
176+
}
177+
178+
} // namespace
179+
104180
void hook_handle::remove() noexcept {
105181
if (!armed_) {
106182
return;
@@ -133,19 +209,10 @@ void hook_handle::remove() noexcept {
133209
// practice (single-digit hook counts). A not-found result is the
134210
// idempotent no-op case: the slot was already erased by an earlier
135211
// remove() or never inserted (defensive).
136-
//
137-
// erase_if_found returns true iff the slot was found and erased.
138-
// We use the return value to skip reset_gate_if_empty on a no-op
139-
// erase (the gate value is already consistent -- no erase happened).
140-
// A false return from an armed handle would indicate a bug in
141-
// register_hook_impl (slot never inserted); an assert catches this in
142-
// debug builds. The !armed_ early-return above ensures that a second
143-
// remove() on the same handle never reaches this code path.
144212
// erase_and_reset: linear scan for the slot, erase if found, and
145-
// clear the any_hooks_ gate if the vector becomes empty. The two
146-
// operations stay together so a new phase only adds a one-line
147-
// switch arm. Phase vectors are tiny in practice (single-digit
148-
// hook counts), so linear scan is the right shape here.
213+
// clear the any_hooks_ gate if the vector becomes empty. Phase
214+
// vectors are tiny in practice (single-digit hook counts), so
215+
// linear scan is the right shape here.
149216
auto erase_and_reset = [id, impl, phase](auto& vec) {
150217
for (auto it = vec.begin(); it != vec.end(); ++it) {
151218
if (it->slot_id == id) {
@@ -159,46 +226,7 @@ void hook_handle::remove() noexcept {
159226
}
160227
};
161228

162-
// The switch arms below cannot collapse into a table lookup because
163-
// each phase vector is a differently-typed phase_entry<Sig> — a
164-
// type-erased unification would either need std::visit over a
165-
// tuple-of-vectors or boxing each callable, both of which add
166-
// indirection on the hot path. One arm per phase is the safest
167-
// shape until the phase set stabilises post-TASK-051.
168-
switch (phase) {
169-
case hook_phase::connection_opened:
170-
erase_and_reset(impl->hooks_connection_opened_); break;
171-
case hook_phase::accept_decision:
172-
erase_and_reset(impl->hooks_accept_decision_); break;
173-
case hook_phase::request_received:
174-
erase_and_reset(impl->hooks_request_received_); break;
175-
case hook_phase::body_chunk:
176-
erase_and_reset(impl->hooks_body_chunk_); break;
177-
case hook_phase::route_resolved:
178-
erase_and_reset(impl->hooks_route_resolved_); break;
179-
case hook_phase::before_handler:
180-
erase_and_reset(impl->hooks_before_handler_); break;
181-
case hook_phase::handler_exception:
182-
// Finding #6 (forward-looking): if a future task adds a runtime
183-
// re-registration path for handler_exception_alias_, remove()
184-
// will also need a path to clear that slot and re-evaluate
185-
// any_hooks_. Currently any_hooks_ remains true while the alias
186-
// is wired; removing only a user-vector entry does not clear it
187-
// if the alias is still set. Add that handling alongside the
188-
// runtime setter.
189-
erase_and_reset(impl->hooks_handler_exception_); break;
190-
case hook_phase::after_handler:
191-
erase_and_reset(impl->hooks_after_handler_); break;
192-
case hook_phase::response_sent:
193-
erase_and_reset(impl->hooks_response_sent_); break;
194-
case hook_phase::request_completed:
195-
erase_and_reset(impl->hooks_request_completed_); break;
196-
case hook_phase::connection_closed:
197-
erase_and_reset(impl->hooks_connection_closed_); break;
198-
case hook_phase::count_:
199-
// Unreachable: an armed handle always carries a valid phase.
200-
break;
201-
}
229+
erase_slot_for_phase(impl, phase, erase_and_reset);
202230
}
203231

204232
hook_handle hook_handle::detach() && noexcept {

0 commit comments

Comments
 (0)