Skip to content

Commit 5140308

Browse files
etrclaude
andcommitted
refactor: close collaborator duplication seams post-decomposition
Tidy the orchestrator/collaborator boundaries flagged by validation: - daemon_lifecycle gains handle(), a noexcept acquire-load accessor; the nine webserver run/info methods in webserver_lifecycle.cpp call it instead of repeating daemon_.daemon.load(memory_order_acquire), centralizing the memory-order contract in one place. - route_table gains erase_exact_and_regex_locked_() and remove_param_prefix_locked_(); unregister_impl_ and unregister_resource call them instead of poking route_table's raw tier containers and open-coding the regex remove_if sweep twice. Drops the now-unused <algorithm>/<regex> includes from webserver_register.cpp. - hooks.md: repoint the four fire_*_gated phase-table entries at webserver_hook_firing.cpp (renamed from webserver_finalize.cpp in commit 2e73dcb). Behavior-preserving; 113/113 pass. Also records the validation run's unworked-findings ledger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a5bf116 commit 5140308

7 files changed

Lines changed: 136 additions & 36 deletions

File tree

specs/architecture/04-components/hooks.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,11 @@
1111
| `request_received` | `webserver_body_pipeline.cpp:requests_answer_first_step` — post header-parse, pre body-read | **yes** | no (route not yet known) |
1212
| `body_chunk` | `webserver_body_pipeline.cpp:requests_answer_second_step` — per chunk | **yes** | no |
1313
| `route_resolved` | `webserver_request.cpp:fire_route_resolved_gated` — called from `webserver_request.cpp:finalize_answer`, after lookup | no | n/a (boundary phase) |
14-
| `before_handler` | `webserver_finalize.cpp:fire_before_handler_gated` — called from `webserver_request.cpp:finalize_answer`, before dispatch | **yes** | yes |
14+
| `before_handler` | `webserver_hook_firing.cpp:fire_before_handler_gated` — called from `webserver_request.cpp:finalize_answer`, before dispatch | **yes** | yes |
1515
| `handler_exception` | `webserver_dispatch.cpp:dispatch_resource_handler` — each catch arm | **yes** (maps exception to response) | yes |
16-
| `after_handler` | `webserver_finalize.cpp:fire_after_handler_gated` — called from `webserver_request.cpp:finalize_answer` | **yes** (replaces response) | yes |
17-
| `response_sent` | `webserver_finalize.cpp:fire_response_sent_gated` — called from `webserver_request.cpp:materialize_and_queue_response`, post `MHD_queue_response` | no | yes |
18-
| `request_completed` | `webserver_finalize.cpp:fire_request_completed_gated` — called from `webserver_callbacks.cpp:request_completed`, NOTIFY_COMPLETED | no | yes |
16+
| `after_handler` | `webserver_hook_firing.cpp:fire_after_handler_gated` — called from `webserver_request.cpp:finalize_answer` | **yes** (replaces response) | yes |
17+
| `response_sent` | `webserver_hook_firing.cpp:fire_response_sent_gated` — called from `webserver_request.cpp:materialize_and_queue_response`, post `MHD_queue_response` | no | yes |
18+
| `request_completed` | `webserver_hook_firing.cpp:fire_request_completed_gated` — called from `webserver_callbacks.cpp:request_completed`, NOTIFY_COMPLETED | no | yes |
1919
| `connection_closed` | `detail/webserver_callbacks_lifecycle.cpp:connection_notify` — NOTIFY_CLOSED | no | no |
2020

2121
**Implementation.** Each phase has its own `std::vector<std::function<...>>` in `webserver_impl`, guarded by a single `std::shared_mutex hook_table_mutex_`. A per-phase `std::atomic<bool> any_hooks_[hook_phase::count_]` flag short-circuits the dispatch site to a relaxed atomic load and a compare-with-zero when no subscribers exist — the only hook-related cost on the hot request path for a server with zero hooks registered.

specs/unworked_review_issues/2026-07-17_214922_manual-validation.md

Lines changed: 77 additions & 0 deletions
Large diffs are not rendered by default.

src/detail/route_table.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
#include "httpserver/detail/route_table.hpp"
3030

31+
#include <algorithm>
3132
#include <cassert>
3233
#include <memory>
3334
#include <mutex>
@@ -525,5 +526,20 @@ void route_table::register_v2_route(const http_endpoint& idx,
525526
}
526527
}
527528

529+
void route_table::erase_exact_and_regex_locked_(const std::string& key) {
530+
exact_routes_.erase(key);
531+
regex_routes_.erase(
532+
std::remove_if(regex_routes_.begin(), regex_routes_.end(),
533+
[&key](const regex_route& rr) {
534+
return rr.url_complete == key;
535+
}),
536+
regex_routes_.end());
537+
}
538+
539+
void route_table::remove_param_prefix_locked_(const std::string& key,
540+
bool is_prefix) {
541+
param_and_prefix_routes_.remove(key, is_prefix);
542+
}
543+
528544
} // namespace detail
529545
} // namespace httpserver

src/detail/webserver_lifecycle.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -193,44 +193,44 @@ bool webserver::stop() {
193193
}
194194

195195
int webserver::quiesce() {
196-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
196+
struct MHD_Daemon* d = impl_->daemon_.handle();
197197
if (d == nullptr) return -1;
198198
MHD_socket fd = MHD_quiesce_daemon(d);
199199
return static_cast<int>(fd);
200200
}
201201

202202
int webserver::get_listen_fd() const {
203-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
203+
struct MHD_Daemon* d = impl_->daemon_.handle();
204204
if (d == nullptr) return -1;
205205
const union MHD_DaemonInfo* info = MHD_get_daemon_info(d, MHD_DAEMON_INFO_LISTEN_FD);
206206
if (info == nullptr) return -1;
207207
return static_cast<int>(info->listen_fd);
208208
}
209209

210210
unsigned int webserver::get_active_connections() const {
211-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
211+
struct MHD_Daemon* d = impl_->daemon_.handle();
212212
if (d == nullptr) return 0;
213213
const union MHD_DaemonInfo* info = MHD_get_daemon_info(d, MHD_DAEMON_INFO_CURRENT_CONNECTIONS);
214214
if (info == nullptr) return 0;
215215
return info->num_connections;
216216
}
217217

218218
uint16_t webserver::get_bound_port() const {
219-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
219+
struct MHD_Daemon* d = impl_->daemon_.handle();
220220
if (d == nullptr) return 0;
221221
const union MHD_DaemonInfo* info = MHD_get_daemon_info(d, MHD_DAEMON_INFO_BIND_PORT);
222222
if (info == nullptr) return 0;
223223
return info->port;
224224
}
225225

226226
bool webserver::run() {
227-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
227+
struct MHD_Daemon* d = impl_->daemon_.handle();
228228
if (d == nullptr) return false;
229229
return MHD_run(d) == MHD_YES;
230230
}
231231

232232
bool webserver::run_wait(int32_t millisec) {
233-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
233+
struct MHD_Daemon* d = impl_->daemon_.handle();
234234
if (d == nullptr) return false;
235235
return MHD_run_wait(d, millisec) == MHD_YES;
236236
}
@@ -241,7 +241,7 @@ bool webserver::get_fdset(fd_set* read_fd_set, fd_set* write_fd_set,
241241
// restoring compile-time type safety. The public header pulls in
242242
// <sys/select.h> / <winsock2.h> directly because fd_set is a typedef
243243
// and cannot be portably forward-declared.
244-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
244+
struct MHD_Daemon* d = impl_->daemon_.handle();
245245
if (d == nullptr) return false;
246246
MHD_socket mhd_max_fd = 0;
247247
if (MHD_get_fdset(d,
@@ -256,7 +256,7 @@ bool webserver::get_fdset(fd_set* read_fd_set, fd_set* write_fd_set,
256256
}
257257

258258
bool webserver::get_timeout(uint64_t* timeout) {
259-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
259+
struct MHD_Daemon* d = impl_->daemon_.handle();
260260
if (d == nullptr) return false;
261261
MHD_UNSIGNED_LONG_LONG mhd_timeout = 0;
262262
if (MHD_get_timeout(d, &mhd_timeout) != MHD_YES) {
@@ -272,7 +272,7 @@ bool webserver::add_connection(int client_socket, const struct sockaddr* addr, u
272272
// POSIX guarantees `socklen_t` is an unsigned integer of at least 32
273273
// bits; `unsigned int` matches on every supported platform.
274274
// (The sizeof static_assert is at file scope above.)
275-
struct MHD_Daemon* d = impl_->daemon_.daemon.load(std::memory_order_acquire);
275+
struct MHD_Daemon* d = impl_->daemon_.handle();
276276
if (d == nullptr) return false;
277277
return MHD_add_connection(d, client_socket, addr,
278278
static_cast<socklen_t>(addrlen)) == MHD_YES;

src/detail/webserver_register.cpp

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,11 @@
4343
#include <stdio.h>
4444
#include <stdlib.h>
4545
#include <strings.h>
46-
#include <algorithm>
4746
#include <cstring>
4847
#include <iosfwd>
4948
#include <iostream>
5049
#include <memory>
5150
#include <mutex>
52-
#include <regex>
5351
#include <set>
5452
#include <shared_mutex>
5553
#include <stdexcept>
@@ -170,19 +168,12 @@ void webserver::unregister_impl_(const string& resource, bool family) {
170168
auto table_lock = impl_->routes_.lock_for_write();
171169
const std::string& key = he.get_url_complete();
172170
if (family) {
173-
impl_->routes_.param_and_prefix_routes_.remove(key, /*is_prefix=*/true);
171+
impl_->routes_.remove_param_prefix_locked_(key, /*is_prefix=*/true);
174172
} else if (!he.get_url_pars().empty()) {
175-
impl_->routes_.param_and_prefix_routes_.remove(key, /*is_prefix=*/false);
173+
impl_->routes_.remove_param_prefix_locked_(key, /*is_prefix=*/false);
176174
} else {
177175
// Erase from exact tier; also sweep regex tier (url_complete key).
178-
impl_->routes_.exact_routes_.erase(key);
179-
impl_->routes_.regex_routes_.erase(
180-
std::remove_if(impl_->routes_.regex_routes_.begin(),
181-
impl_->routes_.regex_routes_.end(),
182-
[&key](const detail::route_table::regex_route& rr) {
183-
return rr.url_complete == key;
184-
}),
185-
impl_->routes_.regex_routes_.end());
176+
impl_->routes_.erase_exact_and_regex_locked_(key);
186177
}
187178
}
188179
impl_->routes_.invalidate_route_cache();
@@ -209,17 +200,11 @@ void webserver::unregister_resource(const string& resource) {
209200
{
210201
auto table_lock = impl_->routes_.lock_for_write();
211202
const std::string& key = he_exact.get_url_complete();
212-
impl_->routes_.exact_routes_.erase(key);
213-
impl_->routes_.param_and_prefix_routes_.remove(key, /*is_prefix=*/false);
214-
impl_->routes_.param_and_prefix_routes_.remove(key, /*is_prefix=*/true);
215-
// Also sweep the regex tier by url_complete.
216-
impl_->routes_.regex_routes_.erase(
217-
std::remove_if(impl_->routes_.regex_routes_.begin(),
218-
impl_->routes_.regex_routes_.end(),
219-
[&key](const detail::route_table::regex_route& rr) {
220-
return rr.url_complete == key;
221-
}),
222-
impl_->routes_.regex_routes_.end());
203+
// Sweep every tier the key could occupy so a prior register_path AND
204+
// register_prefix on the same path are both cleared atomically.
205+
impl_->routes_.erase_exact_and_regex_locked_(key);
206+
impl_->routes_.remove_param_prefix_locked_(key, /*is_prefix=*/false);
207+
impl_->routes_.remove_param_prefix_locked_(key, /*is_prefix=*/true);
223208
}
224209
// Delegate cache clearing to invalidate_route_cache() matching the
225210
// pattern used by register_impl_ and on_methods_ (table lock released

src/httpserver/detail/daemon_lifecycle.hpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,15 @@ class daemon_lifecycle {
103103
// a TSan-flagged data race in the ws_start_stop integ test.
104104
std::atomic<struct MHD_Daemon*> daemon{nullptr};
105105

106+
// Acquire-load the published daemon handle, or nullptr when the daemon
107+
// is not currently started. Single home for the acquire-semantics
108+
// contract documented on `daemon` above, so webserver's run/info
109+
// accessors (webserver_lifecycle.cpp) don't each repeat the load + the
110+
// memory_order argument.
111+
struct MHD_Daemon* handle() const noexcept {
112+
return daemon.load(std::memory_order_acquire);
113+
}
114+
106115
// MHD_socket (int on POSIX, SOCKET on Windows) for a caller-supplied
107116
// pre-bound socket passed via create_webserver().bind_socket().
108117
// MHD_INVALID_SOCKET is the sentinel meaning "no pre-bound socket".

src/httpserver/detail/route_table.hpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,19 @@ class route_table {
181181
std::shared_ptr<http_resource> res,
182182
bool family);
183183

184+
// Erase @p key from the exact tier and sweep it from the regex tier
185+
// (every regex_route whose url_complete == key). These two tiers are
186+
// always cleared together on an exact-shaped unregister, so the sweep
187+
// lives here in one place rather than being open-coded at each call
188+
// site. Caller must hold route_table_mutex_ (unique_lock).
189+
void erase_exact_and_regex_locked_(const std::string& key);
190+
191+
// Remove @p key from the param/prefix trie tier. @p is_prefix selects
192+
// the prefix terminus (register_prefix) vs the parameterized terminus
193+
// (register_path with :params). Caller must hold route_table_mutex_
194+
// (unique_lock).
195+
void remove_param_prefix_locked_(const std::string& key, bool is_prefix);
196+
184197
// --- Route-table state -----------------------------------------------
185198
// One shared_mutex covering the three tiers. Writes (registration)
186199
// are rare; reads (dispatch lookups) take shared ownership briefly.

0 commit comments

Comments
 (0)