gnodet-bot commented on code in PR #27990:
URL: https://github.com/apache/camel/pull/27990#discussion_r4301865352
##########
components/camel-mail/src/main/java/org/apache/camel/component/mail/MailConsumer.java:
##########
@@ -266,8 +278,32 @@ public String toString() {
// process the exchange
processExchange(exchange);
}
+ }
- return total;
+ /**
+ * Gives back exchanges that are not routed: removes the key of each mail
from the idempotent repository (where
+ * {@link #isValidMessage(String, Message)} added it when the mail was
retrieved), so the mail is not skipped as a
+ * duplicate later, and releases the exchange.
+ */
+ private void releaseNotRouted(Queue<Exchange> notRouted) {
+ Exchange exchange;
+ while ((exchange = notRouted.poll()) != null) {
+ removeIdempotentKey(exchange.getProperty(MAIL_MESSAGE_UID,
String.class));
+ releaseExchange(exchange, true);
Review Comment:
⚠️ **`releaseExchange` not guarded in cleanup loop** — if it throws a
`RuntimeException` (e.g. a pooled exchange factory error), the `while` loop
terminates and all remaining exchanges in the queue are skipped: their
idempotent keys leak, exactly the bug this PR is fixing. `removeIdempotentKey`
is already guarded with `try/catch`; `releaseExchange` should be too:
```suggestion
removeIdempotentKey(exchange.getProperty(MAIL_MESSAGE_UID,
String.class));
try {
releaseExchange(exchange, true);
} catch (Exception e) {
LOG.warn("Cannot release an exchange that is not routed:
{}.", e.toString());
}
```
##########
components/camel-mail/src/main/java/org/apache/camel/component/mail/MailConsumer.java:
##########
@@ -266,8 +278,32 @@ public String toString() {
// process the exchange
processExchange(exchange);
Review Comment:
💡 **Residual key leak for the exchange that `processExchange` throws on** —
that exchange is already polled from the queue (two lines above), so
`releaseNotRouted` in the `finally` block won't see it. Its idempotent key
stays in the repository until the repository is cleared externally. Test 3
(`keysOfTheRestOfTheBatchAreRemovedWhenRoutingAnExchangeThrows`) explicitly
asserts this: `assertEquals(new TreeSet<>(routed), repo.keys())` — the key of
the throwing exchange stays. This is the same gap @davsclaus flagged, and the
PR closes the *rest-of-batch* part of it. The *current-exchange* part could be
addressed by catching the exception in `routeBatch` itself, removing the key,
and re-throwing — but that is a separate, minor case (only unchecked exceptions
escaping `processExchange` reach here).
--
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]
