allthingssecurity commented on PR #29528: URL: https://github.com/apache/camel/pull/29528#issuecomment-6762578335
Thanks for the review. The smaller points are in 39a5da11b25b. On the design gates, here is what exists and what does not. **Design gates** - **Order of work.** Yes, this is step 6 ahead of R2, R3 and the console. It needs only step 2 (#29409, merged). It does not use the on completion SPI of step 3 (#29436, merged) or the console. If you would rather keep to the design order, this PR can wait until R2 and the console are in. - **TLA+ model.** The model the design asks for (a first callback, a second callback, a throw and an overlap on different threads) is the model of R1's per-call record, `SanitizerR1.tla`. Its result is in the description of #29409: TLC found no error in 789 distinct states, a mutant without the lock is caught, and each kind of report is reachable. There is **no** model of this PR's pending table (`PendingCalls`). Each of its operations runs entirely under one monitor, so an interleaving is a sequence of whole operations. The deterministic tests drive those sequences, with a test-controlled clock, and the 8-thread test leaves nothing pending. If you want a model of the table first, I can add it before merge. - **JMH with the sanitizer on.** It has **not** been run. The only JMH run so far is step 1's (#29374): with no listener this branch measured 1517 ns/message against 1526 on main, and one no-op listener added about 34 ns per step. I agree it should run before merge: the same route, with the sanitizer off and on (`pendingTimeout=0` and `60s`, `stackDepth` 0 and 32), on 1 and N threads so contention on the table's monitor shows. I can run it and post the numbers here before merge. - **The checker holding the monitor.** `stalled()` only loops over the exchanges in the table and compares two fields of each. The warning text, the stacks and the processor lookups are built after the lock is released. A rough single-thread probe (not JMH) gave about 80 µs per scan with 10 000 pending calls, once per period (the timeout, at most 1 s). It also gave about 45 ns for a call, return and callback through the listener. Taking a snapshot under the lock would cost the same as the scan, so I left the scan as it is. Your point about masking races stands: every step event of the context takes the same monitor, which adds happens-before edges between routing threads. `pendingTimeout=0` removes the table and its lock (R1's own record locks only its call). If the N-thread JMH run shows the monitor matters, the next step is to stripe the table by exchange id. That is a bigger change than this PR, so I would do it only on that evidence. **Smaller points (39a5da11b25b)** - `checkPending` now catches `Throwable`. The first failure is logged at WARN, later ones at DEBUG, and the next period checks again. The shared checker thread is guarded by a `private static final Object` lock instead of `Sanitizer.class`. A new test, `aCheckThatFailsWithAnErrorDoesNotStopTheLaterChecks`, makes the table's clock throw an `AssertionError` twice and checks that a later check still reports the stuck call. - Docs, "Still pending": next to the `ProducerTemplate` case, the docs now list a request-reply `to` over `seda`/`vm` and an `aggregate` whose completion runs in the caller. Both wait for an exchange copy of their own, so they are reported when the other side is slow. - Docs: a chaos `delay` on a step (`step=`) runs before the step is called. It shows at the enclosing split or `to`, and not at all at the top level. An `endpoint=` delay runs inside the send, so it shows at that `to`. - `PendingCalls` Javadoc: the part ids are outside the 10 000 bound, and an id goes when its part is done or its exchange has no pending call left. Tests: the sanitizer tests of camel-core, camel-management, camel-main and camel-test-main-junit6 pass with and without `-Psanitizer`: 73 tests, 0 failures each run. The catalog copy of the docs is identical to the source. I updated the PR description to match. _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]
