Skip to content

Commit 1cc7883

Browse files
committed
TASK-036 minors: continue marking remaining items (40 still pending verification)
1 parent 72614c0 commit 1cc7883

1 file changed

Lines changed: 6 additions & 3 deletions

File tree

specs/unworked_review_issues/2026-05-18_142351_task-036.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,17 +73,20 @@ then use entry (or std::move(entry) for the regex push_back). This removes ~6 du
7373

7474
## Minor
7575

76-
13. [ ] **architecture-alignment-checker** | `src/httpserver/create_webserver.hpp:67` | adr-violation
76+
13. [x] **architecture-alignment-checker** | `src/httpserver/create_webserver.hpp:67` | adr-violation
7777
auth_handler_ptr is still typed as std::function<std::shared_ptr<http_response>(const http_request&)> — a v1 shared_ptr return pattern — while DR-004 mandates http_response by value for all handler signatures. The dispatch site in webserver.cpp (line 2227-2229) bridges the gap with a manual std::move(*auth_response) into mr->response_, with a comment explicitly marking this as out of scope for TASK-036.
7878
*Recommendation:* Track in a follow-up task: change auth_handler_ptr to std::function<std::optional<http_response>(const http_request&)> (optional because null means 'pass through') and remove the bridging code. This would complete the DR-004 rollout to the auth hook.
79+
*Status:* already addressed — TODO comment added on auth_handler_ptr in create_webserver.hpp (commit a11e5ee). The deferred migration is tracked.
7980

80-
14. [ ] **architecture-alignment-checker** | `src/httpserver/detail/modded_request.hpp:51` | pattern-violation
81+
14. [x] **architecture-alignment-checker** | `src/httpserver/detail/modded_request.hpp:51` | pattern-violation
8182
The callback pointer-to-member (http_response (http_resource::*callback)(...)) has no default member initializer. If an unrecognized HTTP method is received and a resource is found, mr->callback is indeterminate. The code comment at webserver.cpp:2382 acknowledges this as a pre-existing latent bug from TASK-027, and the 405 guard (is_allowed(count_) == false) prevents the indeterminate pointer from being dereferenced in practice. However, TASK-036 introduced the member with its current return-by-value signature, making it the right time to add a null initializer or a static_assert.
8283
*Recommendation:* Add a default member initializer: http_response (httpserver::http_resource::*callback)(const httpserver::http_request&) = nullptr; and guard the dispatch call site with an assert or explicit null check to eliminate the latent UB.
84+
*Status:* already addressed — `= nullptr` default initializer added to `callback` field (commit 22d8f07). Comment in resolve_method_callback documents the nullptr-for-unrecognized-method invariant.
8385

84-
15. [ ] **architecture-alignment-checker** | `src/webserver.cpp:2307` | interface-contract
86+
15. [x] **architecture-alignment-checker** | `src/webserver.cpp:2307` | interface-contract
8587
For a deferred (streaming) response, finalize_answer calls MHD_destroy_response(raw_response) immediately after MHD_queue_response. The deferred_body trampoline passes this (deferred_body*) as the cls pointer to MHD_create_response_from_callback. The body is safe because MHD's own reference counting keeps the MHD_Response alive until streaming completes, and request_completed (which destroys the modded_request and its embedded response_) only fires after MHD is done. However the code comment at the call site does not document this MHD ref-counting assumption, leaving a subtle lifetime dependency undocumented.
8688
*Recommendation:* Add a comment near MHD_destroy_response in finalize_answer explaining that for MHD_create_response_from_callback responses, MHD increments its own refcount during MHD_queue_response, so the immediate MHD_destroy_response only releases the caller's reference — the trampoline's cls pointer (deferred_body*) remains valid until request_completed fires.
89+
*Status:* fixed in this batch (commit 72614c0 — added MHD refcounting comment near MHD_destroy_response in materialize_and_queue_response in webserver_request.cpp).
8790

8891
16. [ ] **code-quality-reviewer** | `src/Makefile.am:25` | code-elegance
8992
Removal of http_resource.cpp from sources is correct and clean. No issue, noted for completeness that the build system correctly reflects the header-only migration.

0 commit comments

Comments
 (0)