From 05a1d3e072a121eca23f8bc64d11187065e711b2 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 7 Dec 2015 11:33:25 +0100 Subject: 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 Reviewed-on: https://git.eclipse.org/r/62082 Tested-by: Hudson CI Reviewed-by: Jeff Johnston (cherry picked from commit 87a3781d972e8c61056418d351745e9eb5616eae) Reviewed-on: https://git.eclipse.org/r/62155 --- .../internal/docker/core/DockerConnection.java | 135 +++++++++++++++------ .../internal/docker/core/DockerContainer.java | 32 ++++- 2 files changed, 126 insertions(+), 41 deletions(-) (limited to 'containers/org.eclipse.linuxtools.docker.core/src/org/eclipse') 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 printIds = new HashSet(); + // containers sorted by name private List containers; + // containers indexed by id + private Map containersById; private boolean containersLoaded = false; private List 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 list) { if (containerListeners != null) { Object[] listeners = containerListeners.getListeners(); @@ -400,23 +408,14 @@ public class DockerConnection implements IDockerConnection, Closeable { @Override public List getContainers(final boolean force) { - List 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 listContainers() throws DockerException { - final List dclist = new ArrayList<>(); - synchronized (containerLock) { - List 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 updatedContainers = new HashMap<>(); + try { + final List 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() { + + @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 List sort(final Collection values, + final Comparator comparator) { + final List 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; -- cgit v1.2.3