JAMES-2063 SelectedMailboxImpl should allow concurrent events when initializing
Also reduce lock contention. Avoid locking while building conversion and reading from the mailbox, to avoid delaying concurrent sessions. Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/53d75365 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/53d75365 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/53d75365 Branch: refs/heads/master Commit: 53d75365fc2de63c5c8ab7205b91f2695e8dccd5 Parents: b74db02 Author: benwa <btell...@linagora.com> Authored: Tue Jun 20 10:55:21 2017 +0700 Committer: benwa <btell...@linagora.com> Committed: Wed Jun 21 14:58:20 2017 +0700 ---------------------------------------------------------------------- protocols/imap/pom.xml | 5 +- .../processor/base/SelectedMailboxImpl.java | 12 +- .../imap/processor/base/UidMsnConverter.java | 17 ++- .../processor/base/SelectedMailboxImplTest.java | 44 +++---- .../processor/base/UidMsnConverterTest.java | 120 ++++++++++++++++--- .../imap/src/test/resources/logback-test.xml | 24 ++++ 6 files changed, 170 insertions(+), 52 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/53d75365/protocols/imap/pom.xml ---------------------------------------------------------------------- diff --git a/protocols/imap/pom.xml b/protocols/imap/pom.xml index 12b9826..78ac115 100644 --- a/protocols/imap/pom.xml +++ b/protocols/imap/pom.xml @@ -76,8 +76,9 @@ <artifactId>slf4j-api</artifactId> </dependency> <dependency> - <groupId>org.slf4j</groupId> - <artifactId>slf4j-simple</artifactId> + <groupId>ch.qos.logback</groupId> + <artifactId>logback-classic</artifactId> + <version>1.1.7</version> <scope>test</scope> </dependency> http://git-wip-us.apache.org/repos/asf/james-project/blob/53d75365/protocols/imap/src/main/java/org/apache/james/imap/processor/base/SelectedMailboxImpl.java ---------------------------------------------------------------------- diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/base/SelectedMailboxImpl.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/base/SelectedMailboxImpl.java index d929b76..d56c6ef 100644 --- a/protocols/imap/src/main/java/org/apache/james/imap/processor/base/SelectedMailboxImpl.java +++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/base/SelectedMailboxImpl.java @@ -44,6 +44,7 @@ import org.apache.james.mailbox.model.SearchQuery; import org.apache.james.mailbox.model.UpdatedFlags; import com.google.common.base.Optional; +import com.google.common.collect.ImmutableList; /** * Default implementation of {@link SelectedMailbox} @@ -84,17 +85,14 @@ public class SelectedMailboxImpl implements SelectedMailbox, MailboxListener{ MailboxSession mailboxSession = ImapSessionUtils.getMailboxSession(session); + uidMsnConverter = new UidMsnConverter(); + mailboxManager.addListener(path, this, mailboxSession); MessageManager messageManager = mailboxManager.getMailbox(path, mailboxSession); applicableFlags = messageManager.getApplicableFlags(mailboxSession); - uidMsnConverter = getUidMsnConverter(mailboxSession, messageManager); - } - - private UidMsnConverter getUidMsnConverter(MailboxSession mailboxSession , MessageManager messageManager) throws MailboxException { - return new UidMsnConverter( - messageManager.search( - new SearchQuery(SearchQuery.all()), mailboxSession)); + uidMsnConverter.addAll(ImmutableList.copyOf( + messageManager.search(new SearchQuery(SearchQuery.all()), mailboxSession))); } @Override http://git-wip-us.apache.org/repos/asf/james-project/blob/53d75365/protocols/imap/src/main/java/org/apache/james/imap/processor/base/UidMsnConverter.java ---------------------------------------------------------------------- diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/base/UidMsnConverter.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/base/UidMsnConverter.java index 9276e27..9e09053 100644 --- a/protocols/imap/src/main/java/org/apache/james/imap/processor/base/UidMsnConverter.java +++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/base/UidMsnConverter.java @@ -21,7 +21,9 @@ package org.apache.james.imap.processor.base; import java.util.ArrayList; import java.util.Collections; -import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.TreeSet; import org.apache.james.mailbox.MessageUid; @@ -35,9 +37,16 @@ public class UidMsnConverter { @VisibleForTesting final ArrayList<MessageUid> uids; - public UidMsnConverter(Iterator<MessageUid> iterator) { - uids = Lists.newArrayList(iterator); - Collections.sort(uids); + public UidMsnConverter() { + this.uids = Lists.newArrayList(); + } + + public synchronized void addAll(List<MessageUid> addedUids) { + TreeSet<MessageUid> tmp = new TreeSet<MessageUid>(); + tmp.addAll(uids); + tmp.addAll(addedUids); + uids.clear(); + uids.addAll(tmp); } public synchronized Optional<Integer> getMsn(MessageUid uid) { http://git-wip-us.apache.org/repos/asf/james-project/blob/53d75365/protocols/imap/src/test/java/org/apache/james/imap/processor/base/SelectedMailboxImplTest.java ---------------------------------------------------------------------- diff --git a/protocols/imap/src/test/java/org/apache/james/imap/processor/base/SelectedMailboxImplTest.java b/protocols/imap/src/test/java/org/apache/james/imap/processor/base/SelectedMailboxImplTest.java index a9e7144..cdeef78 100644 --- a/protocols/imap/src/test/java/org/apache/james/imap/processor/base/SelectedMailboxImplTest.java +++ b/protocols/imap/src/test/java/org/apache/james/imap/processor/base/SelectedMailboxImplTest.java @@ -52,7 +52,6 @@ import org.apache.james.mailbox.store.mail.model.DefaultMessageId; import org.apache.james.mailbox.store.mail.model.Mailbox; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -65,7 +64,9 @@ import com.google.common.collect.ImmutableList; public class SelectedMailboxImplTest { private static final Logger LOGGER = LoggerFactory.getLogger(SelectedMailboxImplTest.class); - public static final MessageUid MESSAGE_UID_5 = MessageUid.of(5); + private static final MessageUid EMITTED_EVENT_UID = MessageUid.of(5); + private static final int MOD_SEQ = 12; + private static final int SIZE = 38; private ExecutorService executorService; private MailboxManager mailboxManager; @@ -77,20 +78,23 @@ public class SelectedMailboxImplTest { @Before public void setUp() throws Exception { executorService = Executors.newFixedThreadPool(1); - mailboxPath = new MailboxPath(MailboxConstants.USER_NAMESPACE, "tell...@linagora.com", MailboxConstants.INBOX); mailboxManager = mock(MailboxManager.class); messageManager = mock(MessageManager.class); - when(mailboxManager.getMailbox(eq(mailboxPath), any(MailboxSession.class))).thenReturn(messageManager); - when(messageManager.getApplicableFlags(any(MailboxSession.class))).thenReturn(new Flags()); + imapSession = mock(ImapSession.class); + mailbox = mock(Mailbox.class); + + when(mailboxManager.getMailbox(eq(mailboxPath), any(MailboxSession.class))) + .thenReturn(messageManager); + when(messageManager.getApplicableFlags(any(MailboxSession.class))) + .thenReturn(new Flags()); when(messageManager.search(any(SearchQuery.class), any(MailboxSession.class))) - .then(sleepThenSearchAnswer()); + .then(delayedSearchAnswer()); - imapSession = mock(ImapSession.class); when(imapSession.getAttribute(ImapSessionUtils.MAILBOX_SESSION_ATTRIBUTE_SESSION_KEY)).thenReturn(mock(MailboxSession.class)); - mailbox = mock(Mailbox.class); - when(mailbox.generateAssociatedPath()).thenReturn(mailboxPath); + when(mailbox.generateAssociatedPath()) + .thenReturn(mailboxPath); } @After @@ -98,11 +102,10 @@ public class SelectedMailboxImplTest { executorService.shutdownNow(); } - @Ignore @Test - public void concurrentEventShouldNotSkipUidEmmitedDuringInitialization() throws Exception { - final AtomicInteger success = new AtomicInteger(0); - doAnswer(generateEmitEventAnswer(success)) + public void concurrentEventShouldNotSkipAddedEventsEmittedDuringInitialisation() throws Exception { + final AtomicInteger successCount = new AtomicInteger(0); + doAnswer(generateEmitEventAnswer(successCount)) .when(mailboxManager) .addListener(eq(mailboxPath), any(MailboxListener.class), any(MailboxSession.class)); @@ -111,14 +114,13 @@ public class SelectedMailboxImplTest { imapSession, mailboxPath); - assertThat(selectedMailbox.getLastUid().get()).isEqualTo(MESSAGE_UID_5); + assertThat(selectedMailbox.getLastUid().get()).isEqualTo(EMITTED_EVENT_UID); } - @Ignore @Test - public void concurrentEventShouldBeSupportedDuringInitialisation() throws Exception { - final AtomicInteger success = new AtomicInteger(0); - doAnswer(generateEmitEventAnswer(success)) + public void concurrentEventShouldBeProcessedSuccessfullyDuringInitialisation() throws Exception { + final AtomicInteger successCount = new AtomicInteger(0); + doAnswer(generateEmitEventAnswer(successCount)) .when(mailboxManager) .addListener(eq(mailboxPath), any(MailboxListener.class), any(MailboxSession.class)); @@ -127,12 +129,12 @@ public class SelectedMailboxImplTest { imapSession, mailboxPath); - assertThat(success.get()) + assertThat(successCount.get()) .as("Get the incremented value in case of successful event processing.") .isEqualTo(1); } - private Answer<Iterator<MessageUid>> sleepThenSearchAnswer() { + private Answer<Iterator<MessageUid>> delayedSearchAnswer() { return new Answer<Iterator<MessageUid>>() { @Override public Iterator<MessageUid> answer(InvocationOnMock invocation) throws Throwable { @@ -166,7 +168,7 @@ public class SelectedMailboxImplTest { private void emitEvent(MailboxListener mailboxListener) { TreeMap<MessageUid, MessageMetaData> result = new TreeMap<MessageUid, MessageMetaData>(); - result.put(MESSAGE_UID_5, new SimpleMessageMetaData(MESSAGE_UID_5, 12, new Flags(), 38, new Date(), new DefaultMessageId())); + result.put(EMITTED_EVENT_UID, new SimpleMessageMetaData(EMITTED_EVENT_UID, MOD_SEQ, new Flags(), SIZE, new Date(), new DefaultMessageId())); mailboxListener.event(new EventFactory().added(mock(MailboxSession.class), result, mailbox)); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/53d75365/protocols/imap/src/test/java/org/apache/james/imap/processor/base/UidMsnConverterTest.java ---------------------------------------------------------------------- diff --git a/protocols/imap/src/test/java/org/apache/james/imap/processor/base/UidMsnConverterTest.java b/protocols/imap/src/test/java/org/apache/james/imap/processor/base/UidMsnConverterTest.java index 4606e83..ae81f2b 100644 --- a/protocols/imap/src/test/java/org/apache/james/imap/processor/base/UidMsnConverterTest.java +++ b/protocols/imap/src/test/java/org/apache/james/imap/processor/base/UidMsnConverterTest.java @@ -42,7 +42,7 @@ public class UidMsnConverterTest { @Before public void setUp() { - testee = new UidMsnConverter(ImmutableList.<MessageUid>of().iterator()); + testee = new UidMsnConverter(); messageUid1 = MessageUid.of(1); messageUid2 = MessageUid.of(2); messageUid3 = MessageUid.of(3); @@ -178,7 +178,7 @@ public class UidMsnConverterTest { } @Test - public void removeShouldKeepAValidMappingWhenDeletingBeginning() { + public void removeShouldKeepAMonoticMSNToUIDConversionMappingWhenDeletingBeginning() { testee.addUid(messageUid1); testee.addUid(messageUid2); testee.addUid(messageUid3); @@ -194,7 +194,7 @@ public class UidMsnConverterTest { } @Test - public void removeShouldKeepAValidMappingWhenDeletingEnd() { + public void removeShouldKeepAMonoticMSNToUIDConversionMappingWhenDeletingEnd() { testee.addUid(messageUid1); testee.addUid(messageUid2); testee.addUid(messageUid3); @@ -210,7 +210,7 @@ public class UidMsnConverterTest { } @Test - public void removeShouldKeepAValidMappingWhenDeletingMiddle() { + public void removeShouldKeepAMonoticMSNToUIDConversionMappingWhenDeletingMiddle() { testee.addUid(messageUid1); testee.addUid(messageUid2); testee.addUid(messageUid3); @@ -233,7 +233,7 @@ public class UidMsnConverterTest { testee.addUid(messageUid4); assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) - .containsOnlyElementsOf(ImmutableMap.of( + .containsExactlyElementsOf(ImmutableMap.of( 1, messageUid1, 2, messageUid2, 3, messageUid3, @@ -241,14 +241,14 @@ public class UidMsnConverterTest { } @Test - public void addUidShouldLeadToValidConversionWhenInsertInFirstPosition() { + public void addUidShouldLeadToMonoticMSNToUIDConversionWhenInsertInFirstPosition() { testee.addUid(messageUid2); testee.addUid(messageUid3); testee.addUid(messageUid4); testee.addUid(messageUid1); assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) - .containsOnlyElementsOf(ImmutableMap.of( + .containsExactlyElementsOf(ImmutableMap.of( 1, messageUid1, 2, messageUid2, 3, messageUid3, @@ -256,15 +256,99 @@ public class UidMsnConverterTest { } @Test - public void constructorWithOutOfOrderIteratorShouldLeadToValidConversion() { - testee = new UidMsnConverter(ImmutableList.of(messageUid2, + public void addAllShouldLeadToMonoticMSNToUIDConversion() { + testee.addAll(ImmutableList.of( + messageUid1, + messageUid2, + messageUid3, + messageUid4)); + + assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) + .containsExactlyElementsOf(ImmutableMap.of( + 1, messageUid1, + 2, messageUid2, + 3, messageUid3, + 4, messageUid4).entrySet()); + } + + @Test + public void addAllShouldRemoveDuplicates() { + testee.addAll(ImmutableList.of( + messageUid1, + messageUid2, + messageUid2, + messageUid3, + messageUid4)); + + assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) + .containsExactlyElementsOf(ImmutableMap.of( + 1, messageUid1, + 2, messageUid2, + 3, messageUid3, + 4, messageUid4).entrySet()); + } + + @Test + public void addAllShouldDeduplicateElements() { + testee.addUid(messageUid1); + + testee.addAll(ImmutableList.of( + messageUid1, + messageUid2, + messageUid3, + messageUid4)); + + assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) + .containsExactlyElementsOf(ImmutableMap.of( + 1, messageUid1, + 2, messageUid2, + 3, messageUid3, + 4, messageUid4).entrySet()); + } + + @Test + public void addAllShouldMergeWithPreviousData() { + testee.addUid(messageUid1); + + testee.addAll(ImmutableList.of(messageUid2, + messageUid3, + messageUid4)); + + assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) + .containsExactlyElementsOf(ImmutableMap.of( + 1, messageUid1, + 2, messageUid2, + 3, messageUid3, + 4, messageUid4).entrySet()); + } + + @Test + public void addAllShouldMergeAndDeduplicatePreviousData() { + testee.addUid(messageUid1); + testee.addUid(messageUid3); + + testee.addAll(ImmutableList.of(messageUid2, + messageUid3, + messageUid4)); + + assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) + .containsExactlyElementsOf(ImmutableMap.of( + 1, messageUid1, + 2, messageUid2, + 3, messageUid3, + 4, messageUid4).entrySet()); + } + + @Test + public void addAllWithOutOfOrderIteratorShouldLeadToMonoticMSNToUIDConversion() { + testee.addAll(ImmutableList.of( + messageUid2, messageUid3, messageUid4, - messageUid1) - .iterator()); + messageUid1)); assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) - .containsOnlyElementsOf(ImmutableMap.of( + .containsExactlyElementsOf(ImmutableMap.of( 1, messageUid1, 2, messageUid2, 3, messageUid3, @@ -281,7 +365,7 @@ public class UidMsnConverterTest { } @Test - public void addAndRemoveShouldLeadToValidConversionWhenMixed() throws Exception { + public void addAndRemoveShouldLeadToMonoticMSNToUIDConversionWhenMixed() throws Exception { final int initialCount = 1000; for (int i = 1; i <= initialCount; i++) { testee.addUid(MessageUid.of(i)); @@ -307,11 +391,11 @@ public class UidMsnConverterTest { resultBuilder.put(i, MessageUid.of(initialCount + i)); } assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) - .containsOnlyElementsOf(resultBuilder.build().entrySet()); + .containsExactlyElementsOf(resultBuilder.build().entrySet()); } @Test - public void addShouldLeadToValidConversionWhenConcurrent() throws Exception { + public void addShouldLeadToMonoticMSNToUIDConversionWhenConcurrent() throws Exception { final int operationCount = 1000; int threadCount = 2; @@ -330,11 +414,11 @@ public class UidMsnConverterTest { resultBuilder.put(i, MessageUid.of(i)); } assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) - .containsOnlyElementsOf(resultBuilder.build().entrySet()); + .containsExactlyElementsOf(resultBuilder.build().entrySet()); } @Test - public void removeShouldLeadToValidConversionWhenConcurrent() throws Exception { + public void removeShouldLeadToMonoticMSNToUIDConversionWhenConcurrent() throws Exception { final int operationCount = 1000; int threadCount = 2; for (int i = 1; i <= operationCount * (threadCount + 1); i++) { @@ -356,7 +440,7 @@ public class UidMsnConverterTest { resultBuilder.put(i, MessageUid.of((threadCount * operationCount) + i)); } assertThat(mapTesteeInternalDataToMsnByUid().entrySet()) - .containsOnlyElementsOf(resultBuilder.build().entrySet()); + .containsExactlyElementsOf(resultBuilder.build().entrySet()); } private Map<Integer, MessageUid> mapTesteeInternalDataToMsnByUid() { http://git-wip-us.apache.org/repos/asf/james-project/blob/53d75365/protocols/imap/src/test/resources/logback-test.xml ---------------------------------------------------------------------- diff --git a/protocols/imap/src/test/resources/logback-test.xml b/protocols/imap/src/test/resources/logback-test.xml new file mode 100644 index 0000000..5e4c459 --- /dev/null +++ b/protocols/imap/src/test/resources/logback-test.xml @@ -0,0 +1,24 @@ +<?xml version="1.0" encoding="UTF-8"?> +<configuration> + + <contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"> + <resetJUL>true</resetJUL> + </contextListener> + + <appender name="CONSOLE" class="ch.qos.logback.core.ConsoleAppender"> + <encoder> + <pattern>%d{HH:mm:ss.SSS} %highlight([%-5level]) %logger{15} - %msg%n%rEx</pattern> + <immediateFlush>false</immediateFlush> + </encoder> + </appender> + + <root level="ERROR"> + <appender-ref ref="CONSOLE" /> + </root> + + + <logger name="org.apache.james" level="DEBUG" > + <appender-ref ref="CONSOLE" /> + </logger> + +</configuration> --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org