allthingssecurity commented on code in PR #27796:
URL: https://github.com/apache/camel/pull/27796#discussion_r4270612941
##########
components/camel-browse/src/main/java/org/apache/camel/component/browse/BrowseEndpoint.java:
##########
@@ -140,14 +143,22 @@ protected List<Exchange> createExchangeList() {
*/
protected void onExchange(Exchange exchange) throws Exception {
if (browseLimit > 0) {
- // make room if end of capacity
- int max = exchanges.size();
- if (max >= browseLimit) {
- int space = Math.abs(max - (browseLimit - 1));
- exchanges = exchanges.subList(space, max);
+ // add first and then remove the oldest from the list itself: a
sub list is only a view that keeps them
+ // all in memory, and a reader never sees fewer than the limit
once it was reached. Under a lock, as
+ // concurrent producers would otherwise remove too many or
overtake the limit
+ limitLock.lock();
+ try {
+ exchanges.add(exchange);
+ int excess = exchanges.size() - browseLimit;
+ if (excess > 0) {
+ exchanges.subList(0, excess).clear();
Review Comment:
Done in 9175029a0e1e.
- `BrowsableEndpoint.getBrowseStatus` now iterates the list once and takes
size, first and last from that pass (the iterator of a copy-on-write list is a
snapshot). It allocates no array, which matters for seda: `SedaSpanDecorator`
calls `getBrowseStatus` per span, and `SedaEndpoint.getExchanges()` already
returns a new `ArrayList` of the queue.
- `ManagedBrowsableEndpoint`: `browseExchange`, `browseMessageBody`,
`browseMessageAsXml`, `browseMessageAsJSon` and the two range loops read
`getExchanges().toArray(new Exchange[0])` once and index into that array. Index
checks, from/to bounds, order and the null returns are unchanged. `queueSize()`
calls `size()` once and is unchanged.
- Other implementors: mock returns its `CopyOnWriteArrayList` (which
`retainLast` also shrinks), so it gets the same fix. Seda, file/ftp and
jms/activemq build a new list on each call; for them the JMX operations cost
one more array copy of that list, and `getBrowseStatus` costs nothing extra.
- `BrowseDevConsole` reads through `getExchanges(limit, filter)`, whose
default streams the list into a new list for any limit > 0 (default 100, or
`Integer.MAX_VALUE` with fresh size), so I left it as is.
Test: `ManagedBrowsableEndpointSnapshotTest` (camel-management). It uses a
`BrowseEndpoint` with `browseLimit=3` whose `createExchangeList()` returns a
`CopyOnWriteArrayList` subclass. The subclass holds a producer thread right
after its add (four exchanges, before the trim) and lets it finish the trim at
the reader's n-th `get(index)`. It uses latches and bounded joins, no sleeps.
On the PR head:
- `browseMessageBody(3)` threw `ArrayIndexOutOfBoundsException: Index 3 out
of bounds for length 3`
- `getBrowseStatus(0)` threw the same exception at `list.get(list.size() -
1)`
- `browseRangeMessagesAsJSon(0, MAX)` threw nothing but returned B, C, D
after its size check saw four
With this commit all 3 pass. They return D, (4, 1, 4) and A..D, which is the
list as it was between the add and the trim.
Ran on JDK 21: the camel-management tests under
`org.apache.camel.management` (449), the camel-core browse tests including
`BrowseLimitMemoryTest` (14), and camel-console `BrowseDevConsoleTest` and
`ConsoleEdgeCasesTest` (10). All pass, and the formatter/impsort left 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]
