JAMES-2047 Concurrency issues when saving mailboxes

apache · Jun 7, 2017 · 048f95d · 048f95d
1 parent 8357286
commit 048f95d
Showing 1 changed file with 25 additions and 34 deletions.
diff --git a/...ox/memory/src/main/java/org/apache/james/mailbox/inmemory/mail/InMemoryMailboxMapper.java b/...ox/memory/src/main/java/org/apache/james/mailbox/inmemory/mail/InMemoryMailboxMapper.java
@@ -20,7 +20,6 @@
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
 
@@ -36,55 +35,49 @@
 import org.apache.james.mailbox.store.mail.model.impl.SimpleMailbox;
 
 import com.google.common.base.Objects;
+import com.google.common.base.Optional;
 
 public class InMemoryMailboxMapper implements MailboxMapper {
 
     private static final int INITIAL_SIZE = 128;
-    private final Map<InMemoryId, Mailbox> mailboxesById;
+    private final ConcurrentHashMap<MailboxPath, Mailbox> mailboxesByPath;
     private final AtomicLong mailboxIdGenerator = new AtomicLong();
 
     public InMemoryMailboxMapper() {
-        mailboxesById = new ConcurrentHashMap<InMemoryId, Mailbox>(INITIAL_SIZE);
+        mailboxesByPath = new ConcurrentHashMap<MailboxPath, Mailbox>(INITIAL_SIZE);
     }
 
     /**
      * @see org.apache.james.mailbox.store.mail.MailboxMapper#delete(org.apache.james.mailbox.store.mail.model.Mailbox)
      */
     public void delete(Mailbox mailbox) throws MailboxException {
-        mailboxesById.remove(mailbox.getMailboxId());
+        mailboxesByPath.remove(mailbox.generateAssociatedPath());
     }
 
     public void deleteAll() throws MailboxException {
-        mailboxesById.clear();
+        mailboxesByPath.clear();
     }
 
     /**
      * @see org.apache.james.mailbox.store.mail.MailboxMapper#findMailboxByPath(org.apache.james.mailbox.model.MailboxPath)
      */
     public synchronized Mailbox findMailboxByPath(MailboxPath path) throws MailboxException {
-        Mailbox result = null;
+        Mailbox result = mailboxesByPath.get(path);
-        for (Mailbox mailbox:mailboxesById.values()) {
-            MailboxPath mp = new MailboxPath(mailbox.getNamespace(), mailbox.getUser(), mailbox.getName());
-            if (mp.equals(path)) {
-                result = mailbox;
-                break;
-            }
-        }
         if (result == null) {
             throw new MailboxNotFoundException(path);
         } else {
-            return result;
+            return new SimpleMailbox(result);
         }
     }
 
     public synchronized Mailbox findMailboxById(MailboxId id) throws MailboxException {
         InMemoryId mailboxId = (InMemoryId)id;
-        Mailbox result = mailboxesById.get(mailboxId);
+        for (Mailbox mailbox: mailboxesByPath.values()) {
-        if (result == null) {
+            if (mailbox.getMailboxId().equals(mailboxId)) {
-            throw new MailboxNotFoundException(mailboxId.serialize());
+                return new SimpleMailbox(mailbox);
-        } else {
+            }
-            return result;
         }
+        throw new MailboxNotFoundException(mailboxId.serialize());
     }
 
     /**
@@ -93,9 +86,9 @@ public synchronized Mailbox findMailboxById(MailboxId id) throws MailboxExceptio
     public List<Mailbox> findMailboxWithPathLike(MailboxPath path) throws MailboxException {
         final String regex = path.getName().replace("%", ".*");
         List<Mailbox> results = new ArrayList<Mailbox>();
-        for (Mailbox mailbox:mailboxesById.values()) {
+        for (Mailbox mailbox: mailboxesByPath.values()) {
             if (mailboxMatchesRegex(mailbox, path, regex)) {
-                results.add(mailbox);
+                results.add(new SimpleMailbox(mailbox));
             }
         }
         return results;
@@ -115,23 +108,21 @@ public MailboxId save(Mailbox mailbox) throws MailboxException {
         if (id == null) {
             id = InMemoryId.of(mailboxIdGenerator.incrementAndGet());
             ((SimpleMailbox) mailbox).setMailboxId(id);
+        } else {
+            try {
+                Mailbox mailboxWithPreviousName = findMailboxById(id);
+                mailboxesByPath.remove(mailboxWithPreviousName.generateAssociatedPath());
+            } catch (MailboxNotFoundException e) {
+                // No need to remove the previous mailbox
+            }
         }
-        if (isPathAlreadyUsedByAnotherMailbox(mailbox)) {
+        Mailbox previousMailbox = mailboxesByPath.putIfAbsent(mailbox.generateAssociatedPath(), mailbox);
+        if (previousMailbox != null) {
             throw new MailboxExistsException(mailbox.getName());
         }
-        mailboxesById.put(id, mailbox);
         return mailbox.getMailboxId();
     }
 
-    private boolean isPathAlreadyUsedByAnotherMailbox(Mailbox mailbox) throws MailboxException {
-        try {
-            Mailbox storedMailbox = findMailboxByPath(mailbox.generateAssociatedPath());
-            return !Objects.equal(storedMailbox.getMailboxId(), mailbox.getMailboxId());
-        } catch (MailboxNotFoundException e) {
-            return false;
-        }
-    }
-
     /**
      * Do nothing
      */
@@ -144,7 +135,7 @@ public void endRequest() {
      */
     public boolean hasChildren(Mailbox mailbox, char delimiter) throws MailboxException {
         String mailboxName = mailbox.getName() + delimiter;
-        for (Mailbox box:mailboxesById.values()) {
+        for (Mailbox box: mailboxesByPath.values()) {
             if (belongsToSameUser(mailbox, box) && box.getName().startsWith(mailboxName)) {
                 return true;
             }
@@ -161,7 +152,7 @@ private boolean belongsToSameUser(Mailbox mailbox, Mailbox otherMailbox) {
      * @see org.apache.james.mailbox.store.mail.MailboxMapper#list()
      */
     public List<Mailbox> list() throws MailboxException {
-        return new ArrayList<Mailbox>(mailboxesById.values());
+        return new ArrayList<Mailbox>(mailboxesByPath.values());
     }
 
     public <T> T execute(Transaction<T> transaction) throws MailboxException {