Bug 483769 - DockerExplorerView flickers when container info is

displayed and refreshed

Added a map to track containers by id and re-use the previous container
  info when creating a new instance of DockerContainer for a given
container,
  so that associated containerInfo does not need to be initialized
again.
Making sure that containers are sorted by name.

Refactored the listContainers() method based on
  https://git.eclipse.org/r/#/c/54209/3

Change-Id: I46aed8ca720041d5d019f80eb75d0c2f78c5e0ea
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Reviewed-on: https://git.eclipse.org/r/62082
Tested-by: Hudson CI
Reviewed-by: Jeff Johnston <jjohnstn@redhat.com>
(cherry picked from commit 87a3781d972e8c61056418d351745e9eb5616eae)
Reviewed-on: https://git.eclipse.org/r/62155

2 files changed, 126 insertions, 41 deletions

diff --git a/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerConnection.java b/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerConnection.java
index 52ace62931..b266d2f248 100644
--- a/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerConnection.java
+++ b/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerConnection.java
@@ -18,7 +18,9 @@ import java.nio.ByteBuffer;
 import java.nio.channels.WritableByteChannel;
 import java.nio.file.FileSystems;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -163,7 +165,10 @@ public class DockerConnection implements IDockerConnection, Closeable {
 
 	// private Set<String> printIds = new HashSet<String>();
 
+	// containers sorted by name
 	private List<IDockerContainer> containers;
+	// containers indexed by id
+	private Map<String, IDockerContainer> containersById;
 	private boolean containersLoaded = false;
 	private List<IDockerImage> images;
 	private boolean imagesLoaded = false;
@@ -357,6 +362,9 @@ public class DockerConnection implements IDockerConnection, Closeable {
 		}
 	}
 
+	// TODO: we might need something more fine grained, to indicate which
+	// container changed, was added or was removed, so we can refresh the UI
+	// accordingly.
 	public void notifyContainerListeners(List<IDockerContainer> list) {
 		if (containerListeners != null) {
 			Object[] listeners = containerListeners.getListeners();
@@ -400,23 +408,14 @@ public class DockerConnection implements IDockerConnection, Closeable {
 
 	@Override
 	public List<IDockerContainer> getContainers(final boolean force) {
-		List<IDockerContainer> latestContainers;
-		synchronized (containerLock) {
-			latestContainers = this.containers;
-		}
 		if (!isContainersLoaded() || force) {
 			try {
-				latestContainers = listContainers();
+				return listContainers();
 			} catch (DockerException e) {
-				synchronized (containerLock) {
-					this.containers = Collections.emptyList();
-				}
 				Activator.log(e);
-			} finally {
-				this.containersLoaded = true;
 			}
 		}
-		return latestContainers;
+		return this.containers;
 	}
 
 	@Override
@@ -506,49 +505,105 @@ public class DockerConnection implements IDockerConnection, Closeable {
 	}
 
 	private List<IDockerContainer> listContainers() throws DockerException {
-		final List<IDockerContainer> dclist = new ArrayList<>();
-		synchronized (containerLock) {
-			List<Container> list = null;
-			try {
-				synchronized (clientLock) {
-					// Check that client is not null as this connection may have
-					// been closed but there is an async request to update the
-					// containers list left in the queue
-					if (client == null)
-						return dclist;
-					list = client.listContainers(
-							DockerClient.ListContainersParam.allContainers());
+		final Map<String, IDockerContainer> updatedContainers = new HashMap<>();
+		try {
+			final List<Container> nativeContainers = new ArrayList<>();
+			synchronized (clientLock) {
+				// Check that client is not null as this connection may have
+				// been closed but there is an async request to update the
+				// containers list left in the queue
+				if (client == null) {
+					// in that case the list becomes empty, which is fine is
+					// there's no client.
+					return Collections.emptyList();
 				}
-			} catch (com.spotify.docker.client.DockerException
-					| InterruptedException e) {
-				throw new DockerException(
-						NLS.bind(
-						Messages.List_Docker_Containers_Failure,
-						this.getName()), e);
+				nativeContainers.addAll(client.listContainers(
+						DockerClient.ListContainersParam.allContainers()));
 			}
-
 			// We have a list of containers. Now, we translate them to our own
 			// core format in case we decide to change the underlying engine
 			// in the future.
-			for (Container c : list) {
+			for (Container nativeContainer : nativeContainers) {
 				// For containers that have exited, make sure we aren't tracking
 				// them with a logging thread.
-				if (c.status().startsWith(Messages.Exited_specifier)) {
-					if (loggingThreads.containsKey(c.id())) {
-						loggingThreads.get(c.id()).requestStop();
-						loggingThreads.remove(c.id());
+				if (nativeContainer.status()
+						.startsWith(Messages.Exited_specifier)) {
+					synchronized (loggingThreads) {
+						if (loggingThreads.containsKey(nativeContainer.id())) {
+							loggingThreads.get(nativeContainer.id())
+									.requestStop();
+							loggingThreads.remove(nativeContainer.id());
+						}
 					}
 				}
-				if (!c.status().equals(Messages.Removal_In_Progress_specifier)) {
-					dclist.add(new DockerContainer(this, c));
+				// skip containers that are being removed
+				if (nativeContainer.status()
+						.equals(Messages.Removal_In_Progress_specifier)) {
+					continue;
+				}
+				// re-use info from existing container with same id
+				if (this.containers != null && this.containersById
+						.containsKey(nativeContainer.id())) {
+					final IDockerContainer container = this.containersById
+							.get(nativeContainer.id());
+					updatedContainers.put(nativeContainer.id(),
+							new DockerContainer(this, nativeContainer,
+									container.info()));
+				} else {
+					updatedContainers.put(nativeContainer.id(),
+							new DockerContainer(this, nativeContainer));
 				}
 			}
-			containers = dclist;
+		} catch (com.spotify.docker.client.DockerException
+				| InterruptedException e) {
+			throw new DockerException(
+					NLS.bind(Messages.List_Docker_Containers_Failure,
+							this.getName()),
+					e);
+		} finally {
+			// assign the new list of containers in a locked block of code to
+			// prevent concurrent access, even if an exception was raised.
+			synchronized (containerLock) {
+				this.containersById = updatedContainers;
+				this.containers = sort(this.containersById.values(),
+						new Comparator<IDockerContainer>() {
+
+							@Override
+							public int compare(final IDockerContainer container,
+									final IDockerContainer otherContainer) {
+								return container.name()
+										.compareTo(otherContainer.name());
+							}
+
+						});
+
+				this.containersLoaded = true;
+			}
 		}
+
 		// perform notification outside of containerLock so we don't have a View
 		// causing a deadlock
-		notifyContainerListeners(dclist);
-		return dclist;
+		// TODO: we should probably notify the listeners only if the containers
+		// list changed.
+		notifyContainerListeners(this.containers);
+		return this.containers;
+	}
+
+	/**
+	 * Sorts the given values using the given comparator and returns the result
+	 * in a {@link List}
+	 * 
+	 * @param values
+	 *            the values to sort
+	 * @param comparator
+	 *            the comparator to use
+	 * @return the list of sorted values
+	 */
+	private <T> List<T> sort(final Collection<T> values,
+			final Comparator<T> comparator) {
+		final List<T> result = new ArrayList<>(values);
+		Collections.sort(result, comparator);
+		return result;
 	}
 
 	@Override
diff --git a/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerContainer.java b/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerContainer.java
index 48f2397b39..e62f220728 100644
--- a/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerContainer.java
+++ b/containers/org.eclipse.linuxtools.docker.core/src/org/eclipse/linuxtools/internal/docker/core/DockerContainer.java
@@ -18,6 +18,7 @@ import org.eclipse.linuxtools.docker.core.IDockerContainer;
 import org.eclipse.linuxtools.docker.core.IDockerContainerInfo;
 import org.eclipse.linuxtools.docker.core.IDockerPortMapping;
 
+import com.spotify.docker.client.DockerClient;
 import com.spotify.docker.client.messages.Container;
 
 public class DockerContainer implements IDockerContainer {
@@ -34,8 +35,17 @@ public class DockerContainer implements IDockerContainer {
 	private Long sizeRootFs;
 	private IDockerContainerInfo containerInfo;
 
+	/**
+	 * Constructor.
+	 * 
+	 * @param connection
+	 *            the Docker connection
+	 * @param container
+	 *            the underlying {@link Container} data returned by the
+	 *            {@link DockerClient}
+	 */
 	public DockerContainer(final IDockerConnection connection,
-			Container container) {
+			final Container container) {
 		this.parent = connection;
 		this.id = container.id();
 		this.image = container.image();
@@ -62,6 +72,26 @@ public class DockerContainer implements IDockerContainer {
 		// TODO: include volumes
 	}
 
+	/**
+	 * Constructor.
+	 * 
+	 * @param connection
+	 *            the Docker connection
+	 * @param container
+	 *            the underlying {@link Container} data returned by the
+	 *            {@link DockerClient}
+	 * @param containerInfo
+	 *            the {@link IDockerContainerInfo} that was previously retrieved
+	 *            for this {@link IDockerContainer}, assuming it did not change
+	 *            in the mean time.
+	 */
+	public DockerContainer(final IDockerConnection connection,
+			final Container container,
+			final IDockerContainerInfo containerInfo) {
+		this(connection, container);
+		this.containerInfo = containerInfo;
+	}
+
 	@Override
 	public IDockerConnection getConnection() {
 		return parent;