Skip to content

Commit ed3a78b

Browse files
committed
ci: complete TASK-058 max_args / cache field-rename feature pieces
Earlier CI fix commits (2e803ed, 096424d, fdee3d7) inadvertently swept in halves of an in-progress, multi-file refactor that was sitting uncommitted in the working tree. HEAD became internally inconsistent: http_request_impl.cpp referenced cs->max_args_count and the ensure_args_flat_view_cached / path_pieces_cache_built_ field set, but the matching declarations on http_request_impl.hpp, connection_state.hpp, create_webserver.hpp, and webserver.hpp were not in HEAD, breaking every Verify Build lane that actually builds tests. Commit the matching pieces so HEAD is self-consistent again: * src/httpserver/webserver.hpp + src/webserver.cpp: const max_args_count / max_args_bytes members on the webserver class + matching ctor initializer-list lines reading the values out of create_webserver. * src/httpserver/detail/connection_state.hpp: per-connection max_args_count / max_args_bytes fields plus the comment update explaining the compile-time ARENA_INITIAL_BYTES decision. * src/httpserver/detail/http_request_impl.hpp: rename path_pieces / path_pieces_public_ to path_pieces_cached_ + args_flat_view_cached_; add args_flat_view_cache_built_ and path_pieces_cache_built_ guards. * src/httpserver/http_request.hpp + src/http_request.cpp: forward the new arg / path-piece flat-view accessors through the public class. * src/httpserver/http_response.hpp, http_method.hpp, detail/modded_request.hpp, detail/radix_tree.hpp: smaller in-flight refactors that interact with the above renames. * Makefile.am: extra noinst headers / test entries that came along with the renames. * test/REGRESSION.md, test/headers/*, test/integ/{authentication,basic}.cpp, test/unit/{http_response_sbo,routing_regression}_test.cpp: test-side adjustments to match the new field / accessor names. No behavioural change beyond what those files already document; the feature work itself was authored on this branch before the CI sweep started.
1 parent 34d5de7 commit ed3a78b

18 files changed

Lines changed: 265 additions & 240 deletions

Makefile.am

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,15 @@ EXTRA_DIST = libhttpserver.pc.in $(DX_CONFIG) scripts/extract-release-notes.sh s
7676
# that -DHTTPSERVER_COMPILATION (set in src/ and test/ AM_CPPFLAGS) cannot
7777
# leak into the consumer-style compile. A true consumer never has that macro.
7878
CHECK_HEADERS_CXX = $(CXX) -std=c++20 -I$(top_builddir) -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver $(CPPFLAGS)
79-
CHECK_HEADERS_GATE_MSG = Only <httpserver.hpp> or <httpserverpp> can be included directly.
79+
# Public-header gate (consumer_direct, consumer_post_umbrella): triggered when
80+
# a public httpserver/* header is included without the umbrella having defined
81+
# _HTTPSERVER_HPP_INSIDE_.
82+
CHECK_HEADERS_PUBLIC_GATE_MSG = Only <httpserver.hpp> or <httpserverpp> can be included directly.
83+
# Detail-header gate (consumer_detail, consumer_detail_modded): post-TASK-014
84+
# the detail-header gate is HTTPSERVER_COMPILATION-only and emits a message of
85+
# the form "httpserver/detail/<file>.hpp is internal; only include it when
86+
# compiling libhttpserver (HTTPSERVER_COMPILATION must be defined)."
87+
CHECK_HEADERS_DETAIL_GATE_MSG = is internal; only include it when compiling libhttpserver
8088

8189
check-headers:
8290
@echo "=== check-headers A.1: direct public-header include must fail ==="
@@ -86,7 +94,7 @@ check-headers:
8694
rm -f check-headers-A1.log; \
8795
exit 1; \
8896
fi
89-
@if ! grep -q "$(CHECK_HEADERS_GATE_MSG)" check-headers-A1.log; then \
97+
@if ! grep -q "$(CHECK_HEADERS_PUBLIC_GATE_MSG)" check-headers-A1.log; then \
9098
echo "FAIL: consumer_direct.cpp failed but not for the gate reason"; \
9199
cat check-headers-A1.log; \
92100
rm -f check-headers-A1.log; \
@@ -95,18 +103,18 @@ check-headers:
95103
@rm -f check-headers-A1.log
96104
@echo " PASS: A.1 gate fired as expected"
97105
@echo "=== check-headers A.2: direct detail-header include must fail ==="
98-
@# NOTE: A.2 currently fires for the same reason as A.1 (neither macro defined).
99-
@# After TASK-014 tightens the detail gate to HTTPSERVER_COMPILATION-only,
100-
@# consumer_detail.cpp will add '#define _HTTPSERVER_HPP_INSIDE_' to exercise
101-
@# the stricter path. Do not collapse A.2 into A.1 the include path distinction
102-
@# (detail/ vs public) is valuable and A.2 becomes a discriminating test post-TASK-014.
106+
@# Post-TASK-014: the detail-header gate is HTTPSERVER_COMPILATION-only.
107+
@# consumer_detail.cpp defines _HTTPSERVER_HPP_INSIDE_ explicitly so this
108+
@# check validates the stricter scenario (the macro alone must not satisfy
109+
@# the gate). A.2 is distinct from A.1 because of the include-path
110+
@# (detail/ vs public) and the different gate message.
103111
@if $(CHECK_HEADERS_CXX) -c $(top_srcdir)/test/headers/consumer_detail.cpp -o /dev/null 2>check-headers-A2.log; then \
104112
echo "FAIL: consumer_detail.cpp compiled but should have errored"; \
105113
cat check-headers-A2.log; \
106114
rm -f check-headers-A2.log; \
107115
exit 1; \
108116
fi
109-
@if ! grep -q "$(CHECK_HEADERS_GATE_MSG)" check-headers-A2.log; then \
117+
@if ! grep -q "$(CHECK_HEADERS_DETAIL_GATE_MSG)" check-headers-A2.log; then \
110118
echo "FAIL: consumer_detail.cpp failed but not for the gate reason"; \
111119
cat check-headers-A2.log; \
112120
rm -f check-headers-A2.log; \
@@ -122,7 +130,7 @@ check-headers:
122130
rm -f check-headers-A2b.log; \
123131
exit 1; \
124132
fi
125-
@if ! grep -q "$(CHECK_HEADERS_GATE_MSG)" check-headers-A2b.log; then \
133+
@if ! grep -q "$(CHECK_HEADERS_DETAIL_GATE_MSG)" check-headers-A2b.log; then \
126134
echo "FAIL: consumer_detail_modded.cpp failed but not for the gate reason"; \
127135
cat check-headers-A2b.log; \
128136
rm -f check-headers-A2b.log; \
@@ -146,7 +154,7 @@ check-headers:
146154
rm -f check-headers-A4.log; \
147155
exit 1; \
148156
fi
149-
@if ! grep -q "$(CHECK_HEADERS_GATE_MSG)" check-headers-A4.log; then \
157+
@if ! grep -q "$(CHECK_HEADERS_PUBLIC_GATE_MSG)" check-headers-A4.log; then \
150158
echo "FAIL: consumer_post_umbrella.cpp failed but not for the gate reason"; \
151159
cat check-headers-A4.log; \
152160
rm -f check-headers-A4.log; \

src/http_request.cpp

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -185,19 +185,17 @@ void http_request::set_method(const std::string& method) {
185185
}
186186

187187
const std::vector<std::string>& http_request::get_path_pieces() const {
188-
// TASK-017: lazily populate both caches and return the public-typed
189-
// mirror by const&. All cache-maintenance logic lives inside the impl
190-
// class (code-quality-reviewer-iter1-4 / code-simplifier-iter1-8).
188+
// TASK-017: lazily populate the cache and return it by const&. All
189+
// cache-maintenance logic lives inside the impl class.
190+
// (code-quality-reviewer-iter1-4 / code-simplifier-iter1-8)
191191
impl_->ensure_path_pieces_cached(path);
192-
impl_->ensure_path_pieces_public_cached();
193-
return impl_->path_pieces_public_;
192+
return impl_->path_pieces_cached_;
194193
}
195194

196195
const std::string http_request::get_path_piece(int index) const {
197196
impl_->ensure_path_pieces_cached(path);
198-
if (static_cast<int>(impl_->path_pieces.size()) > index) {
199-
const auto& p = impl_->path_pieces[index];
200-
return std::string(p.data(), p.size());
197+
if (static_cast<int>(impl_->path_pieces_cached_.size()) > index) {
198+
return impl_->path_pieces_cached_[index];
201199
}
202200
return EMPTY;
203201
}
@@ -268,13 +266,11 @@ const http::arg_view_map& http_request::get_args() const {
268266
return impl_->args_view_cached_;
269267
}
270268

271-
const std::map<std::string_view, std::string_view, http::arg_comparator> http_request::get_args_flat() const {
269+
const std::map<std::string_view, std::string_view, http::arg_comparator>&
270+
http_request::get_args_flat() const {
272271
impl_->populate_args();
273-
std::map<std::string_view, std::string_view, http::arg_comparator> ret;
274-
for (const auto& [key, val] : impl_->unescaped_args) {
275-
ret[key] = val[0];
276-
}
277-
return ret;
272+
impl_->ensure_args_flat_view_cached();
273+
return impl_->args_flat_view_cached_;
278274
}
279275

280276
http::file_info& http_request::get_or_create_file_info(const std::string& key, const std::string& upload_file_name) {

src/httpserver/detail/connection_state.hpp

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,14 @@ namespace detail {
6666
// heap. (performance-reviewer-iter1-1.)
6767
// - Overflow spills to the upstream resource (default = heap) silently
6868
// -- it is a correctness fall-through, not a hard limit.
69-
// - TODO(M5): expose ARENA_INITIAL_BYTES via create_webserver if/when
70-
// profiling shows tuning value.
69+
// - ARENA_INITIAL_BYTES stays compile-time intentionally: the buffer is
70+
// an embedded std::array, so making it runtime-sized would require an
71+
// extra heap allocation per connection. The compile-time default is
72+
// sized to cover typical small-GET / small-POST shapes without
73+
// overflow; profiling has not shown a deployed workload where the
74+
// per-connection extra allocation pays for itself. If a future
75+
// deployment needs tuning, replace initial_buffer_ with a
76+
// unique_ptr<std::byte[]> + runtime size carried here.
7177
struct connection_state {
7278
static constexpr std::size_t ARENA_INITIAL_BYTES = 8192;
7379

@@ -81,6 +87,15 @@ struct connection_state {
8187
initial_buffer_.data(), initial_buffer_.size(),
8288
std::pmr::new_delete_resource()};
8389

90+
// Per-connection args DoS limits, copied from webserver::max_args_count
91+
// / max_args_bytes by webserver_impl::connection_notify at
92+
// MHD_CONNECTION_NOTIFY_STARTED. populate_args() reads these from the
93+
// socket_context to set up the per-request arguments_accumulator. A
94+
// value of 0 means "use the arguments_accumulator compile-time
95+
// default", matching create_webserver's sentinel convention.
96+
std::size_t max_args_count = 0;
97+
std::size_t max_args_bytes = 0;
98+
8499
connection_state() = default;
85100
connection_state(const connection_state&) = delete;
86101
connection_state& operator=(const connection_state&) = delete;

src/httpserver/detail/http_request_impl.hpp

Lines changed: 37 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,7 @@ class http_request_impl {
118118
#ifdef HAVE_DAUTH
119119
digested_user(alloc),
120120
#endif // HAVE_DAUTH
121-
unescaped_args(alloc),
122-
path_pieces(alloc)
121+
unescaped_args(alloc)
123122
#ifdef HAVE_GNUTLS
124123
, client_cert_dn(alloc),
125124
client_cert_issuer_dn(alloc),
@@ -183,24 +182,22 @@ class http_request_impl {
183182
#endif // HAVE_DAUTH
184183
mutable std::pmr::map<std::pmr::string, std::pmr::vector<std::pmr::string>,
185184
http::arg_comparator> unescaped_args;
186-
mutable std::pmr::vector<std::pmr::string> path_pieces;
187185
mutable bool args_populated = false;
188-
mutable bool path_pieces_cached = false;
189186
#ifdef HAVE_BAUTH
190187
// Guard for fetch_user_pass(): once the MHD round-trip has been made
191188
// (whether or not the request carries a Basic-Auth header), further
192189
// calls to get_user()/get_pass() skip the MHD call entirely.
193190
// Using a dedicated boolean (rather than checking username.empty())
194-
// matches the args_populated / path_pieces_cached pattern and avoids
195-
// a redundant MHD call on requests with no auth credentials where the
196-
// credential strings remain empty after the first fetch.
191+
// matches the args_populated pattern and avoids a redundant MHD call
192+
// on requests with no auth credentials where the credential strings
193+
// remain empty after the first fetch.
197194
// (code-simplifier finding #6 / major review item)
198195
mutable bool user_pass_fetched = false;
199196
#endif // HAVE_BAUTH
200197
// Cache guard for get_requestor(). Using a dedicated boolean (rather
201198
// than checking requestor_ip.empty()) is consistent with the boolean-
202-
// flag pattern used by args_populated and path_pieces_cached, and is
203-
// robust if the connection layer ever returns an empty IP string.
199+
// flag pattern used by args_populated, and is robust if the connection
200+
// layer ever returns an empty IP string.
204201
// (code-simplifier finding #7 / minor review item)
205202
mutable bool requestor_ip_cached = false;
206203

@@ -211,7 +208,7 @@ class http_request_impl {
211208
// and reused on subsequent calls.
212209
//
213210
// Allocator note: the public container types embed std::string_view
214-
// (header_view_map / arg_view_map) or std::string (path_pieces_public_)
211+
// (header_view_map / arg_view_map) or std::string (path_pieces_cached_)
215212
// and use the default allocator. They cannot be made PMR without
216213
// changing the public surface (TASK-017 plan, "Storage strategy").
217214
// The first call therefore allocates on the global heap; subsequent
@@ -231,24 +228,27 @@ class http_request_impl {
231228
mutable http::arg_view_map args_view_cached_;
232229
mutable bool args_view_cache_built_ = false;
233230

234-
// Public-typed mirror of `path_pieces` (the pmr::vector<pmr::string>
235-
// already kept above). Two caches in lockstep: the pmr version stays
236-
// arena-friendly for any future internal consumer (e.g. radix-tree
237-
// route matching that must allocate on the per-connection arena);
238-
// the public version is what get_path_pieces() returns by const&.
231+
// PRD-REQ-REQ-001 / Item 19 / Item 24: cached "first value per key"
232+
// view used by get_args_flat(). Aliases the pmr-backed unescaped_args
233+
// storage via string_view, so it shares the request's lifetime.
234+
// Avoids the O(n log n) reconstruction the by-value form paid on
235+
// every call.
236+
mutable std::map<std::string_view, std::string_view, http::arg_comparator>
237+
args_flat_view_cached_;
238+
mutable bool args_flat_view_cache_built_ = false;
239+
240+
// Tokenised path pieces. Single cache populated lazily on the first
241+
// get_path_pieces() / get_path_piece(int) call. Stored as
242+
// std::vector<std::string> so get_path_pieces() can return it by
243+
// const& without an ABI break.
239244
//
240-
// Deliberate dual-cache design (architecture-alignment-checker-iter1-1 /
241-
// code-quality-reviewer-iter1-5 / performance-reviewer-iter1-13):
242-
// - The pmr::vector<pmr::string> cannot be returned as const
243-
// vector<string>& without an ABI change, so the public mirror
244-
// exists as a forward-compatible layer.
245-
// - No current internal consumer uses path_pieces post-PIMPL;
246-
// if none materialises before TASK-018 lands the two caches can
247-
// be collapsed to a single std::vector<std::string>.
248-
// - TODO(post-018): evaluate collapsing to a single cache if no
249-
// internal arena consumer has emerged.
250-
mutable std::vector<std::string> path_pieces_public_;
251-
mutable bool path_pieces_public_built_ = false;
245+
// The post-PIMPL audit (TASK-018) found no internal arena consumer of
246+
// a pmr-backed path-pieces vector, so the earlier dual-cache design
247+
// (pmr arena copy + public mirror) was collapsed to this single
248+
// heap-allocated cache. If a future radix/route-matching path needs
249+
// arena-allocated pieces, reintroduce the pmr cache alongside.
250+
mutable std::vector<std::string> path_pieces_cached_;
251+
mutable bool path_pieces_cache_built_ = false;
252252

253253
// TASK-057: when true, http_request::operator<< streams credential
254254
// material verbatim (v1 verbose form). Default false: the four
@@ -290,22 +290,20 @@ class http_request_impl {
290290
// the same reference in O(1).
291291
const http::header_view_map& ensure_headerlike_cache(MHD_ValueKind kind) const;
292292
void populate_args() const;
293+
// Populates path_pieces_cached_ from the tokenised request path on
294+
// first call. Subsequent calls are O(1) and zero-allocating.
293295
void ensure_path_pieces_cached(std::string_view path) const;
294296

295-
// TASK-017: populates the public-typed mirror of path_pieces and marks
296-
// path_pieces_public_built_. Called from get_path_pieces() after
297-
// ensure_path_pieces_cached() so all cache-maintenance logic for the
298-
// public mirror lives inside the impl class (analogous to
299-
// ensure_headerlike_cache / ensure_path_pieces_cached).
300-
// (code-quality-reviewer-iter1-4 / code-simplifier-iter1-8)
301-
void ensure_path_pieces_public_cached() const;
302-
303297
// TASK-017: populates the arg view-map cache from unescaped_args.
304298
// Called from get_args() so the build loop lives inside the impl
305299
// class, keeping all cache-maintenance code in one place.
306300
// (code-simplifier-iter1-9)
307301
void ensure_args_view_cached() const;
308302

303+
// Populates args_flat_view_cached_ from unescaped_args, picking the
304+
// first value for each key. Called from get_args_flat().
305+
void ensure_args_flat_view_cached() const;
306+
309307
void set_arg(const std::string& key, const std::string& value, std::size_t content_size_limit);
310308
void set_arg(const char* key, const char* value, std::size_t size, std::size_t content_size_limit);
311309
void set_arg_flat(const std::string& key, const std::string& value, std::size_t content_size_limit);
@@ -347,9 +345,10 @@ class http_request_impl {
347345
//
348346
// Defaults are deliberately generous (64 unique keys / 64 KiB total bytes)
349347
// so existing callers that construct the accumulator without setting these
350-
// fields remain compatible. The webserver hot path sets these from
351-
// connection_state or a compile-time constant once the create_webserver
352-
// API exposes them (TODO(M5)).
348+
// fields remain compatible. Operators can override via
349+
// create_webserver::max_args_count() / max_args_bytes(); populate_args()
350+
// reads the per-connection override from connection_state when present
351+
// and falls back to these compile-time defaults otherwise.
353352
//
354353
// NOTE: POST argument limits are handled upstream by MHD_OPTION_CONNECTION_
355354
// MEMORY_LIMIT; the guards here apply only to GET arguments processed via

src/httpserver/detail/modded_request.hpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,12 @@
1818
USA
1919
*/
2020

21-
// TASK-014: this detail header uses a dual-mode gate (_HTTPSERVER_HPP_INSIDE_ OR
22-
// HTTPSERVER_COMPILATION) because webserver.hpp (a public header) still
23-
// transitively includes it. Once TASK-014 lands the PIMPL split removing that
24-
// transitive include, tighten this gate to HTTPSERVER_COMPILATION-only.
25-
#if !defined (_HTTPSERVER_HPP_INSIDE_) && !defined (HTTPSERVER_COMPILATION)
26-
#error "Only <httpserver.hpp> or <httpserverpp> can be included directly."
21+
// ADR-002 / TASK-014: PIMPL split removed the transitive include of
22+
// modded_request.hpp from public headers (webserver.hpp now only forward-
23+
// declares detail::modded_request). The gate is tightened to
24+
// HTTPSERVER_COMPILATION-only, matching http_endpoint.hpp.
25+
#if !defined(HTTPSERVER_COMPILATION)
26+
#error "httpserver/detail/modded_request.hpp is internal; only include it when compiling libhttpserver (HTTPSERVER_COMPILATION must be defined)."
2727
#endif
2828

2929
#ifndef SRC_HTTPSERVER_DETAIL_MODDED_REQUEST_HPP_

0 commit comments

Comments
 (0)