Fix concurrent modification in UpdateManager.

If we add or remove components while an update check is ongoing
we used to throw a CME. This could happen either when the client
is first starting and in the process of adding components, or
if some listener decided to add/remove components as they were
being checked.

Change-Id: I200a3b4804bd7e815a4f053ca596b41f7c4c4c24
Fixes-issue: CLIENT-404
Reviewed-on: http://gerrit.dmdirc.com/2795
Automatic-Compile: DMDirc Build Manager
Reviewed-by: Greg Holmes <greg@dmdirc.com>

src/com/dmdirc/updater/manager/UpdateManagerImpl.java

@@ -56,36 +56,31 @@ import lombok.extern.slf4j.Slf4j;
 public class UpdateManagerImpl implements UpdateManager {
 
     /** Collection of known update checking strategies. */
-    private final List<UpdateCheckStrategy> checkers
-            = new CopyOnWriteArrayList<UpdateCheckStrategy>();
+    private final List<UpdateCheckStrategy> checkers = new CopyOnWriteArrayList<>();
 
     /** Collection of known update retrieval strategies. */
-    private final List<UpdateRetrievalStategy> retrievers
-            = new CopyOnWriteArrayList<UpdateRetrievalStategy>();
+    private final List<UpdateRetrievalStategy> retrievers = new CopyOnWriteArrayList<>();
 
     /** Collection of known update installation strategies. */
-    private final List<UpdateInstallationStrategy> installers
-            = new CopyOnWriteArrayList<UpdateInstallationStrategy>();
+    private final List<UpdateInstallationStrategy> installers = new CopyOnWriteArrayList<>();
 
-    /** Map of known component names to their components. */
-    private final Map<String, UpdateComponent> components
-            = new HashMap<String, UpdateComponent>();
+    /** Map of known component names to their components. Guarded by {@link #componentsLock}. */
+    private final Map<String, UpdateComponent> components = new HashMap<>();
 
     /** Listener used to proxy retrieval events. */
-    private final UpdateRetrievalListener retrievalListener
-            = new RetrievalListener();
+    private final UpdateRetrievalListener retrievalListener = new RetrievalListener();
 
     /** Listener used to proxy setRetrievalResult events. */
-    private final UpdateInstallationListener installationListener
-            = new InstallListener();
+    private final UpdateInstallationListener installationListener = new InstallListener();
 
     /** Cache of update check results. */
-    private final Map<UpdateComponent, UpdateCheckResult> checkResults
-            = new HashMap<UpdateComponent, UpdateCheckResult>();
+    private final Map<UpdateComponent, UpdateCheckResult> checkResults = new HashMap<>();
 
     /** Cache of retrieval results. */
-    private final Map<UpdateComponent, UpdateRetrievalResult> retrievalResults
-            = new HashMap<UpdateComponent, UpdateRetrievalResult>();
+    private final Map<UpdateComponent, UpdateRetrievalResult> retrievalResults = new HashMap<>();
+
+    /** Lock for accessing {@link #components}. */
+    private final Object componentsLock = new Object();
 
     /** Executor to use to schedule retrieval and installation jobs. */
     private final Executor executor;
@@ -125,14 +120,19 @@ public class UpdateManagerImpl implements UpdateManager {
     @Override
     public void addComponent(final UpdateComponent component) {
         log.trace("Adding new component: {}", component);
-        this.components.put(component.getName(), component);
+        synchronized (componentsLock) {
+            this.components.put(component.getName(), component);
+        }
     }
 
     /** {@inheritDoc} */
     @Override
     public void removeComponent(final UpdateComponent component) {
         log.trace("Removing component: {}", component);
-        this.components.remove(component.getName());
+        synchronized (componentsLock) {
+            this.components.remove(component.getName());
+        }
+
         this.checkResults.remove(component);
         this.retrievalResults.remove(component);
     }
@@ -140,7 +140,9 @@ public class UpdateManagerImpl implements UpdateManager {
     /** {@inheritDoc} */
     @Override
     public Collection<UpdateComponent> getComponents() {
-        return Collections.unmodifiableCollection(this.components.values());
+        synchronized (componentsLock) {
+            return Collections.unmodifiableCollection(this.components.values());
+        }
     }
 
     /** {@inheritDoc} */
@@ -152,27 +154,36 @@ public class UpdateManagerImpl implements UpdateManager {
     /** {@inheritDoc} */
     @Override
     public void checkForUpdates() {
-        final Collection<Map<UpdateComponent, UpdateCheckResult>> results
-                = new ArrayList<Map<UpdateComponent, UpdateCheckResult>>();
+        final Collection<Map<UpdateComponent, UpdateCheckResult>> results = new ArrayList<>();
 
         log.info("Checking for updates for {} components using {} strategies",
                 components.size(), checkers.size());
         log.trace("Components: {}", components);
         log.trace("Strategies: {}", checkers);
 
-        final List<UpdateComponent> enabledComponents
-                = new ArrayList<UpdateComponent>(components.size());
-
-        for (UpdateComponent component : components.values()) {
-            if (policy.canCheck(component)) {
-                fireUpdateStatusChanged(component, UpdateStatus.CHECKING, 0);
-                enabledComponents.add(component);
-            } else {
-                log.debug("Checking for updates for {} denied by policy", component.getName());
-                fireUpdateStatusChanged(component, UpdateStatus.CHECKING_NOT_PERMITTED, 0);
+        final List<UpdateComponent> enabledComponents = new ArrayList<>(components.size());
+        final List<UpdateComponent> disabledComponents = new ArrayList<>(components.size());
+
+        synchronized (componentsLock) {
+            for (UpdateComponent component : components.values()) {
+                if (policy.canCheck(component)) {
+                    enabledComponents.add(component);
+                } else {
+                    log.debug("Checking for updates for {} denied by policy", component.getName());
+                    disabledComponents.add(component);
+                }
             }
         }
 
+        // Fire the listeners now we don't care about concurrent modifications.
+        for (UpdateComponent component : enabledComponents) {
+            fireUpdateStatusChanged(component, UpdateStatus.CHECKING, 0);
+        }
+
+        for (UpdateComponent component : disabledComponents) {
+            fireUpdateStatusChanged(component, UpdateStatus.CHECKING_NOT_PERMITTED, 0);
+        }
+
         for (UpdateCheckStrategy strategy : checkers) {
             results.add(strategy.checkForUpdates(enabledComponents));
         }

test/com/dmdirc/updater/manager/UpdateManagerImplTest.java

@@ -0,0 +1,84 @@
+/*
+ * Copyright (c) 2006-2013 DMDirc Developers
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package com.dmdirc.updater.manager;
+
+import com.dmdirc.updater.UpdateComponent;
+import com.dmdirc.updater.checking.CheckResultConsolidator;
+
+import java.util.concurrent.Executor;
+
+import lombok.extern.slf4j.Slf4j;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.mockito.stubbing.Answer;
+
+import static org.mockito.Mockito.*;
+
+@Slf4j
+@RunWith(MockitoJUnitRunner.class)
+public class UpdateManagerImplTest {
+
+    @Mock private Executor executor;
+    @Mock private CheckResultConsolidator checkResultConsolidator;
+    @Mock private UpdateComponentPolicy updateComponentPolicy;
+    @Mock private UpdateComponent component1;
+    @Mock private UpdateComponent component2;
+    @Mock private UpdateComponent component3;
+    @Mock private UpdateStatusListener statusListener;
+    private UpdateManagerImpl manager;
+
+    @Before
+    public void setup() {
+        manager = new UpdateManagerImpl(executor, checkResultConsolidator, updateComponentPolicy);
+        when(component1.getName()).thenReturn("test1");
+        when(component2.getName()).thenReturn("test2");
+        when(component3.getName()).thenReturn("test3");
+    }
+
+    @Test
+    public void testRemovingComponentInStatusChangeListener() {
+        manager.addComponent(component1);
+        manager.addComponent(component2);
+
+        manager.addUpdateStatusListener(statusListener);
+
+        doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(final InvocationOnMock invocation) throws Throwable {
+                // Add a new component from the manager while it's checking for updates.
+                // This potentially causes a CME, see CLIENT-404.
+                manager.addComponent(component3);
+                return null;
+            }
+        }).when(statusListener).updateStatusChanged(component1, UpdateStatus.CHECKING, 0);
+
+        when(updateComponentPolicy.canCheck(any(UpdateComponent.class))).thenReturn(true);
+
+        manager.checkForUpdates();
+    }
+
+}