Skip to content

Commit a5bf116

Browse files
etrclaude
andcommitted
test: add collaborator unit/concurrency coverage; fix bench build
Close the coverage gaps left by the webserver_impl decomposition and restore the route-lookup performance gate: - hook_bus, ws_registry, daemon_lifecycle each gain a dedicated unit test that constructs the collaborator directly and pins its documented invariants (gate arming, collision contracts, flag composition), plus class-level route_table tests. - ws_registry's independent mutex gains a 4-writer/16-reader concurrency stress test mirroring route_table_concurrency. - bench_route_lookup.cpp referenced the moved constant webserver_impl::ROUTE_CACHE_MAX_SIZE, which stopped compiling after the route_table extraction and silently disabled the release perf gate; repoint it at route_table::ROUTE_CACHE_MAX_SIZE. - hoist the duplicated under_valgrind() helper into test/integ/test_utils.hpp; both concurrency tests include it. Full suite: 113/113 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e6af49f commit a5bf116

9 files changed

Lines changed: 978 additions & 16 deletions

test/Makefile.am

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ LDADD += -lcurl
2626

2727
AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver/ -DHTTPSERVER_COMPILATION
2828
METASOURCES = AUTO
29-
check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ip_access_control ws_start_stop authentication digest_challenge_format deferred http_resource http_response create_webserver create_webserver_explicit new_response_types daemon_info uri_log post_iterator_null_key feature_unavailable header_hygiene_iovec header_hygiene iovec_entry http_method constants body http_response_sbo http_response_factories http_response_digest_factory http_response_move_sanitizer webserver_pimpl http_request_pimpl create_test_request http_request_arena http_request_unescape_arena http_request_const_getters http_request_tls_accessors http_request_operator_stream webserver_register_smartptr webserver_register_path_prefix webserver_on_methods webserver_route route_table lookup_pipeline route_table_concurrency routing_regression route_lookup_canonicalize auth_skip_normalize http_resource_allow_cache v2_dispatch_contract threadsafety_stress webserver_features webserver_ws_unavailable webserver_ws_available webserver_register_ws_smartptr webserver_dauth_unavailable webserver_dauth_available consumer_fixture header_hygiene_hooks hook_api_shape hooks_no_firing hooks_accept_ctx_shape hooks_connection_lifecycle hooks_accept_decision_denied hooks_accept_decision_throwing hooks_body_chunk_ctx_shape hooks_request_received_short_circuit hooks_body_chunk_observes_progress hooks_body_chunk_short_circuit_no_leak hooks_before_handler_ctx_shape hooks_route_resolved_miss_and_hit hooks_before_handler_short_circuit hooks_alias_count hooks_alias_functional hooks_handler_exception_chain hooks_handler_exception_user_handler_throws_continues_chain hooks_handler_exception_fallback_to_hardcoded_500 hooks_handler_exception_slot hooks_response_sent_ctx_shape hooks_request_completed_ctx_shape hooks_after_handler_replaces_response hooks_after_handler_mutates_response_in_place hooks_response_sent_carries_status_bytes_timing hooks_request_completed_fires_on_early_failure hooks_log_access_alias_slot hooks_per_route_invalid_phase_throws hooks_per_route_order hooks_per_route_early_413_per_endpoint hooks_per_route_resource_destroyed_first hooks_per_route_concurrent_registration hooks_not_found_alias auth_handler_optional_signature no_v1_compat_shim cookie_header_sentinel cookie_render cookie_deprecation_sentinel http_response_cookie_wire http_request_cookies_parsed peer_address_to_string secure_zero_dce connection_state_sentinel connection_state_body_residue debug_dump_request_body_unset debug_dump_request_body_set debug_dump_request_body_zero littletest_skip_semantics digest_client_self_test unescaper_func
29+
check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ip_access_control ws_start_stop authentication digest_challenge_format deferred http_resource http_response create_webserver create_webserver_explicit new_response_types daemon_info uri_log post_iterator_null_key feature_unavailable header_hygiene_iovec header_hygiene iovec_entry http_method constants body http_response_sbo http_response_factories http_response_digest_factory http_response_move_sanitizer webserver_pimpl http_request_pimpl create_test_request http_request_arena http_request_unescape_arena http_request_const_getters http_request_tls_accessors http_request_operator_stream webserver_register_smartptr webserver_register_path_prefix webserver_on_methods webserver_route route_table lookup_pipeline route_table_concurrency hook_bus ws_registry ws_registry_concurrency daemon_lifecycle routing_regression route_lookup_canonicalize auth_skip_normalize http_resource_allow_cache v2_dispatch_contract threadsafety_stress webserver_features webserver_ws_unavailable webserver_ws_available webserver_register_ws_smartptr webserver_dauth_unavailable webserver_dauth_available consumer_fixture header_hygiene_hooks hook_api_shape hooks_no_firing hooks_accept_ctx_shape hooks_connection_lifecycle hooks_accept_decision_denied hooks_accept_decision_throwing hooks_body_chunk_ctx_shape hooks_request_received_short_circuit hooks_body_chunk_observes_progress hooks_body_chunk_short_circuit_no_leak hooks_before_handler_ctx_shape hooks_route_resolved_miss_and_hit hooks_before_handler_short_circuit hooks_alias_count hooks_alias_functional hooks_handler_exception_chain hooks_handler_exception_user_handler_throws_continues_chain hooks_handler_exception_fallback_to_hardcoded_500 hooks_handler_exception_slot hooks_response_sent_ctx_shape hooks_request_completed_ctx_shape hooks_after_handler_replaces_response hooks_after_handler_mutates_response_in_place hooks_response_sent_carries_status_bytes_timing hooks_request_completed_fires_on_early_failure hooks_log_access_alias_slot hooks_per_route_invalid_phase_throws hooks_per_route_order hooks_per_route_early_413_per_endpoint hooks_per_route_resource_destroyed_first hooks_per_route_concurrent_registration hooks_not_found_alias auth_handler_optional_signature no_v1_compat_shim cookie_header_sentinel cookie_render cookie_deprecation_sentinel http_response_cookie_wire http_request_cookies_parsed peer_address_to_string secure_zero_dce connection_state_sentinel connection_state_body_residue debug_dump_request_body_unset debug_dump_request_body_set debug_dump_request_body_zero littletest_skip_semantics digest_client_self_test unescaper_func
3030

3131
MOSTLYCLEANFILES = *.gcda *.gcno *.gcov
3232

@@ -301,9 +301,13 @@ webserver_route_SOURCES = unit/webserver_route_test.cpp
301301

302302
# route_table: TASK-027. Unit tests for the bespoke segment-trie
303303
# (detail::segment_trie<route_entry>) and the LRU route cache
304-
# (detail::route_cache). Exercises insert/find/remove on exact,
305-
# parameterized and prefix routes; cache key equality (method-distinct),
306-
# LRU eviction order, hit-promotion, and clear(). Default LDADD
304+
# (detail::route_cache), PLUS class-level tests that construct
305+
# detail::route_table directly (register_v2_route / lookup_v2 /
306+
# lock_for_write + the locked probe/upsert primitives) added when the
307+
# route_table collaborator was extracted from webserver_impl (commit
308+
# 21296ac) -- pinning the table-before-cache lock discipline and the
309+
# duplicate/collision-rejection logic at the class boundary rather than
310+
# only through webserver_impl's forwarders. Default LDADD
307311
# (libhttpserver + curl) is sufficient.
308312
route_table_SOURCES = unit/route_table_test.cpp
309313

@@ -329,6 +333,48 @@ lookup_pipeline_LDADD = $(LDADD) -lmicrohttpd
329333
route_table_concurrency_SOURCES = integ/route_table_concurrency.cpp
330334
route_table_concurrency_LDADD = $(LDADD) -lmicrohttpd
331335

336+
# hook_bus: extracted-collaborator unit test (commit 1f9a82e). Constructs
337+
# detail::hook_bus directly (no running webserver) and drives
338+
# add()/remove()/has_hooks_for()/phase_hook_count(), the phase-mismatch
339+
# and empty-callable registration throws, the any_hooks_ advisory-gate
340+
# arm/clear invariant, the two alias slots (handler_exception_alias_ DOES
341+
# arm the gate; log_access_alias_ does NOT), and the fire_* helpers'
342+
# try/catch-into-log_fn containment. Previously only reachable indirectly
343+
# through webserver's add_hook/hook_handle API. Default LDADD
344+
# (libhttpserver + curl) is sufficient -- hook_bus never touches MHD.
345+
hook_bus_SOURCES = unit/hook_bus_test.cpp
346+
347+
# ws_registry: extracted-collaborator unit test (commit b596fc8).
348+
# Constructs detail::ws_registry directly and drives
349+
# try_register/unregister/find/empty, the duplicate-key rejection (false
350+
# return, original registration untouched), the no-op unregister of an
351+
# unknown key, and the find()-returns-a-copy lifetime contract. Default
352+
# LDADD (libhttpserver + curl) is sufficient -- ws_registry never
353+
# dereferences websocket_handler, so it needs no MHD/websocket backend.
354+
ws_registry_SOURCES = unit/ws_registry_test.cpp
355+
356+
# ws_registry_concurrency: concurrency stress companion to ws_registry,
357+
# mirroring route_table_concurrency.cpp's pattern (4 writers + 16
358+
# readers hammering try_register/unregister/find/empty). ws_registry's
359+
# class comment documents its mutex as independent of every other
360+
# cluster's; unlike route_table and hook_bus, it previously had zero
361+
# concurrency verification anywhere in the suite (threadsafety_stress.cpp
362+
# has no register_ws_resource/unregister_ws_resource op). Needs no MHD
363+
# daemon -- ws_registry is self-contained, so default LDADD suffices.
364+
ws_registry_concurrency_SOURCES = integ/ws_registry_concurrency.cpp
365+
366+
# daemon_lifecycle: extracted-collaborator unit test (commit e6af49f).
367+
# Reaches the daemon_lifecycle every httpserver::webserver already owns
368+
# (impl_->daemon_) via webserver_test_access and drives
369+
# compose_transport_flags/compose_runtime_flags/compose_start_flags and
370+
# build_mhd_option_array's per-flag composition directly -- no MHD daemon
371+
# is ever started. Previously only reachable indirectly through a
372+
# running daemon via the integ-level start/stop tests. Needs
373+
# -lmicrohttpd because webserver's PIMPL impl carries microhttpd-typed
374+
# members (same rationale as lookup_pipeline / route_table_concurrency).
375+
daemon_lifecycle_SOURCES = unit/daemon_lifecycle_test.cpp
376+
daemon_lifecycle_LDADD = $(LDADD) -lmicrohttpd
377+
332378
# threadsafety_stress: TASK-032. DR-008 multi-thread contract gate.
333379
# Spawns 16 curl clients hitting an on_get handler that re-enters the
334380
# public webserver surface (register_path / unregister_path / deny_ip /

test/bench_route_lookup.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ int main() {
189189
static_assert(kNumPaths <= 9999,
190190
"buf[64] in make_radix_paths is sized for at most "
191191
"4-digit path indices");
192-
static_assert(kNumPaths > hs::detail::webserver_impl::ROUTE_CACHE_MAX_SIZE,
192+
static_assert(kNumPaths > hs::detail::route_table::ROUTE_CACHE_MAX_SIZE,
193193
"kNumPaths must exceed ROUTE_CACHE_MAX_SIZE so the LRU is "
194194
"guaranteed to evict every iteration (radix_pure must "
195195
"measure a cold cache)");

test/integ/route_table_concurrency.cpp

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737

3838
#include <atomic>
3939
#include <chrono>
40-
#include <cstdlib>
4140
#include <memory>
4241
#include <string>
4342
#include <thread>
@@ -46,19 +45,10 @@
4645
#include "./httpserver.hpp"
4746
#include "./httpserver/detail/webserver_impl.hpp"
4847
#include "./littletest.hpp"
48+
#include "./test_utils.hpp"
4949

5050
namespace ht = httpserver;
5151

52-
namespace {
53-
// The check-valgrind-* lanes run with VALGRIND=valgrind in the environment
54-
// (set by the Automake TESTS_ENVIRONMENT). Under valgrind every thread is
55-
// serialised onto one core, so the writer threads can starve the readers for
56-
// the entire window — a scheduler artifact, not a lock-discipline bug
57-
// (Memcheck/Helgrind/DRD report 0 errors on the access pattern itself). Detect
58-
// it so we relax ONLY the reader-liveness assertion, never the race/crash gate.
59-
bool under_valgrind() { return std::getenv("VALGRIND") != nullptr; }
60-
} // namespace
61-
6252
class noop_resource : public ht::http_resource {
6353
public:
6454
ht::http_response render_get(const ht::http_request&) override {

test/integ/test_utils.hpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,19 @@
2121
#ifndef TEST_INTEG_TEST_UTILS_HPP_
2222
#define TEST_INTEG_TEST_UTILS_HPP_
2323

24+
#include <cstdlib>
25+
2426
// Shared helpers for the integration test suite.
2527
//
28+
// under_valgrind(): the check-valgrind-* lanes run with VALGRIND=valgrind in
29+
// the environment (set by the Automake TESTS_ENVIRONMENT). Under valgrind
30+
// every thread is serialised onto one core, so writer/reader threads can
31+
// starve each other for the entire window of a concurrency stress test -- a
32+
// scheduler artifact, not a lock-discipline bug. Concurrency stress tests
33+
// (route_table_concurrency.cpp, ws_registry_concurrency.cpp) use this to
34+
// relax ONLY their liveness assertions, never the race/crash gate.
35+
inline bool under_valgrind() { return std::getenv("VALGRIND") != nullptr; }
36+
//
2637
// Historical note: this header previously exposed `as_shared(http_resource&)`,
2738
// a thin wrapper that returned a std::shared_ptr<http_resource> with a no-op
2839
// deleter pointing at a stack-allocated resource. That helper was incompatible
Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
This file is part of libhttpserver
3+
Copyright (C) 2011-2026 Sebastiano Merlino
4+
5+
This library is free software; you can redistribute it and/or
6+
modify it under the terms of the GNU Lesser General Public
7+
License as published by the Free Software Foundation; either
8+
version 2.1 of the License, or (at your option) any later version.
9+
10+
This library is distributed in the hope that it will be useful,
11+
but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
Lesser General Public License for more details.
14+
15+
You should have received a copy of the GNU Lesser General Public
16+
License along with this library; if not, write to the Free Software
17+
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
18+
USA
19+
*/
20+
21+
// Concurrent registration + lookup stress test for detail::ws_registry.
22+
//
23+
// ws_registry's class comment documents "its mutex is independent of
24+
// every other cluster's; no call site holds two of them at once" --
25+
// but unlike route_table (route_table_concurrency.cpp) and hook_bus
26+
// (folded into threadsafety_stress.cpp), ws_registry had zero
27+
// concurrency verification anywhere in the suite. This binary mirrors
28+
// route_table_concurrency.cpp's pattern: N writer threads doing
29+
// try_register/unregister and M reader threads doing find/empty
30+
// against disjoint and overlapping keys. Without correct lock
31+
// discipline in ws_registry's shared_mutex (unique_lock for
32+
// try_register/unregister, shared_lock for find/empty), this test
33+
// would deadlock, crash, or (under TSan) report a data race.
34+
//
35+
// Unlike route_table_concurrency.cpp, this test needs no webserver /
36+
// MHD daemon at all -- ws_registry is a self-contained collaborator
37+
// (it only stores/erases/copies shared_ptr<websocket_handler>, never
38+
// dereferencing the pointee), so the stress driver talks to a bare
39+
// instance directly.
40+
//
41+
// **TSan gate:** rebuild with `CXXFLAGS="-fsanitize=thread -g -O1"
42+
// LDFLAGS="-fsanitize=thread"` and re-run this binary; the tsan CI lane
43+
// (`build-type: tsan` in .github/workflows/verify-build.yml) picks it
44+
// up automatically via `make check` (no separate wiring needed --
45+
// route_table_concurrency required the RTC_ITERATIONS repeat gate
46+
// because it is TASK-092's headline stress target; this binary rides
47+
// the ordinary single-pass `make check` under that same tsan lane).
48+
49+
#include <atomic>
50+
#include <chrono>
51+
#include <memory>
52+
#include <string>
53+
#include <thread>
54+
#include <vector>
55+
56+
#include "./httpserver.hpp"
57+
#include "./httpserver/detail/ws_registry.hpp"
58+
#include "./littletest.hpp"
59+
#include "./test_utils.hpp"
60+
61+
namespace ht = httpserver;
62+
namespace htd = httpserver::detail;
63+
64+
namespace {
65+
class noop_ws_handler : public ht::websocket_handler {
66+
public:
67+
void on_message(ht::websocket_session&, std::string_view) override {}
68+
};
69+
} // namespace
70+
71+
LT_BEGIN_SUITE(ws_registry_concurrency_suite)
72+
void set_up() {}
73+
void tear_down() {}
74+
LT_END_SUITE(ws_registry_concurrency_suite)
75+
76+
LT_BEGIN_AUTO_TEST(ws_registry_concurrency_suite,
77+
concurrent_register_and_find_no_data_race)
78+
htd::ws_registry reg;
79+
80+
// Pre-register a stable set of keys so readers always have
81+
// something to find regardless of writer timing.
82+
for (int i = 0; i < 32; ++i) {
83+
reg.try_register("/stable/" + std::to_string(i),
84+
std::make_shared<noop_ws_handler>());
85+
}
86+
87+
std::atomic<bool> stop{false};
88+
std::atomic<int> writer_ops{0};
89+
std::atomic<int> reader_ops{0};
90+
91+
constexpr int kWriters = 4;
92+
constexpr int kReaders = 16;
93+
94+
std::vector<std::thread> threads;
95+
threads.reserve(kWriters + kReaders);
96+
97+
// Writers: register / unregister disjoint keys in their own numeric
98+
// range so try_register's duplicate-rejection path (false return)
99+
// is exercised only by the intentional re-register-after-unregister
100+
// sequence below, not by cross-writer collisions.
101+
for (int w = 0; w < kWriters; ++w) {
102+
threads.emplace_back([&, w] {
103+
int counter = 0;
104+
while (!stop.load(std::memory_order_relaxed)) {
105+
std::string key = "/dyn/" + std::to_string(w) + "/"
106+
+ std::to_string(counter % 8);
107+
(void)reg.try_register(key, std::make_shared<noop_ws_handler>());
108+
reg.unregister(key);
109+
++counter;
110+
writer_ops.fetch_add(1, std::memory_order_relaxed);
111+
}
112+
});
113+
}
114+
115+
// Readers: hammer find()/empty() against the stable + dynamic keys.
116+
for (int r = 0; r < kReaders; ++r) {
117+
threads.emplace_back([&, r] {
118+
int counter = 0;
119+
while (!stop.load(std::memory_order_relaxed)) {
120+
std::string key = "/stable/" + std::to_string(counter % 32);
121+
(void)reg.find(key);
122+
std::string key2 = "/dyn/" + std::to_string(r % 4) + "/"
123+
+ std::to_string(counter % 8);
124+
(void)reg.find(key2);
125+
(void)reg.empty();
126+
++counter;
127+
reader_ops.fetch_add(1, std::memory_order_relaxed);
128+
}
129+
});
130+
}
131+
132+
// Run until enough operations have been observed by both roles, or
133+
// until a wall-clock safety ceiling (30s) to survive valgrind/TSan
134+
// overhead. Mirrors route_table_concurrency.cpp's threshold-based
135+
// stop condition (avoids a fixed wall-clock sleep).
136+
constexpr int kOpThreshold = 10000;
137+
auto deadline =
138+
std::chrono::steady_clock::now() + std::chrono::seconds(30);
139+
while ((writer_ops.load() < kOpThreshold || reader_ops.load() < kOpThreshold)
140+
&& std::chrono::steady_clock::now() < deadline) {
141+
std::this_thread::sleep_for(std::chrono::milliseconds(10));
142+
}
143+
stop.store(true, std::memory_order_relaxed);
144+
for (auto& t : threads) t.join();
145+
146+
// We don't assert specific counts -- the gate is "completed without
147+
// deadlock or crash". TSan-detected races would break the build
148+
// when this TU is rebuilt under the manual TSan gate. Liveness (both
149+
// roles made progress) can be defeated by valgrind's single-core
150+
// scheduler, so only assert it off-valgrind (same posture as
151+
// route_table_concurrency.cpp).
152+
if (!under_valgrind()) {
153+
LT_CHECK(writer_ops.load() > 0);
154+
LT_CHECK(reader_ops.load() > 0);
155+
}
156+
LT_END_AUTO_TEST(concurrent_register_and_find_no_data_race)
157+
158+
LT_BEGIN_AUTO_TEST_ENV()
159+
AUTORUN_TESTS()
160+
LT_END_AUTO_TEST_ENV()

0 commit comments

Comments
 (0)