Skip to content

Commit c7b969f

Browse files
etrclaude
andcommitted
docs(architecture): DR-014 — behavior decomposition of webserver_impl
Record the decision to extract the ~80 request-processing methods still living on the webserver_impl god-object into eight per-server behavior services (error_pages, response_materializer, hook_dispatcher, upload_pipeline, websocket_upgrader, connection_callbacks, request_dispatcher, request_pipeline), leaving webserver_impl as a pure composition root over the five existing state collaborators plus these services. DR-014 also names the two collaborator kinds (state vs behavior) and is the missing decision record for the already-landed state extractions. Adds the §4.11 dispatch-pipeline component spec (services, the DAG, the MHD adapter layer, free-function extractions) and updates webserver.md §4.1, which still described webserver_impl monolithically. Implementation lands leaf-first, one service per commit; the helgrind lane fix is sequenced after the decomposition (still v2.0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1bb47bd commit c7b969f

3 files changed

Lines changed: 111 additions & 1 deletion

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
### 4.11 Dispatch pipeline (request-processing services)
2+
3+
**Responsibility:** The request-processing behavior of the server — everything between a libmicrohttpd callback firing and a response being queued — factored out of the `webserver_impl` god-object (DR-014) into eight per-server **behavior services**. Each is an internal `httpserver::detail` type gated on `HTTPSERVER_COMPILATION`; none appears on the public surface or ABI.
4+
5+
**What a behavior service is.** Distinct from a *state collaborator* (route_table/hook_bus/… — owns a mutex + data): a behavior service owns *logic*, not state. It is constructed once per `webserver_impl`, holds only `const&` references to its dependencies, owns no mutable state, takes no locks, and operates on the per-request `detail::modded_request` passed in by reference. Because it is stateless and lock-free it is inherently shareable across MHD worker threads; the decomposition adds no synchronization.
6+
7+
**The eight services.**
8+
9+
| Service (`src/httpserver/detail/…hpp`, `src/detail/…cpp`) | Owns | Constructed with |
10+
|---|---|---|
11+
| `error_pages` | not_found / method_not_allowed / internal_error synthesis, `run_internal_error_handler_safely` | `const webserver_config&` |
12+
| `response_materializer` | `http_response``MHD_Response`, decorate + queue, digest-challenge queueing, null-response fallback | `error_pages&`, digest opaque string |
13+
| `hook_dispatcher` | the four gated `fire_*_gated` helpers + the eleven per-phase forwarders over `hook_bus` | `hook_bus&` |
14+
| `upload_pipeline` | `process_file_upload`, upload-stream lifecycle, post-iterator target | `const webserver_config&` |
15+
| `websocket_upgrader` | RFC-6455 handshake validate/complete + the upgrade callback (`HAVE_WEBSOCKET`) | `ws_registry&` |
16+
| `connection_callbacks` | connection-notify arena new/delete, accept-policy decision, request-completed teardown | `const webserver_config&`, `ip_access_control&`, `hook_dispatcher&` |
17+
| `request_dispatcher` | `finalize_answer` orchestration, route resolution, auth-skip, handler invocation, 405 path | `route_table&`, `hook_dispatcher&`, `error_pages&`, `response_materializer&`, `const webserver_config&` |
18+
| `request_pipeline` | `answer_to_connection` body, first/second body steps, `complete_request` | `const webserver_config&`, `hook_dispatcher&`, `request_dispatcher&` |
19+
20+
**Dependency graph (a DAG).**
21+
22+
```
23+
request_pipeline
24+
├─▶ hook_dispatcher ─▶ hook_bus
25+
└─▶ request_dispatcher
26+
├─▶ route_table
27+
├─▶ hook_dispatcher
28+
├─▶ error_pages ─▶ (const webserver_config&)
29+
└─▶ response_materializer
30+
├─▶ error_pages
31+
└─▶ hook_dispatcher
32+
connection_callbacks ─▶ { ip_access_control, hook_dispatcher }
33+
websocket_upgrader ─▶ ws_registry
34+
```
35+
36+
Acyclic, so the composition root wires it with plain member references — no mediator. Services store references at construction but never invoke a dependency during their own constructor, so binding a reference to a sibling member is well-defined irrespective of member-declaration order (`-Wreorder -Werror` guards accidental reorders).
37+
38+
**The MHD adapter layer.** libmicrohttpd calls in through fixed-signature C trampolines carrying a `void* cls` closure: `answer_to_connection`, `request_completed`, `connection_notify`, `policy_callback`, `post_iterator`, `uri_log`, `error_log`, `unescaper_func`, `upgrade_handler`, plus the GnuTLS `psk_cred_handler_func` / `sni_cert_callback_func`. These stay `static`/free functions — they unpack `cls` (a `webserver_impl*`, `webserver*`, `modded_request*`, or `ws_upgrade_data*`) and forward into a service. They are the C-ABI boundary, not behavior.
39+
40+
**Free functions (not services).** Pure, instance-stateless helpers live as free functions in `httpserver::detail`, not one-method classes: `log_dispatch_error(const webserver_config&, std::string_view)` (called by every error path — a free function keeps it dependency-edge-free), `serialize_allow_methods` / `format_allow_header`, `resolve_method_callback`, `materialize_response`, `decorate_mhd_response`, `handle_post_form_arg`, `manage_upload_stream`.
41+
42+
**Per-request context.** `detail::modded_request` (per connection, arena/PMR-allocated per DR-003b) is the object the pipeline threads through the MHD callback sequence: `uri_log` allocates it; `answer_to_connection` (first invocation) stamps `start_time` / `standardized_url` / `method_enum` and builds the `http_request`; the body steps accumulate upload data; `finalize_answer` stages `response`; `request_completed` fires the terminal hook and deletes it. The services read and write its fields but do not own it.
43+
44+
**Key design notes:**
45+
- Config is read-only to every service (`const webserver_config&`); no service holds the `webserver*` back-pointer. The back-pointer stays on the composition root for the members that genuinely need the owning `webserver*`.
46+
- The decomposition is behavior-only: no new mutexes, no new shared mutable state, no change to lock order (route_table → resource hook mutex → hook_table mutex, per §4.10).
47+
- Structural acceptance is enforced by the same gates as the rest of `src/`: 500 SLOC per file, CCN ≤ 10 per function, CPD ≥ 100-token duplication.
48+
49+
**Related decisions:** DR-014 (this decomposition), DR-012 (hook bus — the state-collaborator precedent and the phase/firing map in §4.10), DR-007 (route table), DR-003b (request PIMPL / arena allocation), DR-009 §5.2 (exception → 500 contract that `error_pages` implements).
50+
51+
---

specs/architecture/04-components/webserver.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
**Responsibility:** Library entry point. Owns the libmicrohttpd daemon, the route table, the IP block list, the connection arena pool. Provides start/stop, route registration (lambda + class forms), `block_ip`/`unblock_ip`, `features()`.
44

5-
**Implementation:** PIMPL via `std::unique_ptr<webserver_impl>`. Public header `<httpserver/webserver.hpp>` includes only `<httpserver/create_webserver.hpp>` and standard library, never `<microhttpd.h>` or `<pthread.h>`. `webserver_impl` (in `src/httpserver/detail/webserver_impl.hpp`) holds the `MHD_Daemon*`, the route-table data structures, per-connection arena state, and synchronization primitives.
5+
**Implementation:** PIMPL via `std::unique_ptr<webserver_impl>`. Public header `<httpserver/webserver.hpp>` includes only `<httpserver/create_webserver.hpp>` and standard library, never `<microhttpd.h>` or `<pthread.h>`. `webserver_impl` (in `src/httpserver/detail/webserver_impl.hpp`) is a **composition root** (DR-014): it does not itself hold the daemon handle, route tiers, or synchronization primitives — those live behind *state collaborators* (`daemon_lifecycle`, `route_table`, `hook_bus`, `ip_access_control`, `ws_registry`, each owning its own mutex + data) — and it does not itself carry the request-processing logic — that lives in *behavior services* (`request_pipeline`, `request_dispatcher`, `response_materializer`, `error_pages`, `hook_dispatcher`, `upload_pipeline`, `websocket_upgrader`, `connection_callbacks`; see §4.11). `webserver_impl` constructs and wires both kinds, holds the `parent` back-pointer for the few members that need the owning `webserver*`, and exposes the static libmicrohttpd trampolines that forward into the services.
66

77
**Interfaces:**
88
- Exposes (from PRD §3.4 and §3.7):
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
### DR-014: Behavior decomposition of `webserver_impl` into request-pipeline services
2+
3+
**Status:** Accepted
4+
**Date:** 2026-07-18
5+
**Context:** The v2.0 refactor extracted five *state* clusters out of the `webserver_impl` god-object, each into its own collaborator that owns a mutex plus data: `daemon_lifecycle`, `route_table`, `hook_bus`, `ip_access_control`, `ws_registry` (recorded to date only as implementation-status prose in the §4.x component docs — this DR is also the missing decision record for that work). What remained un-extracted is *behavior*: ~80 `webserver_impl::` member functions, spread across ~20 `src/detail/webserver_*.cpp` translation units, that drive the libmicrohttpd callback sequence and read/write the per-request `detail::modded_request`. The physical file split (forced by the 500-SLOC per-file gate) gave the *appearance* of decomposition without the ownership structure behind it: every one of those methods is still a method of one class, free to reach any field via the `parent` back-pointer. This is the residual god-object.
6+
7+
**Two kinds of collaborator.** This DR names the distinction the refactor had been making implicitly:
8+
- **State collaborators** own a mutex + mutable data (the five above). Extracted by DR-007/DR-012 and predecessors.
9+
- **Behavior services** own *logic*, not state. They are per-server, effectively stateless, operate on a `modded_request&` passed in, and hold only `const&` references to their dependencies. This DR introduces them.
10+
11+
When both kinds are extracted, `webserver_impl` becomes a **composition root**: a constructor, the five state members, the behavior-service members, and the static MHD trampolines. It contains no request logic of its own.
12+
13+
**Options considered:**
14+
1. **Eight behavior services over `modded_request`, wired as a DAG, held by the composition root.** Config-hungry services take `const webserver_config&`; the rest take specific collaborator/service references.
15+
2. **Fewer, coarser services (4–5).** Merge the pipeline/dispatch/materialize stages.
16+
3. **A single `request_processor` mediator** owning all the logic, sub-methods still on one class.
17+
4. **Leave it as files; add a `request_context` bundle** passed to each method for testability.
18+
19+
**Decision:** Option 1. Eight services:
20+
21+
| Service | Responsibility | Ctor dependencies |
22+
|---|---|---|
23+
| `error_pages` | synthesise 404/405/500 responses | `const webserver_config&` |
24+
| `response_materializer` | `http_response``MHD_Response` + queue | `error_pages&`, digest opaque |
25+
| `hook_dispatcher` | dispatch-time hook gating + firing | `hook_bus&` |
26+
| `upload_pipeline` | multipart / file-upload handling | `const webserver_config&` |
27+
| `websocket_upgrader` | RFC-6455 handshake (`HAVE_WEBSOCKET`) | `ws_registry&` |
28+
| `connection_callbacks` | MHD connection/daemon lifecycle mapping | `const webserver_config&`, `ip_access_control&`, `hook_dispatcher&` |
29+
| `request_dispatcher` | route + auth + handler invocation | `route_table&`, `hook_dispatcher&`, `error_pages&`, `response_materializer&`, `const webserver_config&` |
30+
| `request_pipeline` | MHD re-entrant body accumulation | `const webserver_config&`, `hook_dispatcher&`, `request_dispatcher&` |
31+
32+
**Rationale:**
33+
- **Eight, by reason-to-change (Option 1 over 2).** Each service has a distinct change axis — MHD body protocol vs routing/auth vs wire translation vs hook gating vs upload vs ws handshake vs connection lifecycle vs synthetic pages. The 500-SLOC and CCN-10 gates already reward this granularity; coarser services would re-hit the gates and re-fragment. The one defensible merge (`request_pipeline`+`request_dispatcher`, the most-coupled pair) was considered and rejected: they have genuinely different reasons to change.
34+
- **DAG, no mediator (over Option 3).** The dependency graph is acyclic — `request_pipeline → request_dispatcher → {response_materializer, error_pages, hook_dispatcher, route_table}`, `response_materializer → {error_pages, hook_dispatcher}`, `error_pages` is a leaf. Plain member references in the composition root suffice; no mediator/service-locator is needed. Because services only *store* references at construction (never invoke a dependency during their own ctor), binding a reference to a sibling member is well-defined regardless of member-declaration order; `-Wreorder -Werror` still guards accidental reorders.
35+
- **`const webserver_config&`, not the `parent` back-pointer (rejecting Option 4's bundle).** The `webserver*` back-pointer is exactly the "reach into anything, including sibling impl state" anti-pattern that made the class a god-object; a `request_context` bundle is a service-locator that re-grants it. A `const webserver_config&` states at the type level "I read configuration, I mutate nothing." The `parent` pointer stays on the composition root for the few members that genuinely need the owning `webserver*` (e.g. `modded_request::ws`), not on the services.
36+
37+
**MHD adapter layer.** libmicrohttpd calls in through fixed-signature C trampolines with a `void* cls` closure (`answer_to_connection`, `request_completed`, `connection_notify`, `policy_callback`, `post_iterator`, `uri_log`, `error_log`, `unescaper_func`, `upgrade_handler`, and the GnuTLS `psk_cred_handler_func` / `sni_cert_callback_func`). These remain `static`/free functions — they unpack `cls` and forward into the appropriate service. They are the C-ABI boundary, not behavior.
38+
39+
**Free functions, not classes.** Pure statics with no instance state become free functions in `httpserver::detail` rather than spurious one-method classes: `log_dispatch_error(const webserver_config&, std::string_view)` (shared by every error path, so a free function avoids a dependency edge to `error_pages`), `serialize_allow_methods`/`format_allow_header`, `resolve_method_callback`, `materialize_response`, `decorate_mhd_response`, `handle_post_form_arg`, `manage_upload_stream`.
40+
41+
**Route registration is not request behavior.** `prepare_or_create_lambda_shim` / `commit_handlers_to_shim` are write-path shim lifecycle already run under `route_table::lock_for_write()`; they fold into `route_table`'s writer side, not the request path.
42+
43+
**Threading / lifetime.** The eight services are per-server, constructed once at `webserver_impl` construction, destroyed with it. They own no mutable state and take no locks, so they are inherently shareable across MHD worker threads — the decomposition adds **zero mutexes and is race-detector-neutral**. `modded_request` remains the per-request context, arena/PMR-allocated per DR-003b; the services take `modded_request&` and never allocate it, so DR-003b is untouched. Config is read-only (`const&`), matching the single-writer-at-construction posture.
44+
45+
**Scope boundaries.**
46+
- **`http_request` is out of scope.** Its backing `detail::http_request_impl` is a fat impl (~40 members over five concerns, file-split into `_args`/`_tls`/`_auth`), but it is handled as a single impl object and the sprawl is a minor, tolerable issue. Rather than decompose it, its structure and file layout are documented (§4.2) so tooling and contributors can navigate it. A future collaborator split would be constrained by DR-003b's per-connection arena/PMR allocation and is deferred.
47+
- **`http_response`, `http_resource`, `webserver_config`, and the `webserver` facade need no decomposition** — a sealed single-concern value type, a cohesive handler base (per-route hooks already in `resource_hook_table`), an intentional passive data bag, and a thin pimpl facade respectively.
48+
49+
**Consequences:**
50+
- No public-surface change: all eight services are internal `detail` types under the `HTTPSERVER_COMPILATION` gate; the public headers and ABI are untouched.
51+
- `webserver_impl` shrinks to a composition root; the ~20 `webserver_*.cpp` TUs consolidate into eight service TUs plus the MHD-adapter and free-function TUs.
52+
- White-box tests reaching in via `webserver_test_access` update their access points per extraction step.
53+
- The `helgrind`/`drd` race-detector lanes regressed during the state-decomposition window (green through 2026-07-15); the fix is sequenced *after* this behavior decomposition lands, still within v2.0 (the decomposition is race-detector-neutral, so it neither fixes nor worsens the lanes).
54+
55+
**Migration.** Leaf-first, one service per commit into `feature/v2.0`, each keeping CI green — mirroring how the five state collaborators landed: free-function extractions → `error_pages``response_materializer``hook_dispatcher``upload_pipeline``websocket_upgrader``connection_callbacks``request_dispatcher``request_pipeline` → fold route registration into `route_table`.
56+
57+
**Verification:** existing `make check` (113 tests) stays green at every step; the per-file 500-SLOC and per-function CCN-10 gates are the structural acceptance criteria; `basic`/`file_upload`/`ws_start_stop`/`deferred` under helgrind/drd return to green in the follow-on lane fix.
58+
59+
---

0 commit comments

Comments
 (0)