allthingssecurity commented on PR #29664:
URL: https://github.com/apache/camel/pull/29664#issuecomment-6821384471
Thanks for the careful review. Changes are in d6b4c8ec94c6.
1. **Pooled exchanges.** Your trace of `Commits` is right for a reused unit
of work. But R2 never sees a pooled consumer exchange: the plugin decides at
context init, and with `exchangeFactory=pooled` the sanitizer stays off with a
WARN (`SanitizerContextServicePlugin.java:137`, and "It does not support pooled
exchanges" in sanitizer.adoc). camel-main's `exchangeFactory=pooled` sets both
factories when it configures the context, before it starts
(`SanitizerActivationTest.offWithPooledExchanges` already covers the consumer
factory). The check missed one case: a pooled *processor* exchange factory set
on its own (pooled copies of the EIPs also keep a reset unit of work). The
sanitizer now stays off for that too, and the docs say why: a pooled exchange
keeps its unit of work for its next message, and R2 follows commits by unit of
work. New `SanitizerPooledExchangesTest`:
- with the pooled factories, a consumer that adds the same commit to each
message of one pooled exchange (same exchange instance, same unit of work, 3
messages, the commit runs 3 times) runs with the sanitizer off;
- a pooled processor factory alone keeps it off (this one fails before
the change);
- control: the same context without pooling gets the sanitizer.
I did not add the fresh-`Place` re-keying to `Commits`, since the plugin
never installs R2 for pooled exchanges and the code could not be reached. If
pooled exchanges are ever supported, that is the place to start, with a test on
the pooled factory.
2. **Cost and the default.** I ran the JMH before merge, in the
camel-performance-tests style of #162, as a new benchmark
(`SanitizerCommitTest`, kept local for now; I can add it to #162). Setup: a
commit added to each message before routing; `pendingTimeout=0` in every mode,
so R1's table is not measured; 2 forks, 3 × 2 s warmup, 5 × 2 s measurement,
average ns/op; JDK 21 on an Apple M4 Pro (12 cores) with a load of about 8-10
from other work. Modes: `off` = camel-sanitizer on the classpath with
`enabled=false`; `callback` = R1 only; `commit` = `callback,commit`. Routes:
`steps` = 6 steps, the commit runs when the exchange is done; `seda` = 2 steps,
then an `InOnly` send to `seda` (the commit is handed over to the copy), 2
consumers.
| stackDepth 0 | off | callback | callback,commit |
|---|---|---|---|
| steps, 1 thread | 960 ± 20 | 1160 ± 36 | 2163 ± 74 |
| seda, 1 thread | 1065 ± 33 | 1164 ± 35 | 2959 ± 197 |
| steps, 4 threads | 2493 ± 214 | 1897 ± 433 | 5794 ± 151 |
| seda, 4 threads | 3280 ± 118 | 3385 ± 104 | 13002 ± 646 |
With `stackDepth=32` (1 thread): steps 63742 (callback) vs 76713
(callback,commit), seda 36546 vs 49771. There, taking stacks dominates, and the
stacked #26812 roughly halves that part.
So with no stacks, R2 roughly doubles the cost per message of R1 on one
thread (about +1 µs steps, +1.8 µs with the seda handover). With 4 threads it
is 3-4 times R1 (+3.9 µs, +9.6 µs), as you expected from the single monitor of
`Commits` (plus the exchange events that the done notifier turns on). Following
the numbers, as you suggested, the default is now `callback`. R2 is opt-in with
`checks=callback,commit`, and stays on by default only in a test
(`@CamelSanitizer` or the CI switch). There the cost per message does not
matter and R2's reports are the point: it is what found the topic load balancer
bug below. The race-hunting advice says to use `checks=callback` there too.
camel-main no longer publishes a default for `checks`, so the plugin decides
(`@Metadata(defaultValue = "callback")`, and the Javadoc names the test
default). `SanitizerActivationTest` covers it: R1 only with `enabled=true` and
with profile `dev`; both rules in a test; both with `checks=callback,commit`. T
he design text no longer claims the run: step 3 now points to "Cost", which
has these numbers. Striping `Commits` as #29528 did for the pending calls would
be the way to bring R2 closer to R1 if it should later be on by default with
the dev console step; I left that for then.
3. **Race-hunting advice.** The camel-main Javadoc of
`setPendingTimeout`/`withPendingTimeout`, and so `main.adoc` and the metadata
(regenerated; the catalog copies are identical), now give the same advice as
sanitizer.adoc: when hunting races between parts, use `pendingTimeout=0` and
leave `commit` out of `checks` (`checks=callback`). Both places now say that R2
takes its lock at each unit of work given, each add, handover and run of an on
completion, and each exchange done.
4. **TopicLoadBalancer.** You were right; it is a real bug, not a false
report. `copyExchangeStrategy` uses `exchange.copy()`, which keeps the unit of
work of the exchange with a new id, so the first copy sent `InOnly` to `seda`
handed over the consumer's commit. The commit then ran when that seda copy was
done, with the copy's outcome: the CAMEL-26576 bug in another EIP. It was
small, so it is fixed in this PR: the copy now shares the unit of work through
`setSharedUnitOfWork` (as the parts with `shareUnitOfWork` do), so it hands
over only the on completions it added itself. New tests:
- `TopicLoadBalancerSedaHandoverTest`, without the sanitizer: the commit
runs once, with the exchange, on failure when a later endpoint fails and on
completion when all succeed; and the copy's own on completion still goes with
its seda copy (control).
-
`SanitizerCommitTest.theCommitOfAnExchangeWhoseTopicCopyIsSentToSedaRunsOnceWithTheExchange`:
R2 reports nothing, and the commit ran with route `topic`.
Before the fix, the first two fail (the commit ran with the seda copy),
and the R2 test fails (the commit ran with route `sedaA`, and R2 reports
`COMMIT_HANDED_OVER_BY_OTHER_EXCHANGE`). The 4.24 upgrade guide entry of
CAMEL-26576 gets a paragraph for the topic load balancer. FailOver copies keep
the exchange id (`createCopy(exchange, true)`), so a handover there is a
handover by the owner, as before.
5. **gnodet-bot threads.** `contextStopped()` and `stopPendingCheck(...)`
are now in a `try`/`finally`, so the pending calls are still listed if a report
fails. The finds over `taken`/`ran` have a comment at the call site: they are
linear in `MAX_FOLLOWED`/`MAX_RAN`, only for an on completion whose identity
hash is indexed, and raising those bounds much needs an index by identity.
Replied on both threads.
On your other two answers: agreed. Never-run at context stop stays a
violation, and the end-of-unit-of-work event can wait for the dev console step.
Tests: the sanitizer tests of camel-core (223, with the new classes and two
new activation cases), camel-management, camel-main and camel-test-main-junit6,
plus the load balancer and `shareUnitOfWork` tests of camel-core, pass without
and with `-Psanitizer`. The new and changed classes ran 1340/1340 over 20 runs
in one JVM. With 8482ed211f65's `TopicLoadBalancer` and plugin, the two load
balancer tests, the R2 topic case and the pooled-processor case fail. The
record logic of `Commits` is unchanged (only a comment), so I did not re-run
the TLA+ model.
The description and the design are updated.
_Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
