Skip to content

Commit 58e3399

Browse files
etrclaude
andcommitted
docs(dr-014): drop connection_callbacks — 7 services, not 8
Per implementation finding: connection_notify / policy_callback / request_completed are static MHD trampolines (addresses registered with libmicrohttpd) whose real behavior is already decomposed — classify_decision and make_peer_address are free functions, IP work delegates to ip_access_control, hook firing to hook_dispatcher. What remains is MHD-adapter glue (per-connection arena via socket_context, the con_cls request lifecycle, a null-cls guard). Wrapping it in a class adds indirection without removing god-object state and would force a behavior change. So they stay in the MHD-adapter layer that DR-014 already keeps static. Updates DR-014 (7-service table, rationale, adapter-layer note, migration order) and dispatch-pipeline.md §4.11 (table, DAG, adapter paragraph). A revisit flag remains to reconfirm at the end of the migration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 4da8201 commit 58e3399

2 files changed

Lines changed: 29 additions & 14 deletions

File tree

specs/architecture/04-components/dispatch-pipeline.md

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,18 @@
11
### 4.11 Dispatch pipeline (request-processing services)
22

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.
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 seven per-server **behavior services**. Each is an internal `httpserver::detail` type gated on `HTTPSERVER_COMPILATION`; none appears on the public surface or ABI. (The connection-lifecycle callbacks — `connection_notify` / `policy_callback` / `request_completed` — were *not* extracted into an eighth service; they stay in the MHD-adapter layer, see below and DR-014.)
44

55
**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.
66

7-
**The eight services.**
7+
**The seven services.**
88

99
| Service (`src/httpserver/detail/…hpp`, `src/detail/…cpp`) | Owns | Constructed with |
1010
|---|---|---|
1111
| `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&` |
12+
| `response_materializer` | `http_response``MHD_Response`, decorate + queue, digest-challenge queueing, null-response fallback | `error_pages&`, `hook_dispatcher&`, digest opaque, `const webserver_config&` |
13+
| `hook_dispatcher` | the four gated `fire_*_gated` helpers + the eleven per-phase forwarders over `hook_bus` | `hook_bus&`, `const webserver_config&` |
1414
| `upload_pipeline` | `process_file_upload`, upload-stream lifecycle, post-iterator target | `const webserver_config&` |
1515
| `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&` |
1716
| `request_dispatcher` | `finalize_answer` orchestration, route resolution, auth-skip, handler invocation, 405 path | `route_table&`, `hook_dispatcher&`, `error_pages&`, `response_materializer&`, `const webserver_config&` |
1817
| `request_pipeline` | `answer_to_connection` body, first/second body steps, `complete_request` | `const webserver_config&`, `hook_dispatcher&`, `request_dispatcher&` |
1918

@@ -29,13 +28,12 @@ request_pipeline
2928
└─▶ response_materializer
3029
├─▶ error_pages
3130
└─▶ hook_dispatcher
32-
connection_callbacks ─▶ { ip_access_control, hook_dispatcher }
3331
websocket_upgrader ─▶ ws_registry
3432
```
3533

3634
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).
3735

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.
36+
**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. `connection_notify`, `policy_callback`, and `request_completed` are the fullest members of this layer: they keep their per-connection arena (`connection_state` new/delete via `socket_context`), accept-policy, and request-teardown glue inline, delegating only the extracted free functions (`classify_decision`, `make_peer_address`) and the `ip_access_control` / `hook_dispatcher` collaborators — which is why they were not carved into a separate service (DR-014).
3937

4038
**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`.
4139

specs/architecture/11-decisions/DR-014.md

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,42 @@ When both kinds are extracted, `webserver_impl` becomes a **composition root**:
1616
3. **A single `request_processor` mediator** owning all the logic, sub-methods still on one class.
1717
4. **Leave it as files; add a `request_context` bundle** passed to each method for testability.
1818

19-
**Decision:** Option 1. Eight services:
19+
**Decision:** Option 1, refined during implementation to **seven** behavior
20+
services (see the `connection_callbacks` note below):
2021

2122
| Service | Responsibility | Ctor dependencies |
2223
|---|---|---|
2324
| `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&` |
25+
| `response_materializer` | `http_response``MHD_Response` + queue | `error_pages&`, `hook_dispatcher&`, digest opaque, `const webserver_config&` |
26+
| `hook_dispatcher` | dispatch-time hook gating + firing | `hook_bus&`, `const webserver_config&` |
2627
| `upload_pipeline` | multipart / file-upload handling | `const webserver_config&` |
2728
| `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&` |
2929
| `request_dispatcher` | route + auth + handler invocation | `route_table&`, `hook_dispatcher&`, `error_pages&`, `response_materializer&`, `const webserver_config&` |
3030
| `request_pipeline` | MHD re-entrant body accumulation | `const webserver_config&`, `hook_dispatcher&`, `request_dispatcher&` |
3131

32+
**`connection_callbacks` was not extracted (7, not 8).** The eighth candidate
33+
in the original analysis was a `connection_callbacks` service wrapping
34+
`connection_notify` / `policy_callback` / `request_completed`. Implementation
35+
showed these three are **static MHD trampolines** (their addresses are
36+
registered with libmicrohttpd) — i.e. the C-ABI adapter layer that this DR
37+
already keeps static — and their behavior is *already* decomposed: the accept
38+
policy (`classify_decision`) and the sockaddr→`peer_address` adapter
39+
(`make_peer_address`) are free functions, the IP work delegates to
40+
`ip_access_control`, and hook firing delegates to `hook_dispatcher`. What
41+
remains in them is MHD-adapter glue (per-connection arena `new`/`delete` via
42+
`socket_context`, the `con_cls` request lifecycle, a null-`cls` defensive
43+
guard). Wrapping that in an instance-method class adds indirection without
44+
removing any god-object state and would force a behavior change (dropping the
45+
untested null-`cls` arena path). So the three stay as delegating trampolines in
46+
the MHD-adapter layer. (Revisit flag: reconfirm at the end of the migration
47+
that no request-processing behavior beyond adapter glue accreted here.)
48+
3249
**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.
50+
- **Seven, 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 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. The connection-lifecycle callbacks are the exception in the other direction — they stay in the adapter layer rather than become an eighth service (see the note above).
3451
- **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.
3552
- **`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.
3653

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.
54+
**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. `connection_notify` / `policy_callback` / `request_completed` are the fullest expression of this layer: they retain their per-connection arena and accept-policy glue directly (delegating only the extracted free functions and the `ip_access_control` / `hook_dispatcher` collaborators), which is why no `connection_callbacks` service was carved out.
3855

3956
**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`.
4057

@@ -52,7 +69,7 @@ When both kinds are extracted, `webserver_impl` becomes a **composition root**:
5269
- White-box tests reaching in via `webserver_test_access` update their access points per extraction step.
5370
- 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).
5471

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`.
72+
**Migration.** Leaf-first, one service per commit into `feature/v2.0`, each keeping CI green — mirroring how the five state collaborators landed: `error_pages``hook_dispatcher``response_materializer``upload_pipeline``websocket_upgrader``request_dispatcher``request_pipeline` → fold route registration into `route_table`.
5673

5774
**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.
5875

0 commit comments

Comments
 (0)