allthingssecurity commented on PR #26790:
URL: https://github.com/apache/camel/pull/26790#issuecomment-5855483293
@davsclaus you're right, it is real. Fixed in 7d132ed92 (new commit on top,
no force-push).
Reproduced with `testLeftoverTimeoutDoesNotCompleteNewGroupWithoutTimeout`:
`optimisticLocking().completionSize(2).completionTimeout(header("timeout"))`.
`a` (timeout 1 ms) and `b` complete by size. Then `c` without a timeout starts
a new group, and the timeout checker runs.
- On the first commit of this PR it fails: the group `[c]` is completed by
the leftover entry (`expected: <[1]> but was: <[]>` for the repository keys).
- With the `AggregateProcessor` of the PR's base it passes, because
`onCompletion` removes the entry. The timeout map and eviction code on current
`main` is the same as on that base. So the problem only comes from this PR.
The fix follows your suggestion, with one addition. A plain "complete only
if the group has `AGGREGATED_TIMEOUT`" check would drop legitimate timeouts.
`trackTimeout` sets the property on the new exchange, not on the stored group.
`GroupedExchangeAggregationStrategy` stores a new holder exchange without it,
and a strategy that returns the latest exchange stores one that may have no
timeout. So with optimistic locking and a `completionTimeoutExpression` (only
then), `doAggregation` now records the group's timeout on the stored exchange:
the timeout tracked for the new exchange, otherwise the one the group already
had. This is written with the group, under the same compare-and-set.
`onEviction` skips a group without the property, and the leftover entry is
dropped. The repositories already keep `AGGREGATED_TIMEOUT`, since it is used
to restore timeouts on startup (JDBC keeps it explicitly; Hazelcast, JCache and
Redis marshal the properties).
Tests (second commit):
- `testLeftoverTimeoutDoesNotCompleteNewGroupWithoutTimeout`: fails on the
first commit, passes with the second.
- `testGroupKeepsTimeoutWhenLaterExchangeHasNoTimeout`: the latest exchange
(no timeout) becomes the stored one, and the group must still time out by the
first exchange's timeout. It passes on the first commit too, so it guards the
carry-over. With the carry-over removed it fails (`Received message count.
Expected: <1> but was: <0>`).
- `*Aggregat*` in camel-core: 242 tests, 0 failures (5 skipped). The branch
still merges cleanly with `main` (`git merge-tree`).
Limitation: without a lock, a timeout that expires between `trackTimeout`
and the repository update of the same exchange can be missed for a group that
had no timeout before. A new group already has the same window today, when the
eviction finds no group yet. That only matters for a timeout shorter than the
aggregation step itself.
_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]
