Skip to content

Commit 6c8f1e7

Browse files
etrclaude
andcommitted
test(helgrind): re-anchor benign test-fixture race suppressions
The valgrind-helgrind lane went red on basic/file_upload/deferred after the DR-014 dispatch decomposition. Every surviving report is a benign false positive of the same family: an MHD worker thread writes test-fixture or lazy-cache state that the main thread published via the route-table shared_mutex at register_path() and reads back only after the HTTP round-trip completes. Helgrind runs with --history-level=approx on this lane and cannot observe the happens-before that flows through the socket, so it flags the worker write against the main-thread construction/reset. Each test is single-request (littletest is sequential), so there is no real concurrent access; TSan and DRD (both precise) are green on all three. The decomposition did not introduce any race -- it split dispatch into its own TUs (request_dispatcher.cpp / request_pipeline.cpp), and the changed inlining lifted these write frames above the pre-existing per-symbol suppressions: * basic: print_request_resource / print_response_resource stream a request/response into a main-stack std::stringstream via httpserver::operator<<; the top frame is now __ostream_insert / _M_insert / xsputn, past fun:*basic_stringstream*. * basic: http_resource::get_allow_header caches the `Allow:` string under its own shared_mutex (exclusive-lock writer, double-checked); the constructor's default-init on the main thread is the only unsynchronised touch and it happens-before publication. Same benign class as the existing get_allowed_methods / method_set entries. * file_upload: print_file_upload_resource::render_post assigns a std::string member (content = req.get_content()); top frame char_traits::assign, past fun:*basic_string*. * deferred: the deferred-body producer increments a file-scope `counter` in test_callback on the worker, reset by the main thread between tests. Re-anchor each on the exact test-fixture handler symbol (never a libhttpserver locking frame), so a genuine library race stays unsuppressed per DR-008. Suppression-file-only change; helgrind is Linux/valgrind-only and validated in CI. memcheck and drd are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015tAodxYJMEY4VxCX4dk62e
1 parent 4ae4fd9 commit 6c8f1e7

1 file changed

Lines changed: 69 additions & 0 deletions

File tree

test/valgrind-helgrind.supp

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,75 @@
251251
fun:*resourceD*Ev
252252
}
253253

254+
# --- test-fixture handler state written by an MHD worker, synced to main ------
255+
# via the HTTP round-trip (approx-history publication false positive) ----------
256+
# NOT libhttpserver races (DR-008). Each entry is a TEST resource whose render_*
257+
# handler runs on an MHD worker thread and writes fixture-owned state that the
258+
# main thread created before start()/register_path and inspects only AFTER
259+
# curl_easy_perform() returns:
260+
# * print_request_resource / print_response_resource -> a std::stringstream
261+
# that lives on the main thread's stack (basic.cpp), written via
262+
# httpserver::operator<<(ostream&, http_request/http_response) inside the
263+
# handler and read back with ss.str() after the request completes;
264+
# * print_file_upload_resource -> a std::string member of the resource itself
265+
# (content = req.get_content(), file_upload.cpp);
266+
# * deferred's test_callback -> a file-scope `counter` incremented by the
267+
# deferred-body producer on the worker and reset by the main thread between
268+
# tests (deferred.cpp).
269+
# The happens-before flows through the route-table shared_mutex at registration
270+
# and then the HTTP socket round-trip. Helgrind runs with --history-level=approx
271+
# on this lane and cannot observe the socket edge, so it reports a benign race
272+
# between the worker write and the main-thread construction/reset; each test is
273+
# single-request (littletest runs sequentially), so there is no real concurrent
274+
# access. TSan and DRD, which are precise, are green on all three. Anchored on
275+
# the exact TEST-fixture handler symbol -- never a libhttpserver locking frame --
276+
# so a genuine library race stays unsuppressed. (DR-014 split dispatch into its
277+
# own TUs; the changed inlining lifted these write frames above the pre-existing
278+
# per-symbol entries, so they are re-anchored on the fixture symbol here.)
279+
{
280+
test-fixture-print-request-into-shared-stringstream
281+
Helgrind:Race
282+
...
283+
fun:*print_request_resource*render_get*
284+
}
285+
{
286+
test-fixture-print-response-into-shared-stringstream
287+
Helgrind:Race
288+
...
289+
fun:*print_response_resource*render_get*
290+
}
291+
{
292+
test-fixture-file-upload-render-member-write
293+
Helgrind:Race
294+
...
295+
fun:*print_file_upload_resource*render_post*
296+
}
297+
{
298+
test-fixture-deferred-counter-callback
299+
Helgrind:Race
300+
fun:test_callback*
301+
}
302+
303+
# --- benign lazy cache: http_resource `Allow:` header snapshot ---------------
304+
# http_resource::get_allow_header() lazily builds and caches the `Allow:` header
305+
# under the resource's OWN std::shared_mutex (cached_allow_mutex_): the warm path
306+
# takes a shared lock and only READS the cache; the cold path takes an exclusive
307+
# (unique) lock and is the ONLY writer of cached_allow_mask_ / cached_allow_valid_
308+
# / cached_allow_header_ (double-checked). Every access to those fields is
309+
# mutex-guarded, so worker-vs-worker is fully synchronised; the only unsynchronised
310+
# touch is the constructor's default member-init on the main thread, which
311+
# happens-before the resource is published to the workers through the route-table
312+
# shared_mutex at register_path(). Helgrind's approx history drops that publication
313+
# edge and reports the worker's LOCKED write as racing the main-thread construction.
314+
# Same benign class as get_allowed_methods / method_set above; TSan (precise) is
315+
# green. Named on the exact symbol, never a daemon-level locking frame, so DR-008
316+
# is unaffected.
317+
{
318+
lhs-get-allow-header-lazy-cache
319+
Helgrind:Race
320+
fun:*get_allow_header*
321+
}
322+
254323
# --- libp11-kit atexit/dl_fini mutex teardown (third-party) ------------------
255324
# gnutls pulls in libp11-kit (PKCS#11); its cleanup runs from _dl_fini() at
256325
# process exit and calls pthread_mutex_destroy() on an object Helgrind rejects

0 commit comments

Comments
 (0)