quantranhong1999 commented on code in PR #3200:
URL: https://github.com/apache/james-project/pull/3200#discussion_r4090506861
##########
mailbox/postgres/src/test/java/org/apache/james/mailbox/postgres/DeleteMessageListenerContract.java:
##########
@@ -311,6 +311,27 @@ void deleteMessageShouldCleanUpThreadData() throws
Exception {
});
}
+ @Test
+ void
deleteIfUnreferencedShouldNotDeleteBlobWhenConcurrentReferenceExists() throws
Exception {
+ MessageManager.AppendResult appendResult =
inboxManager.appendMessage(MessageManager.AppendCommand.builder()
+
.build(ClassLoaderUtils.getSystemResourceAsByteArray("eml/emailWithOnlyAttachment.eml")),
session);
+ PostgresMessageId messageId = (PostgresMessageId)
appendResult.getId().getMessageId();
+ BlobId messageBodyBlobId =
postgresMessageDAO.getBodyBlobId(messageId).block();
+
+ // Simulate concurrent reference by copying message to other mailbox
+ mailboxManager.copyMessages(MessageRange.all(), inboxManager.getId(),
otherBoxManager.getId(), session);
+
+ // Attempt conditional delete - should not delete row nor return
blobId because of concurrent reference
+ Optional<BlobId> deletedBlobId =
postgresMessageDAO.deleteIfUnreferenced(messageId).block();
Review Comment:
Hmm, I am not sure if this actually reproduces your reported race condition
issue.
##########
mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/DeleteMessageListener.java:
##########
@@ -173,11 +168,19 @@ private Mono<Void>
handleMessageDeletion(PostgresMessageDAO postgresMessageDAO,
return postgresMessageDAO.retrieveMessage(messageId)
.flatMap(messageRepresentation ->
dispatchMessageContentDeletionEvent(mailboxId, owner, mailboxACL, flags,
messageRepresentation, mailboxPath)
.thenReturn(messageId))
- .filterWhen(msgId -> isUnreferenced(msgId,
postgresMailboxMessageDAO))
- .flatMap(msgId -> deleteBodyBlob(msgId, postgresMessageDAO)
- .then(deleteAttachmentIfEnabled(messageId, attachmentDAO))
- .then(threadDAO.deleteSome(owner, messageId))
- .then(postgresMessageDAO.deleteByMessageId(messageId)));
+ .flatMap(msgId -> deleteMessageDataIfUnreferenced(msgId,
postgresMessageDAO, attachmentDAO, threadDAO, owner));
+ }
+
+ private Mono<Void> deleteMessageDataIfUnreferenced(PostgresMessageId
messageId,
+ PostgresMessageDAO
postgresMessageDAO,
+ PostgresAttachmentDAO
attachmentDAO,
+ PostgresThreadDAO
threadDAO,
+ Username owner) {
+ return postgresMessageDAO.deleteIfUnreferenced(messageId)
+ .flatMap(optionalBlobId -> optionalBlobId.map(blobId ->
Mono.from(blobStore.delete(blobStore.getDefaultBucketName(), blobId))
+ .then(deleteAttachmentIfEnabled(messageId, attachmentDAO))
+ .then(threadDAO.deleteSome(owner, messageId)))
+ .orElse(Mono.empty()));
Review Comment:
Concern here: we delete the messageId first; therefore, if blob cleanup
fails later, for example. We can not retry and clean up the remaining data
easily, as the referenced messageId is gone.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
