gnodet-bot commented on code in PR #28200:
URL: https://github.com/apache/camel/pull/28200#discussion_r4333898778
##########
components/camel-mail/src/main/java/org/apache/camel/component/mail/MailConsumer.java:
##########
@@ -298,6 +321,93 @@ private void releaseNotRouted(Queue<Exchange> notRouted) {
}
}
+ private void openFolder() throws MessagingException {
+ folder.open(Folder.READ_WRITE);
+ folderOpens.incrementAndGet();
+ }
+
+ /**
+ * The mail to mark once its exchange completed. When the folder was
opened again since the mail was retrieved (its
+ * connection dropped while the mail was routed), the mail object of the
poll is stale: for IMAP, the mail is looked
+ * up again by its UID. Returns null when it cannot be found (its UID is
unknown, the UIDVALIDITY of the folder
+ * changed, or it was expunged meanwhile): then no mail is marked, as
marking the stale mail object would mark
+ * another mail, and the idempotent key of the mail is settled as on
commit.
+ */
+ Message mailToCommit(Message mail, Long uid, long uidValidity, int
batchOpen, Exchange exchange) {
+ if (getEndpoint().getConfiguration().getProtocol().startsWith("pop3"))
{
+ // processCommit finds a POP3 mail again by its key
+ return mail;
+ }
+ try {
+ if (!folder.isOpen()) {
+ openFolder();
+ }
+ if (folderOpens.get() == batchOpen) {
+ return mail;
+ }
+ String notFound;
+ Message found = null;
+ if (uid == null || !(folder instanceof UIDFolder uidFolder)) {
+ notFound = "its UID could not be read when it was retrieved";
+ } else if (uidFolder.getUIDValidity() != uidValidity) {
+ notFound = "the UIDVALIDITY of the folder changed from " +
uidValidity + " to " + uidFolder.getUIDValidity();
+ } else {
+ found = uidFolder.getMessageByUID(uid);
+ notFound = "no mail has the UID " + uid + " any more (it was
expunged)";
+ }
+ if (found == null) {
+ LOG.warn("The mail folder {} was opened again after the mail
was retrieved, and the mail cannot be found"
+ + " again: {}. It is not marked as {}.",
+ folder.getFullName(), notFound,
getEndpoint().getConfiguration().isDelete() ? "DELETED" : "SEEN");
+ commitIdempotentKey((String)
exchange.removeProperty(MAIL_MESSAGE_UID));
+ }
+ return found;
+ } catch (MessagingException e) {
+ getExceptionHandler().handleException("Error occurred during
committing mail message: " + mail, exchange, e);
+ return null;
Review Comment:
⚠️ **Bug: idempotent key not settled on `MessagingException`**
When `getMessageByUID` (or `openFolder`) throws `MessagingException`, this
catch block calls `handleException` and returns `null` — but
`commitIdempotentKey` is never called.
Back in the `onComplete` callback, `null` from `mailToCommit` causes
`processCommit` to be skipped too:
```java
Message current = mailToCommit(mail, uid, uidValidity, batchOpen, exchange);
if (current != null) {
processCommit(current, exchange); // skipped when current == null
}
```
So on this path, the idempotent key is neither removed (as
`commitIdempotentKey` would do) nor left for `processCommit` to handle — it
leaks. The mail will be skipped on the next poll.
Consider settling the key here:
```suggestion
return null;
```
Something like:
```java
} catch (MessagingException e) {
getExceptionHandler().handleException("Error occurred during
committing mail message: " + mail, exchange, e);
commitIdempotentKey((String)
exchange.removeProperty(MAIL_MESSAGE_UID));
return null;
}
```
--
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]
