Repository: james-project Updated Branches: refs/heads/master b9531d6f5 -> 65c2bb774
JAMES-2402 InMemoryMailRepositoryStore is not thread safe Calling select concurrently leands to different repositories being returned and thus some mails to be lost, leading to test instability. Current changes focuses on correctness, even if it still leads to some non accessible mail repository being initialized. Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/65c2bb77 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/65c2bb77 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/65c2bb77 Branch: refs/heads/master Commit: 65c2bb77460e8781dcc4ca967bf9d69946c159d2 Parents: b9531d6 Author: benwa <[email protected]> Authored: Thu Jun 7 10:15:40 2018 +0700 Committer: benwa <[email protected]> Committed: Fri Jun 8 10:05:16 2018 +0700 ---------------------------------------------------------------------- server/container/guice/guice-common/pom.xml | 5 +++ .../utils/InMemoryMailRepositoryStore.java | 11 ++--- .../utils/InMemoryMailRepositoryStoreTest.java | 44 +++++++++++++++++++- .../src/test/resources/mailrepositorystore.xml | 5 +++ 4 files changed, 58 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/65c2bb77/server/container/guice/guice-common/pom.xml ---------------------------------------------------------------------- diff --git a/server/container/guice/guice-common/pom.xml b/server/container/guice/guice-common/pom.xml index d36ffdc..2c0be52 100644 --- a/server/container/guice/guice-common/pom.xml +++ b/server/container/guice/guice-common/pom.xml @@ -113,6 +113,11 @@ </dependency> <dependency> <groupId>${project.groupId}</groupId> + <artifactId>james-server-mailrepository-memory</artifactId> + <scope>test</scope> + </dependency> + <dependency> + <groupId>${project.groupId}</groupId> <artifactId>james-server-onami</artifactId> </dependency> <dependency> http://git-wip-us.apache.org/repos/asf/james-project/blob/65c2bb77/server/container/guice/guice-common/src/main/java/org/apache/james/utils/InMemoryMailRepositoryStore.java ---------------------------------------------------------------------- diff --git a/server/container/guice/guice-common/src/main/java/org/apache/james/utils/InMemoryMailRepositoryStore.java b/server/container/guice/guice-common/src/main/java/org/apache/james/utils/InMemoryMailRepositoryStore.java index 05d25b6..2c4fab7 100644 --- a/server/container/guice/guice-common/src/main/java/org/apache/james/utils/InMemoryMailRepositoryStore.java +++ b/server/container/guice/guice-common/src/main/java/org/apache/james/utils/InMemoryMailRepositoryStore.java @@ -89,7 +89,7 @@ public class InMemoryMailRepositoryStore implements MailRepositoryStore, Configu } @Override - public void configure(HierarchicalConfiguration configuration) throws ConfigurationException { + public void configure(HierarchicalConfiguration configuration) { this.configuration = configuration; } @@ -111,7 +111,7 @@ public class InMemoryMailRepositoryStore implements MailRepositoryStore, Configu } @Override - public Optional<MailRepository> get(String url) throws MailRepositoryStoreException { + public Optional<MailRepository> get(String url) { return Optional.ofNullable(destinationToRepositoryAssociations.get(url)); } @@ -127,8 +127,9 @@ public class InMemoryMailRepositoryStore implements MailRepositoryStore, Configu private MailRepository createNewMailRepository(Destination destination) throws MailRepositoryStoreException { MailRepository newMailRepository = retrieveMailRepository(destination); newMailRepository = initializeNewRepository(newMailRepository, createRepositoryCombinedConfig(destination)); - destinationToRepositoryAssociations.putIfAbsent(destination.url, newMailRepository); - return newMailRepository; + MailRepository previousRepository = destinationToRepositoryAssociations.putIfAbsent(destination.url, newMailRepository); + return Optional.ofNullable(previousRepository) + .orElse(newMailRepository); } private void readConfigurationEntry(HierarchicalConfiguration repositoryConfiguration) throws ConfigurationException { @@ -153,7 +154,7 @@ public class InMemoryMailRepositoryStore implements MailRepositoryStore, Configu } } - private CombinedConfiguration createRepositoryCombinedConfig(Destination destination) throws MailRepositoryStoreException { + private CombinedConfiguration createRepositoryCombinedConfig(Destination destination) { CombinedConfiguration config = new CombinedConfiguration(); HierarchicalConfiguration defaultProtocolConfig = perProtocolMailRepositoryDefaultConfiguration.get(destination.protocol); if (defaultProtocolConfig != null) { http://git-wip-us.apache.org/repos/asf/james-project/blob/65c2bb77/server/container/guice/guice-common/src/test/java/org/apache/james/utils/InMemoryMailRepositoryStoreTest.java ---------------------------------------------------------------------- diff --git a/server/container/guice/guice-common/src/test/java/org/apache/james/utils/InMemoryMailRepositoryStoreTest.java b/server/container/guice/guice-common/src/test/java/org/apache/james/utils/InMemoryMailRepositoryStoreTest.java index 38f4b9b..3321933 100644 --- a/server/container/guice/guice-common/src/test/java/org/apache/james/utils/InMemoryMailRepositoryStoreTest.java +++ b/server/container/guice/guice-common/src/test/java/org/apache/james/utils/InMemoryMailRepositoryStoreTest.java @@ -22,15 +22,21 @@ package org.apache.james.utils; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.concurrent.TimeUnit; + import org.apache.commons.configuration.ConfigurationException; import org.apache.commons.configuration.HierarchicalConfiguration; +import org.apache.james.core.builder.MimeMessageBuilder; import org.apache.james.mailrepository.api.MailRepository; import org.apache.james.mailrepository.api.MailRepositoryStore; import org.apache.james.mailrepository.file.FileMailRepository; +import org.apache.james.mailrepository.memory.MemoryMailRepository; import org.apache.james.modules.server.MailStoreRepositoryModule; import org.apache.james.server.core.configuration.Configuration; import org.apache.james.server.core.configuration.FileConfigurationProvider; import org.apache.james.server.core.filesystem.FileSystemImpl; +import org.apache.james.util.concurrency.ConcurrentTestRunner; +import org.apache.mailet.base.test.FakeMail; import org.junit.Before; import org.junit.Test; @@ -38,6 +44,18 @@ import com.google.common.collect.Sets; public class InMemoryMailRepositoryStoreTest { + private static class MemoryMailRepositoryProvider implements MailRepositoryProvider { + @Override + public String canonicalName() { + return MemoryMailRepository.class.getCanonicalName(); + } + + @Override + public MailRepository provide(String url) { + return new MemoryMailRepository(); + } + } + private InMemoryMailRepositoryStore repositoryStore; private FileSystemImpl fileSystem; private Configuration configuration; @@ -51,7 +69,8 @@ public class InMemoryMailRepositoryStoreTest { fileSystem = new FileSystemImpl(configuration.directories()); repositoryStore = new InMemoryMailRepositoryStore(Sets.newHashSet( new MailStoreRepositoryModule.FileMailRepositoryProvider( - fileSystem))); + fileSystem), + new MemoryMailRepositoryProvider())); repositoryStore.configure(new FileConfigurationProvider(fileSystem, configuration) .getConfiguration("mailrepositorystore")); repositoryStore.init(); @@ -123,7 +142,7 @@ public class InMemoryMailRepositoryStoreTest { } @Test - public void getShouldReturnEmptyWhenUrlNotInUse() throws Exception { + public void getShouldReturnEmptyWhenUrlNotInUse() { assertThat(repositoryStore.get("file://repo")) .isEmpty(); } @@ -137,4 +156,25 @@ public class InMemoryMailRepositoryStoreTest { .contains(mailRepository); } + @Test + public void selectShouldNotReturnDifferentResultsWhenUsedInAConcurrentEnvironment() throws Exception { + String url = "memory://repo"; + int threadCount = 10; + int operationCount = 1; + + ConcurrentTestRunner concurrentTestRunner = new ConcurrentTestRunner(threadCount, operationCount, + (threadNb, operationNb) -> repositoryStore.select(url) + .store(FakeMail.builder() + .name("name" + threadNb) + .mimeMessage(MimeMessageBuilder.mimeMessageBuilder() + .setText("Any body")) + .build())); + concurrentTestRunner.run().awaitTermination(1, TimeUnit.MINUTES); + concurrentTestRunner.assertNoException(); + + long actualSize = repositoryStore.get(url).get().size(); + + assertThat(actualSize).isEqualTo(threadCount); + } + } http://git-wip-us.apache.org/repos/asf/james-project/blob/65c2bb77/server/container/guice/guice-common/src/test/resources/mailrepositorystore.xml ---------------------------------------------------------------------- diff --git a/server/container/guice/guice-common/src/test/resources/mailrepositorystore.xml b/server/container/guice/guice-common/src/test/resources/mailrepositorystore.xml index 3ca4a1d..736c1c6 100644 --- a/server/container/guice/guice-common/src/test/resources/mailrepositorystore.xml +++ b/server/container/guice/guice-common/src/test/resources/mailrepositorystore.xml @@ -27,5 +27,10 @@ </protocols> <config FIFO="false" CACHEKEYS="true"/> </mailrepository> + <mailrepository class="org.apache.james.mailrepository.memory.MemoryMailRepository"> + <protocols> + <protocol>memory</protocol> + </protocols> + </mailrepository> </mailrepositories> </mailrepositorystore> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
