msg: synchronize turbo snapshot readers and complete $! access paths - #7514
msg: synchronize turbo snapshot readers and complete $! access paths#7514jjourdin wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@rgerhards : This is a proposal for the follow-up you asked for on #7482, around d67fd2c. I kept the source lock: two workers can still share one message (async action with the default copyMsg=off, then a call into a queued ruleset), so first-counter publication and the JSON copy need a coherent view. What I dropped is the materialize-on-allocation-failure fallback in MsgDup. A failed counter malloc now fails the duplicate, same as any other MsgDup OOM. Materialize stays a one-way projection into pMsg->json and no longer clears the turbo callbacks. The never-duped path is unchanged: no extra malloc, no atomic, no lock. mmnormalize-turbo-msgdup-share.sh covers that shared-message topology. Happy to change the shape if you would rather keep the fallback or simplify the lock further. |
rgerhards
left a comment
There was a problem hiding this comment.
Thanks for simplifying the materialization state. The locked MsgDup() path is coherent, but this does not yet close the concurrency and subtree correctness issues, so I am requesting changes.
Blocking issues:
-
The lock-free turbo getter can race snapshot overwrite/release.
getJSONPropVal()andmsgGetJSONPropJSONorString()readpMsg->json,turbo_result, and the callback without acquiringpMsg->mutor pinning the snapshot generation. A queued action with the defaultcopyMsg=offshares the samesmsg_tthroughMsgAddRef(); a later turbommnormalizeon the continuing ruleset can take the writer lock, callMsgReleaseTurboResult(), free the old sole-owner snapshot, clear the slots, and attach another snapshot while the queued action is using the old pointer. Writer-only locking does not synchronize the unlocked reader, and pointer-sized plain loads are not sufficient C synchronization. Please either lock the read, or introduce a coherent immutable descriptor with safe lifetime pinning/retirement. -
Subtree JSON access remains unsafe and does not materialize turbo state.
tplToJSON()still callsjsonFind(), receives a borrowed pointer afterjsonFind()unlocks, and then callsjson_object_get(). A concurrent mutation can replace/free the subtree in that interval, and even a retained json-c reference would not make subsequent in-place mutations into a snapshot. Also,jsonFind()never callsmsgMaterializeTurboJSON(), sotemplate(type="subtree" subtree="$!")can see an empty tree for a turbo-only message. Please route this path through an owned deep copy such asmsgGetJSONPropJSON()while holding the mutex, with ownership adjusted so no extrajson_object_get()leaks the copy. -
The new regression test does not cover the claims strongly enough.
- It performs one
MsgDup()per source message; the other branch usesMsgAddRef(), so it cannot exercise two duplicators racing firstturbo_result_refspublication. template(name="tree" type="string" string="%$!%\n")takes the string/materialization path, not the unsafetype="subtree"/tplToJSON()path.- The full-tree oracle checks the line count but uses
grep -q '"num"', so only one of 5,000 lines needs the field. Thousands of{}lines would still pass. - There is no second turbo
mmnormalizeracing a lock-free getter on the same shared message.
- It performs one
Please add deterministic ASan/TSan coverage for:
- two same-message duplicators,
type="subtree" subtree="$!",- a shared-message lock-free getter racing a second turbo normalization/overwrite,
- and an oracle that validates every output record/sequence.
The new turbo_json_ready bookkeeping is useful, but retaining MsgLock() across both jsonDeepCopy() calls means this PR also does not address normal-case copy contention. That performance redesign can be separate, but the lifetime and subtree correctness gaps above should be resolved before merging.
ef35643 to
4b1a650
Compare
|
@rgerhards thanks for the detailed review. The branch now closes all three points. The tests were written first: the ones that target the reported defects (exists, jsonmesg, unset, shared-getter-tsan, shared-set-tsan, subtree-template under TSan, segdisk-lifecycle, and the unit checks for jsonFind/exists/jsonmesg/unset/shared readers) are red on ef35643 and green after the change; msgdup-share, second-normalize, msgdup-concurrent-tsan, message-lifecycle and hup-tsan are regression coverage and pass on both. The two tplToJSON unit checks were written after the fix; the per-field over-reference they guard is argued from the code (msgGetJSONPropJSON returns a deep copy, json_object_object_add takes one reference) and the post-fix run is LSan-clean. 1. Lock-free getter vs snapshot overwrite/release. The fast path in Reproduced before the fix (reports kept): 2. Subtree / jsonFind. Note on coverage: string-passing outputs (omfile, omfwd) render 3. Tests.
Results on Linux/amd64, clang 21, Performance: getter microbenchmark on the plain build (clang 21 -O2, amd64, 5M calls of getJSONPropVal on a turbo-only message, fake snapshot): sole holder (iRefCount == 1) 21.5 ns/call before vs 21.1 after (the gate is one acquire load and a branch, no lock; earlier runs put it within 19-24 ns on both builds, noise level on that 2-vCPU box); shared message, uncontended (refcount 2, one thread) 19.3 ns before vs 27.4 after (the mutex); shared and contended (two readers on one message) 37.6 ns before vs 176.5 after, which is the price of not reading freed memory. For reference the materialized json-c path costs 68-75 ns/call. End to end, turbo mmnormalize + omfile with a five-field template, 200k messages, 3 runs each: direct queue 79-113k msg/s before vs 123-128k after, async action queue (shared message) 70-74k before vs 74-92k after (3 runs each; the spread is run-to-run noise on that 2-vCPU box), no regression. Three things I noticed and left out because they are pre-existing and unrelated to this PR's turbo scope; happy to send them separately: the oversize-message JSON report ( Things worth saying out loud rather than leaving for you to find:
The jsonDeepCopy contention in |
Why:
The turbo field getters read the snapshot slots and pMsg->json without the
message mutex. A message shared by reference with an action queue
(copyMsg=off, the default) can be renormalized by a second turbo
mmnormalize action on the continuing ruleset: the release of the first
snapshot races the action worker inside the getter (use after free, seen
as a SEGV in ln_fast_result_get_string under ASan). Several $! access paths
also never looked at the snapshot: jsonFind (so exists() was false and
type="subtree" templates empty on turbo-only messages), %jsonmesg%
("$!": null), and unset $! / unset $!field (silent no-ops that left the
removed fields readable through the snapshot).
Impact:
Turbo-normalized messages are now safe to share by reference and every $!
access path sees the normalized fields. The never-shared path keeps its
lock-free field read; shared messages take the mutex on reads.
Before:
- getJSONPropVal / msgGetJSONPropJSONorString read turbo_result,
turbo_result_get_str and pMsg->json with plain loads.
- jsonFind, msgGetJSONMESG and msgDelJSON ignored the snapshot.
- tplToJSON subtree borrowed the live tree after jsonFind unlocked and
took an extra reference on the deep copy returned for each field.
After:
- msgTurboGetStr serves the field under pMsg->mut whenever iRefCount > 1
(a second reference can only be published by a thread that holds one,
so iRefCount == 1 proves exclusive access and stays lock-free); the copy
is taken inside the locked region.
- jsonFind, msgGetJSONPropJSON and msgGetJSONMESG materialize the
snapshot under the mutex; msgGetJSONMESG serializes $! under the mutex.
- msgDelJSON releases the snapshot on unset $! and materializes before a
leaf unset. Materialize runs only for $! accesses (msgAddJSON included:
a $/ write holds the global-variables mutex, not the message mutex).
- msgCheckVarExists answers from the snapshot when it serves the field, so
exists() does not force the message onto the JSON path.
- The segmented disk queue codec materializes the snapshot before it
persists the JSON tree (MsgTurboMaterialize), like the classic queue.
- MsgDup copies APPNAME, PROCID and MSGID under the source mutex: their
getters create them lazily under that mutex on a shared message.
- tplToJSON subtree takes an owned deep copy through msgGetJSONPropJSON;
the per-field path hands the deep copy to the object without an extra
reference (one reference per field per message was leaked).
- runtime_unit_turbo_msgdup exercises every path on a fake snapshot,
single-threaded and with MsgAddRef readers racing snapshot replacement
and materialization; it fails on the previous code under ASan (heap use
after free in the getter) and TSan, and also drives tplToJSON directly.
New testbench scripts cover exists(), %jsonmesg%, unset, second normalization merge, subtree templates, the
shared getter against a second normalization, set on a shared message
(including a $/ write), two concurrent MsgDup callers and a segmented
disk queue, each with a per-record oracle.
Technical Overview:
runtime/msg.c: msgTurboGetStr (refcount-gated lock), materialize guarded on
PROP_CEE in the getters, jsonFind/msgGetJSONMESG/msgDelJSON turbo-aware,
MsgTurboMaterialize, MsgDup copy order, locking comment rewritten.
runtime/segdisk_codec.c: materialize before encoding the JSON tree.
runtime/template.c: tplToJSON subtree and field ownership.
tests/unit/turbo_msgdup_test.c, tests/mmnormalize-turbo-*.sh, Makefile.am.
4b1a650 to
9507ee2
Compare
There was a problem hiding this comment.
All reported issues were addressed across 17 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…t source The doc said the projection happens "under the message mutex", which reads either way, and it sits directly below MsgReleaseTurboResult, whose doc tells the caller to hold the mutex. A caller following the neighbour would lock first and then block on a mutex that is created with default attributes and is therefore not recursive. Both call sites today enter without the lock, so this is a trap for the next caller rather than a live bug. Say plainly that the function takes and releases the lock itself. unit/turbo_msgdup_test.c reaches the build only through a check_PROGRAMS inside a conditional, so like the other conditional unit sources it needs an EXTRA_DIST entry to appear in a dist tarball.
|
@jjourdin Solaris is a VM issue by it's provider. looks longer lasting, I'll check that I disable. The kafka instance is also a CI issue, I'll investigage. Both are not merge blockers. |
rgerhards
left a comment
There was a problem hiding this comment.
if possible, it would be good to get around the codecov issues, so that they do not persist. But this is NOT a merge blocker.
The iRefCount == 1 lock-free path in msgTurboGetStr() still raced a locked overwrite from a second turbo mmnormalize on a message shared by MsgAddRef (copyMsg=off). An acquire load of the refcount is not a happens-before with that write. ubuntu_26_tsan failed mmnormalize-turbo-shared-getter-tsan.sh: write of turbo_result under pMsg->mut, previous read without the mutex. ASan can hit the same window as a use-after-free in ln_fast_result_get_string. Always take pMsg->mut for the json == NULL guard, the slot loads, the get_str callback, and the copy out of the snapshot. The mutex is not recursive; today's callers enter unlocked. Signed-off-by: Jérémie Jourdin <jeremie.jourdin@advens.fr>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8987c992c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - MsgGetRcvFromProp returns fromhost, fromhost-ip, or fromhost-port with the stored prop length; it resolves DNS only when NEEDS_DNSRESOL is still set. | ||
| - MsgDup locks the source message while coherently copying JSON state and the lazy turbo snapshot callbacks. | ||
| - Turbo parse snapshots are immutable and may be shared by MsgDup copies through an atomic reference count; each owner must release only through MsgReleaseTurboResult while holding the message lock or exclusive access. | ||
| - Turbo snapshots are write-once at attach and immutable afterwards. Attach, overwrite, release and every read from a shared message (iRefCount > 1) happen under the message mutex; a sole holder reads the snapshot without the lock (msgTurboGetStr). Materialize is an idempotent one-way projection into pMsg->json under the message mutex and does not clear turbo callbacks; jsonFind, msgGetJSONMESG and msgDelJSON materialize too, and unset $! releases the snapshot. |
There was a problem hiding this comment.
Document that every turbo getter takes the mutex
This entry says a sole holder reads the snapshot without locking, but the committed msgTurboGetStr() now unconditionally calls MsgLock(pMsg). Because this map is the locking contract intended for module authors, retaining the obsolete optimization can lead consumers to make incorrect concurrency and performance assumptions; update it to match the always-locked implementation.
AGENTS.md reference: runtime/AGENTS.md:L48-L49
Useful? React with 👍 / 👎.
| for f in "$RSYSLOG_DYNNAME.spool"/turbo-segdisk*; do | ||
| [ -e "$f" ] && segfiles=$((segfiles + 1)) | ||
| done |
There was a problem hiding this comment.
Wait for segmented queue materialization before inspecting files
On a loaded runner, successful completion of tcpflood only proves that the client finished sending; rsyslog may not yet have processed a message through the asynchronous segmented-disk action. This loop can therefore observe no matching path and fail even though the queue is functioning correctly. Wait for the queue directory or another queue-specific readiness condition before asserting its existence.
AGENTS.md reference: tests/AGENTS.md:L101-L104
Useful? React with 👍 / 👎.
Summary (non-technical, complete)
Turbo snapshot duplication must stay correct when two workers share one
message, without paying a JSON build on every in-memory copy. The lock
added on the previous snapshot-sharing change is sound, but mixing a
materialize-on-allocation-failure fallback into that path made the
ownership model harder to reason about than it needs to be.
References
Refs: #7482
Notes (optional)
In-memory MsgDup still shares the snapshot. A failed counter allocation
now fails the duplicate instead of building JSON.
Before: MsgDup published the lazy refcount under the source lock and, if
that malloc failed, materialized so the copy could keep fields through a
JSON deep copy.
After: MsgDup still locks to publish the counter and copy JSON.
Materialize is an idempotent one-way projection into pMsg->json and no
longer clears turbo callbacks.
Test: mmnormalize-turbo-msgdup-share.sh (async $! materialize via
MsgAddRef plus a call into a queued ruleset). ASan/LSan and TSan clean
on that test and on mmnormalize-turbo-message-lifecycle.sh.
Commit message has Why / Impact / Before / After / Technical Overview.