From 3daa8fc8850528795af855d17352360dc27f9891 Mon Sep 17 00:00:00 2001 From: Marc Nuri Date: Mon, 2 Sep 2019 17:30:28 +0200 Subject: [PATCH 1/3] #27: Use of thread-safe non-blocking concurrent collections in Index --- .../mnimapsync/index/FolderCrawler.java | 2 +- .../com/marcnuri/mnimapsync/index/Index.java | 31 +++++++------------ .../mnimapsync/index/StoreCrawler.java | 2 +- 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/marcnuri/mnimapsync/index/FolderCrawler.java b/src/main/java/com/marcnuri/mnimapsync/index/FolderCrawler.java index 5f7d086..fa848bd 100644 --- a/src/main/java/com/marcnuri/mnimapsync/index/FolderCrawler.java +++ b/src/main/java/com/marcnuri/mnimapsync/index/FolderCrawler.java @@ -72,7 +72,7 @@ public void run() { } folder.close(false); } catch (MessagingException messagingException) { - index.getCrawlExceptions().add(messagingException); + index.addCrawlException(messagingException); } index.updatedIndexedMessageCount(indexedMessages); index.updatedSkippedMessageCount(skippedMessages); diff --git a/src/main/java/com/marcnuri/mnimapsync/index/Index.java b/src/main/java/com/marcnuri/mnimapsync/index/Index.java index 6e2a466..5dcbc5e 100644 --- a/src/main/java/com/marcnuri/mnimapsync/index/Index.java +++ b/src/main/java/com/marcnuri/mnimapsync/index/Index.java @@ -18,11 +18,7 @@ import static com.marcnuri.mnimapsync.imap.IMAPUtils.INBOX_MAILBOX; -import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -43,22 +39,20 @@ public class Index { private final AtomicLong indexedMessageCount; private final AtomicLong skippedMessageCount; //If no empty, the other processes shouldn't continue - private final List crawlExceptions; + private final Set crawlExceptions; public Index() { this.folderSeparator = new AtomicReference<>(); this.inbox = new AtomicReference<>(); this.folders = ConcurrentHashMap.newKeySet(); - this.folderMessages = Collections.synchronizedMap(new HashMap<>()); + this.folderMessages = new ConcurrentHashMap<>(); this.indexedMessageCount = new AtomicLong(); this.skippedMessageCount = new AtomicLong(); - this.crawlExceptions = Collections.synchronizedList(new ArrayList<>()); + this.crawlExceptions = ConcurrentHashMap.newKeySet(); } public final boolean hasCrawlException() { - synchronized (crawlExceptions) { - return !crawlExceptions.isEmpty(); - } + return !crawlExceptions.isEmpty(); } public final void updatedIndexedMessageCount(long delta) { @@ -104,17 +98,16 @@ public final long getSkippedMessageCount() { return skippedMessageCount.longValue(); } - public synchronized Set getFolderMessages(String folder) { - synchronized (folderMessages) { - if (!folderMessages.containsKey(folder)) { - folderMessages.put(folder, Collections.synchronizedSet(new HashSet<>())); - } - return folderMessages.get(folder); - } + public Set getFolderMessages(String folder) { + return folderMessages.computeIfAbsent(folder, k -> ConcurrentHashMap.newKeySet()); + } + + final void addCrawlException(MessagingException exception) { + crawlExceptions.add(exception); } - public final synchronized List getCrawlExceptions() { - return crawlExceptions; + public final Set getCrawlExceptions() { + return Collections.unmodifiableSet(crawlExceptions); } diff --git a/src/main/java/com/marcnuri/mnimapsync/index/StoreCrawler.java b/src/main/java/com/marcnuri/mnimapsync/index/StoreCrawler.java index 6eb4947..369fa7d 100644 --- a/src/main/java/com/marcnuri/mnimapsync/index/StoreCrawler.java +++ b/src/main/java/com/marcnuri/mnimapsync/index/StoreCrawler.java @@ -53,7 +53,7 @@ public static Index populateFromStore(final Index index, Store store, int thread service.shutdown(); service.awaitTermination(1, TimeUnit.HOURS); if (index.hasCrawlException()) { - messagingException = index.getCrawlExceptions().get(0); + messagingException = index.getCrawlExceptions().iterator().next(); } if (messagingException != null) { throw messagingException; From 0e4660e4a225180f25d3cbf6776afb5a0c1081f3 Mon Sep 17 00:00:00 2001 From: Marc Nuri Date: Tue, 3 Sep 2019 07:11:46 +0200 Subject: [PATCH 2/3] #28: Fix code smells before release --- .../com/marcnuri/mnimapsync/MNIMAPSync.java | 3 ++- .../com/marcnuri/mnimapsync/index/Index.java | 4 ++-- .../marcnuri/mnimapsync/index/MessageId.java | 11 +---------- .../mnimapsync/store/MessageCopier.java | 6 +++--- .../mnimapsync/store/MessageDeleter.java | 6 +++--- .../marcnuri/mnimapsync/store/StoreCopier.java | 18 ++++++------------ .../mnimapsync/store/StoreDeleter.java | 13 ++++++------- 7 files changed, 23 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/marcnuri/mnimapsync/MNIMAPSync.java b/src/main/java/com/marcnuri/mnimapsync/MNIMAPSync.java index 2d8087b..95153e7 100644 --- a/src/main/java/com/marcnuri/mnimapsync/MNIMAPSync.java +++ b/src/main/java/com/marcnuri/mnimapsync/MNIMAPSync.java @@ -38,6 +38,7 @@ * * @author Marc Nuri */ +@SuppressWarnings("WeakerAccess") public class MNIMAPSync { static final int THREADS = 5; @@ -120,7 +121,7 @@ public void sync() { indexTargetStore(); copySourceToTarget(); //Delete only if source store was completely indexed (this happens if no exceptions where raised) - if (syncOptions.getDelete() && sourceIndex != null && !sourceCopier.hasCopyException()) { + if (syncOptions.getDelete() && !sourceCopier.hasCopyException()) { deleteFromTarget(); } } catch (MessagingException | GeneralSecurityException ex) { diff --git a/src/main/java/com/marcnuri/mnimapsync/index/Index.java b/src/main/java/com/marcnuri/mnimapsync/index/Index.java index 5dcbc5e..c514a7d 100644 --- a/src/main/java/com/marcnuri/mnimapsync/index/Index.java +++ b/src/main/java/com/marcnuri/mnimapsync/index/Index.java @@ -32,8 +32,8 @@ */ public class Index { - private AtomicReference folderSeparator; - private AtomicReference inbox; + private final AtomicReference folderSeparator; + private final AtomicReference inbox; private final Set folders; private final Map> folderMessages; private final AtomicLong indexedMessageCount; diff --git a/src/main/java/com/marcnuri/mnimapsync/index/MessageId.java b/src/main/java/com/marcnuri/mnimapsync/index/MessageId.java index 0097f09..39bf740 100644 --- a/src/main/java/com/marcnuri/mnimapsync/index/MessageId.java +++ b/src/main/java/com/marcnuri/mnimapsync/index/MessageId.java @@ -58,9 +58,6 @@ public class MessageId implements Serializable { * will duplicate. * * It's a pity because fetching all of the HEADERS is a performance HOG - * - * @param message - * @throws MessageId.MessageIdException */ public MessageId(Message message) throws MessageIdException { try { @@ -111,9 +108,6 @@ public int hashCode() { /** * Really important. Different servers return different address values when they are invalid. - * - * @param addresses - * @return */ private static String[] parseAddress(String[] addresses) { if (addresses != null) { @@ -125,7 +119,7 @@ private static String[] parseAddress(String[] addresses) { } } Collections.sort(ret); - return ret.toArray(new String[ret.size()]); + return ret.toArray(new String[0]); } return new String[0]; } @@ -133,9 +127,6 @@ private static String[] parseAddress(String[] addresses) { /** * Adds required headers to fetch profile - * - * @param fetchProfile - * @return */ public static FetchProfile addHeaders(FetchProfile fetchProfile) { fetchProfile.add(FetchProfile.Item.ENVELOPE); diff --git a/src/main/java/com/marcnuri/mnimapsync/store/MessageCopier.java b/src/main/java/com/marcnuri/mnimapsync/store/MessageCopier.java index 88b9df0..5b2dabf 100644 --- a/src/main/java/com/marcnuri/mnimapsync/store/MessageCopier.java +++ b/src/main/java/com/marcnuri/mnimapsync/store/MessageCopier.java @@ -71,7 +71,7 @@ public void run() { final List toCopy = new ArrayList<>(); for (Message message : sourceMessages) { try { - final MessageId id = new MessageId((IMAPMessage) message); + final MessageId id = new MessageId(message); //Index message for deletion (if necessary) if (storeCopier.getSourceIndex() != null) { storeCopier.getSourceIndex().getFolderMessages(sourceFolderName).add(id); @@ -93,13 +93,13 @@ public void run() { fullProfile.add(FetchProfile.Item.FLAGS); fullProfile.add(IMAPFolder.FetchProfileItem.HEADERS); fullProfile.add(FetchProfile.Item.SIZE); - sourceFolder.fetch(toCopy.toArray(new Message[toCopy.size()]), fullProfile); + sourceFolder.fetch(toCopy.toArray(new Message[0]), fullProfile); final Folder targetFolder = storeCopier.getTargetStore().getFolder(targetFolderName); targetFolder.open(Folder.READ_WRITE); for (Message message : toCopy) { targetFolder.appendMessages(new Message[]{message}); try { - targetFolderMessages.add(new MessageId((IMAPMessage) message)); + targetFolderMessages.add(new MessageId(message)); copied++; if (copied % updateCount == 0) { storeCopier.updatedMessagesCopiedCount(copied); diff --git a/src/main/java/com/marcnuri/mnimapsync/store/MessageDeleter.java b/src/main/java/com/marcnuri/mnimapsync/store/MessageDeleter.java index 939974f..1df13a7 100644 --- a/src/main/java/com/marcnuri/mnimapsync/store/MessageDeleter.java +++ b/src/main/java/com/marcnuri/mnimapsync/store/MessageDeleter.java @@ -18,7 +18,6 @@ import com.marcnuri.mnimapsync.index.Index; import com.marcnuri.mnimapsync.index.MessageId; -import com.sun.mail.imap.IMAPMessage; import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; @@ -53,7 +52,8 @@ public MessageDeleter(StoreDeleter storeDeleter, } public void run() { - long deleted = 0L, skipped = 0L; + long deleted = 0L; + long skipped = 0L; try { final Folder targetFolder = storeDeleter.getTargetStore().getFolder(targetFolderName); //Opens a new connection per Thread @@ -62,7 +62,7 @@ public void run() { targetFolder.fetch(targetMessages, MessageId.addHeaders(new FetchProfile())); for (Message message : targetMessages) { try { - final MessageId id = new MessageId((IMAPMessage) message); + final MessageId id = new MessageId(message); if (!sourceFolderMessages.contains(id)) { message.setFlag(Flags.Flag.DELETED, true); deleted++; diff --git a/src/main/java/com/marcnuri/mnimapsync/store/StoreCopier.java b/src/main/java/com/marcnuri/mnimapsync/store/StoreCopier.java index 3d1535a..dd5af5d 100644 --- a/src/main/java/com/marcnuri/mnimapsync/store/StoreCopier.java +++ b/src/main/java/com/marcnuri/mnimapsync/store/StoreCopier.java @@ -88,9 +88,6 @@ public final void copy() throws InterruptedException { * * It also indexes the source store folders if we want to delete target folders that no longer * exist - * - * @param folder - * @throws MessagingException */ private void copySourceFolder(Folder folder) throws MessagingException { final String sourceFolderName = folder.getFullName(); @@ -106,9 +103,9 @@ private void copySourceFolder(Folder folder) throws MessagingException { throw new MessagingException(String.format( "Couldn't create folder: %s in target server.", sourceFolderName)); } - updatedFoldersCopiedCount(1); + incrementFoldersCopiedCount(); } else { - updatedFoldersSkippedCount(1); + incrementFoldersSkippedCount(); } //Folder recursion. Get all children if ((folder.getType() & Folder.HOLDS_FOLDERS) == Folder.HOLDS_FOLDERS) { @@ -121,9 +118,6 @@ private void copySourceFolder(Folder folder) throws MessagingException { /** * Once the folder structure has been created it copies messages recursively from the root * folder. - * - * @param sourceFolder - * @throws MessagingException */ private void copySourceMessages(IMAPFolder sourceFolder) throws MessagingException { if (sourceFolder != null) { @@ -170,12 +164,12 @@ public final boolean hasCopyException() { } } - private void updatedFoldersCopiedCount(int delta) { - foldersCopiedCount.getAndAdd(delta); + private void incrementFoldersCopiedCount() { + foldersCopiedCount.getAndAdd(1); } - private void updatedFoldersSkippedCount(int delta) { - foldersSkippedCount.getAndAdd(delta); + private void incrementFoldersSkippedCount() { + foldersSkippedCount.getAndAdd(1); } protected final void updatedMessagesCopiedCount(long delta) { diff --git a/src/main/java/com/marcnuri/mnimapsync/store/StoreDeleter.java b/src/main/java/com/marcnuri/mnimapsync/store/StoreDeleter.java index 6fbca25..fcc63f8 100644 --- a/src/main/java/com/marcnuri/mnimapsync/store/StoreDeleter.java +++ b/src/main/java/com/marcnuri/mnimapsync/store/StoreDeleter.java @@ -20,7 +20,6 @@ import com.marcnuri.mnimapsync.MNIMAPSync; import com.marcnuri.mnimapsync.index.Index; -import com.sun.mail.imap.IMAPFolder; import com.sun.mail.imap.IMAPStore; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -65,7 +64,7 @@ public final void delete() throws InterruptedException { //Delete Folder Structure deleteTargetFolder(targetStore.getDefaultFolder()); //Copy messages - deleteTargetMessages((IMAPFolder) targetStore.getDefaultFolder()); + deleteTargetMessages(targetStore.getDefaultFolder()); } catch (MessagingException ex) { Logger.getLogger(StoreDeleter.class.getName()).log(Level.SEVERE, null, ex); } @@ -73,7 +72,7 @@ public final void delete() throws InterruptedException { service.awaitTermination(1, TimeUnit.DAYS); } - private void deleteTargetMessages(IMAPFolder targetFolder) throws MessagingException { + private void deleteTargetMessages(Folder targetFolder) throws MessagingException { if (targetFolder != null) { final String targetFolderName = targetFolder.getFullName(); final String sourceFolderName = targetToSourceFolderName(targetFolderName, sourceIndex, targetIndex); @@ -98,7 +97,7 @@ private void deleteTargetMessages(IMAPFolder targetFolder) throws MessagingExcep //Folder recursion. Get all children if ((targetFolder.getType() & Folder.HOLDS_FOLDERS) == Folder.HOLDS_FOLDERS) { for (Folder child : targetFolder.list()) { - deleteTargetMessages((IMAPFolder) child); + deleteTargetMessages(child); } } } @@ -111,7 +110,7 @@ private void deleteTargetFolder(Folder folder) throws MessagingException { if (!sourceIndex.containsFolder(sourceFolderName)) { //Delete recursively targetStore.getFolder(targetFolderName).delete(true); - updatedFoldersDeletedCount(1); + incrementFoldersDeletedCount(); } //Folder recursion. Get all children if ((folder.getType() & Folder.HOLDS_FOLDERS) == Folder.HOLDS_FOLDERS) { @@ -121,8 +120,8 @@ private void deleteTargetFolder(Folder folder) throws MessagingException { } } - private void updatedFoldersDeletedCount(int delta) { - foldersDeletedCount.getAndAdd(delta); + private void incrementFoldersDeletedCount() { + foldersDeletedCount.getAndAdd(1); } protected final void updatedMessagesDeletedCount(long delta) { From bf0bceb06a2c4f62dacbe867a0fab948bcaf0591 Mon Sep 17 00:00:00 2001 From: Marc Nuri Date: Tue, 3 Sep 2019 07:25:44 +0200 Subject: [PATCH 3/3] #28: Increment coverage of affected classes --- .../mnimapsync/index/StoreCrawlerText.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/marcnuri/mnimapsync/index/StoreCrawlerText.java b/src/test/java/com/marcnuri/mnimapsync/index/StoreCrawlerText.java index 28078c9..07ea15d 100644 --- a/src/test/java/com/marcnuri/mnimapsync/index/StoreCrawlerText.java +++ b/src/test/java/com/marcnuri/mnimapsync/index/StoreCrawlerText.java @@ -24,6 +24,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doAnswer; @@ -68,7 +70,7 @@ void tearDown() { } @Test - void populateFromStore_storeHasFolders_ShouldPopulateIndex() throws Exception { + void populateFromStore_storeHasFolders_shouldPopulateIndex() throws Exception { // Given final Index index = new Index(); doReturn(1).when(defaultFolder).getMessageCount(); @@ -83,6 +85,26 @@ void populateFromStore_storeHasFolders_ShouldPopulateIndex() throws Exception { assertThat(index.hasCrawlException(), equalTo(false)); } + @Test + void populateFromStore_indexHasExceptionsAndStoreHasFolders_shouldThrowException() throws Exception { + // Given + final Index index = new Index(); + index.addCrawlException(new MessagingException("Indexing tasks went wrong at some point")); + doReturn(1).when(defaultFolder).getMessageCount(); + // When + final MessagingException result = assertThrows(MessagingException.class, () -> { + populateFromStore(index, imapStore, 1); + fail(); + }); + // Then + verify(defaultFolder, times(1)).expunge(); + assertThat(index.containsFolder("INBOX"), equalTo(true)); + assertThat(index.containsFolder("Folder 1"), equalTo(true)); + assertThat(index.containsFolder("Folder 2"), equalTo(true)); + assertThat(index.hasCrawlException(), equalTo(true)); + assertThat(result.getMessage(), equalTo("Indexing tasks went wrong at some point")); + } + private static IMAPFolder mockFolder(String name) throws MessagingException { final IMAPFolder mockFolder = Mockito.mock(IMAPFolder.class); doReturn(name).when(mockFolder).getFullName();