allthingssecurity commented on PR #29203: URL: https://github.com/apache/camel/pull/29203#issuecomment-6652229633
Thanks Claus. All four points are done in 9b6807ee0a2a. **Double copy (DefaultUnitOfWork / UnitOfWorkHelper).** `beforeRouteSynchronizations`, `afterRouteSynchronizations` and `doneSynchronizations` each get an overload with a `boolean copy` (`@since 4.24`). With `false`, the helper reverses and sorts the caller's list in place instead of copying it. `DefaultUnitOfWork.beforeRoute`, `afterRoute` and `done` pass their copy taken under the lock with `false`. That makes one list allocation per call instead of two, and the existing 4-argument methods behave as before. I kept the lock on the before/after route path, the option you described as "or reorder once under the lock", because the parts of a parallel split call `beforeRoute`/`afterRoute` on the shared unit of work while other parts add or hand over. An unlocked `new ArrayList<>(list)` of an `ArrayList` that another thread grows or shrinks can copy a `null` slot (and then NPE on `getRouteSynchronization()`) or skip an entry. The lock is uncontended on a normal exchange, and a unit of work with no synchronizations still takes neither the lock nor a copy. I did not benchmark it, so I have no numbers. The CME race stays covered: with the old unlocked `handoverSynchronization`, `SplitParallelShareUnitOfWorkHandoverTest` still fails 2 of 3 with the `ConcurrentModificationException`, and it passes with this change. **Two fields (AbstractExchange).** They are now one field, `SharedUnitOfWork sharedUnitOfWork`: a private holder for the shared unit of work and the on completions the part added (that list is still created lazily). It is created only by `setSharedUnitOfWork`, and by the copy constructors for a copy of such a part, which gets its own empty holder. Every other exchange carries one null field. **Route `onCompletion` reached inside a part.** Yes, this split is intended. The JIRA proposal states it, and the reasons are: - `UnitOfWork.addSynchronization(Synchronization)` has no exchange argument. With a shared unit of work, the parent and every part add to the same list, so a direct addition, like the one `OnCompletionProcessor` makes around line 216, cannot be attributed to a part. Only `ExchangeExtension.addOnCompletion` is called on the part itself. So on 4.14-4.23 it did not go with "its" seda copy: the first part to reach seda took every route `onCompletion` registered so far, by the parent and by the other parts. That is the same defect as the consumer commit. - With `shareUnitOfWork`, a route `onCompletion` reached in a part that does not send to seda stays on the shared unit of work and is done with it when the split is done (4.14 contract). Keeping it with the parent when the part does send to seda means the timing and outcome of that `onCompletion` no longer depend on whether the part's path has an InOnly seda send. - What a part adds through `addOnCompletion` is different: these are resources tied to what the copy carries (a file polled by `pollEnrich`, the part's stream cache, an idempotent key). They still follow the copy, as in CAMEL-9291 and as before 4.14. If you would rather keep the old behaviour for route `onCompletion` only, it is a one-line change: `OnCompletionProcessor` would register through `exchange.getExchangeExtension().addOnCompletion(...)`, which is the same call for any exchange that is not a shared part. I left it out for the consistency reason above, but I am happy to switch. A new test, `testOnCompletionAddedToTheUnitOfWorkStaysWithTheParent`, pins the current behaviour: a synchronization added to the shared unit of work directly, before the part sends to seda, runs once, for the parent. The upgrade guide now says this explicitly, including that before it went with the seda copy. **Upgrade guide.** The text now reads "this is unchanged from earlier versions", and it says the "get the content first" case mainly hits Multicast and Recipient List, where each part carries the whole message body. Also done: `<p/>` is now `<p>` in the `ExchangeExtension.setSharedUnitOfWork` javadoc (the only one in the new javadoc). Tests: `ShareUnitOfWorkSedaHandoverTest` (12) and `SplitParallelShareUnitOfWorkHandoverTest` (3) pass, also 20 runs in one JVM, 300/300. camel-core `*UnitOfWork*`, `*OnCompletion*`, `Split*`, `Multicast*`, `RecipientList*`, `Seda*`, `*Exchange*Test`: 961 tests, 1 failure, `SedaConfigureTest.testConfigureIgnoreCaseLocaleIndependent`, which is a local locale issue that fails on main too. The formatter of the build made no changes. _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]
