Wait robustness

CHANGELOG.md

@@ -9,13 +9,17 @@ All notable changes to this project will be documented in this file.
 - Allowing `addExposedPort` to be used after ports have been specified with `withExposedPorts` (#453)
 - Stopping creation of temporary directory prior to creating temporary file (#443)
 - Ensure that temp files are created in a temp directory (#423)
+- Added `WaitAllStrategy` as a mechanism for composing multiple startup `WaitStrategy` objects together
+- Changed `BrowserWebDriverContainer` to use improved wait strategies, to eliminate race conditions when starting VNC recording containers. This should lead to far fewer 'error' messages logged when starting up selenium containers, and less exposure to race related bugs (fixes #466). 
 
 ### Changed
 - Make Network instances reusable (i.e. work with `@ClassRule`) ([\#469](https://github.com/testcontainers/testcontainers-java/issues/469))
 - Added support for explicitly setting file mode when copying file into container ([\#446](https://github.com/testcontainers/testcontainers-java/issues/446), [\#467](https://github.com/testcontainers/testcontainers-java/issues/467)) 
 - Use Visible Assertions 2.1.0 for pre-flight test output (eliminating Jansi/JNR-POSIX dependencies for lower likelihood of conflict. JNA is now used internally by Visible Assertions instead).
 - Mark all links functionality as deprecated. This is pending removal in a later release. Please see [\#465](https://github.com/testcontainers/testcontainers-java/issues/465). {@link Network} features should be used instead.
 - Added support for copying files to/from running containers ([\#378](https://github.com/testcontainers/testcontainers-java/issues/378))
+- Add `getLivenessCheckPorts` as an eventual replacement for `getLivenessCheckPort`; this allows multiple ports to be included in post-startup wait strategies.
+- Refactor wait strategy port checking and improve test coverage.
 - Added support for customising the recording file name ([\#500](https://github.com/testcontainers/testcontainers-java/issues/500))
 
 ## [1.4.3] - 2017-10-14

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -9,8 +9,9 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import lombok.*;
-import org.apache.commons.compress.utils.IOUtils;
 import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
+import org.apache.commons.compress.utils.IOUtils;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.junit.runner.Description;
 import org.rnorth.ducttape.ratelimits.RateLimiter;
@@ -44,6 +45,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static com.google.common.collect.Lists.newArrayList;
 import static org.testcontainers.containers.output.OutputFrame.OutputType.STDERR;
@@ -96,7 +98,6 @@ public class GenericContainer<SELF extends GenericContainer<SELF>>
     @NonNull
     private List<Bind> binds = new ArrayList<>();
 
-    @NonNull
     private boolean privilegedMode;
 
     @NonNull
@@ -326,8 +327,11 @@ protected void containerIsStarted(InspectContainerResponse containerInfo) {
 
     /**
      * @return the port on which to check if the container is ready
+     * @deprecated see {@link GenericContainer#getLivenessCheckPorts()} for replacement
      */
+    @Deprecated
     protected Integer getLivenessCheckPort() {
+        // legacy implementation for backwards compatibility
         if (exposedPorts.size() > 0) {
             return getMappedPort(exposedPorts.get(0));
         } else if (portBindings.size() > 0) {
@@ -337,6 +341,39 @@ protected Integer getLivenessCheckPort() {
         }
     }
 
+    /**
+     * @return the ports on which to check if the container is ready
+     */
+    @NotNull
+    @NonNull
+    protected Set<Integer> getLivenessCheckPorts() {
+        final Set<Integer> result = new HashSet<>();
+        result.addAll(getExposedPortNumbers());
+        result.addAll(getBoundPortNumbers());
+
+        // for backwards compatibility
+        if (this.getLivenessCheckPort() != null) {
+            result.add(this.getLivenessCheckPort());
+        }
+
+        return result;
+    }
+
+    private List<Integer> getExposedPortNumbers() {
+        return exposedPorts.stream()
+                .map(this::getMappedPort)
+                .collect(Collectors.toList());
+    }
+
+    private List<Integer> getBoundPortNumbers() {
+        return portBindings.stream()
+                .map(PortBinding::parse)
+                .map(PortBinding::getBinding)
+                .map(Ports.Binding::getHostPortSpec)
+                .map(Integer::valueOf)
+                .collect(Collectors.toList());
+    }
+
     private void applyConfiguration(CreateContainerCmd createCommand) {
 
         // Set up exposed ports (where there are no host port bindings defined)
@@ -376,31 +413,16 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
             String alias = linkEntries.getKey();
             LinkableContainer linkableContainer = linkEntries.getValue();
 
-            Set<Link> links = dockerClient.listContainersCmd().exec().stream()
-                    .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
-                    .map(container -> new Link(container.getNames()[0], alias))
-                    .collect(Collectors.toSet());
+            Set<Link> links = findLinksFromThisContainer(alias, linkableContainer);
             allLinks.addAll(links);
 
-            boolean linkableContainerIsRunning = dockerClient.listContainersCmd().exec().stream()
-                    .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
-                    .map(com.github.dockerjava.api.model.Container::getId)
-                    .map(id -> dockerClient.inspectContainerCmd(id).exec())
-                    .anyMatch(linkableContainerInspectResponse -> linkableContainerInspectResponse.getState().getRunning());
-
-            if (!linkableContainerIsRunning) {
+            if (allLinks.size() == 0) {
                 throw new ContainerLaunchException("Aborting attempt to link to container " +
                         linkableContainer.getContainerName() +
                         " as it is not running");
             }
 
-            Set<String> linkedContainerNetworks = dockerClient.listContainersCmd().exec().stream()
-                    .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
-                    .filter(container -> container.getNetworkSettings() != null &&
-                            container.getNetworkSettings().getNetworks() != null)
-                    .flatMap(container -> container.getNetworkSettings().getNetworks().keySet().stream())
-                    .distinct()
-                    .collect(Collectors.toSet());
+            Set<String> linkedContainerNetworks = findAllNetworksForLinkedContainers(linkableContainer);
             allLinkedContainerNetworks.addAll(linkedContainerNetworks);
         }
 
@@ -443,6 +465,26 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
         createCommand.withLabels(Collections.singletonMap("org.testcontainers", "true"));
     }
 
+    private Set<Link> findLinksFromThisContainer(String alias, LinkableContainer linkableContainer) {
+        return dockerClient.listContainersCmd()
+                .withStatusFilter("running")
+                .exec().stream()
+                .flatMap(container -> Stream.of(container.getNames()))
+                .filter(name -> name.endsWith(linkableContainer.getContainerName()))
+                .map(name -> new Link(name, alias))
+                .collect(Collectors.toSet());
+    }
+
+    private Set<String> findAllNetworksForLinkedContainers(LinkableContainer linkableContainer) {
+        return dockerClient.listContainersCmd().exec().stream()
+                .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
+                .filter(container -> container.getNetworkSettings() != null &&
+                        container.getNetworkSettings().getNetworks() != null)
+                .flatMap(container -> container.getNetworkSettings().getNetworks().keySet().stream())
+                .distinct()
+                .collect(Collectors.toSet());
+    }
+
     /**
      * {@inheritDoc}
      */
@@ -1015,11 +1057,20 @@ protected Logger logger() {
 
         /**
          * @return the port on which to check if the container is ready
+         * @deprecated see {@link AbstractWaitStrategy#getLivenessCheckPorts()}
          */
+        @Deprecated
         protected Integer getLivenessCheckPort() {
             return container.getLivenessCheckPort();
         }
 
+        /**
+         * @return the ports on which to check if the container is ready
+         */
+        protected Set<Integer> getLivenessCheckPorts() {
+            return container.getLivenessCheckPorts();
+        }
+
         /**
          * @return the rate limiter to use
          */

core/src/main/java/org/testcontainers/containers/VncRecordingSidekickContainer.java

@@ -1,6 +1,7 @@
 package org.testcontainers.containers;
 
 import com.github.dockerjava.api.command.InspectContainerResponse;
+import org.jetbrains.annotations.NotNull;
 import org.testcontainers.containers.traits.LinkableContainer;
 import org.testcontainers.containers.traits.VncService;
 import org.testcontainers.utility.TestcontainersConfiguration;
@@ -10,6 +11,9 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
+import java.util.Set;
+
+import static java.util.Collections.emptySet;
 
 /**
  * 'Sidekick container' with the sole purpose of recording the VNC screen output from another container.
@@ -46,10 +50,11 @@ protected void containerIsStarting(InspectContainerResponse containerInfo) {
         // do nothing
     }
 
+    @NotNull
     @Override
-    protected Integer getLivenessCheckPort() {
+    protected Set<Integer> getLivenessCheckPorts() {
         // no liveness check needed
-        return null;
+        return emptySet();
     }
 
     @Override

core/src/main/java/org/testcontainers/containers/wait/HostPortWaitStrategy.java

@@ -1,19 +1,18 @@
 package org.testcontainers.containers.wait;
 
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang.SystemUtils;
 import org.rnorth.ducttape.TimeoutException;
 import org.rnorth.ducttape.unreliables.Unreliables;
-import org.testcontainers.DockerClientFactory;
 import org.testcontainers.containers.ContainerLaunchException;
 import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.dockerclient.DockerMachineClientProviderStrategy;
-import org.testcontainers.dockerclient.WindowsClientProviderStrategy;
+import org.testcontainers.containers.wait.internal.ExternalPortListeningCheck;
+import org.testcontainers.containers.wait.internal.InternalCommandPortListeningCheck;
 
-import java.net.Socket;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
 
 /**
  * Waits until a socket connection can be established on a port exposed or mapped by the container.
@@ -23,87 +22,40 @@
 @Slf4j
 public class HostPortWaitStrategy extends GenericContainer.AbstractWaitStrategy {
 
-    private static final String SUCCESS_MARKER = "TESTCONTAINERS_SUCCESS";
     @Override
     protected void waitUntilReady() {
-        final Integer port = getLivenessCheckPort();
-        if (null == port) {
-            log.debug("Liveness check port of {} is empty. Not waiting.", container.getContainerName());
+        final Set<Integer> externalLivenessCheckPorts = getLivenessCheckPorts();
+        if (externalLivenessCheckPorts.isEmpty()) {
+            log.debug("Liveness check ports of {} is empty. Not waiting.", container.getContainerName());
             return;
         }
 
-        Callable<Boolean> checkStrategy;
+        @SuppressWarnings("unchecked")
+        List<Integer> exposedPorts = container.getExposedPorts();
 
-        if (shouldCheckWithCommand()) {
-            List<Integer> exposedPorts = container.getExposedPorts();
+        final Set<Integer> internalPorts = getInternalPorts(externalLivenessCheckPorts, exposedPorts);
 
-            Integer exposedPort = exposedPorts.stream()
-                    .filter(it -> port.equals(container.getMappedPort(it)))
-                    .findFirst()
-                    .orElse(null);
+        Callable<Boolean> internalCheck = new InternalCommandPortListeningCheck(container, internalPorts);
 
-            if (null == exposedPort) {
-                log.warn("Liveness check port of {} is set to {}, but it's not listed in exposed ports.",
-                        container.getContainerName(), port);
-                return;
-            }
-
-            String[][] commands = {
-                     { "/bin/sh", "-c", "nc -vz -w 1 localhost " + exposedPort + " && echo " + SUCCESS_MARKER },
-                     { "/bin/bash", "-c", "</dev/tcp/localhost/" + exposedPort + " && echo " + SUCCESS_MARKER }
-            };
-
-            checkStrategy = () -> {
-                for (String[] command : commands) {
-                    try {
-                        if (container.execInContainer(command).getStdout().contains(SUCCESS_MARKER)) {
-                            return true;
-                        }
-                    } catch (InterruptedException e) {
-                        throw new RuntimeException(e);
-                    } catch (Exception e) {
-                        continue;
-                    }
-                }
-
-                return false;
-            };
-        } else {
-            checkStrategy = () -> {
-                new Socket(container.getContainerIpAddress(), port).close();
-                return true;
-            };
-        }
+        Callable<Boolean> externalCheck = new ExternalPortListeningCheck(container, externalLivenessCheckPorts);
 
         try {
             Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS, () -> {
-                return getRateLimiter().getWhenReady(() -> {
-                    try {
-                        return checkStrategy.call();
-                    } catch (Exception e) {
-                        throw new RuntimeException(e);
-                    }
-                });
+                return getRateLimiter().getWhenReady(() -> internalCheck.call() && externalCheck.call());
             });
 
         } catch (TimeoutException e) {
             throw new ContainerLaunchException("Timed out waiting for container port to open (" +
-                    container.getContainerIpAddress() + ":" + port + " should be listening)");
+                    container.getContainerIpAddress() +
+                    " ports: " +
+                    externalLivenessCheckPorts +
+                    " should be listening)");
         }
     }
 
-    private boolean shouldCheckWithCommand() {
-        // Special case for Docker for Mac, see #160
-        if (! DockerClientFactory.instance().isUsing(DockerMachineClientProviderStrategy.class) &&
-                SystemUtils.IS_OS_MAC_OSX) {
-            return true;
-        }
-
-        // Special case for Docker for Windows, see #160
-        if (DockerClientFactory.instance().isUsing(WindowsClientProviderStrategy.class)) {
-            return true;
-        }
-
-        return false;
+    private Set<Integer> getInternalPorts(Set<Integer> externalLivenessCheckPorts, List<Integer> exposedPorts) {
+        return exposedPorts.stream()
+                .filter(it -> externalLivenessCheckPorts.contains(container.getMappedPort(it)))
+                .collect(Collectors.toSet());
     }
 }

core/src/main/java/org/testcontainers/containers/wait/WaitAllStrategy.java

@@ -0,0 +1,38 @@
+package org.testcontainers.containers.wait;
+
+import org.rnorth.ducttape.timeouts.Timeouts;
+import org.testcontainers.containers.GenericContainer;
+
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Wait strategy that waits for a number of other strategies to pass in series.
+ */
+public class WaitAllStrategy implements WaitStrategy {
+
+    private final List<WaitStrategy> strategies = new ArrayList<>();
+    private Duration timeout = Duration.ofSeconds(30);
+
+    @Override
+    public void waitUntilReady(GenericContainer container) {
+        Timeouts.doWithTimeout((int) timeout.toMillis(), TimeUnit.MILLISECONDS, () -> {
+            for (WaitStrategy strategy : strategies) {
+                strategy.waitUntilReady(container);
+            }
+        });
+    }
+
+    public WaitAllStrategy withStrategy(WaitStrategy strategy) {
+        this.strategies.add(strategy);
+        return this;
+    }
+
+    @Override
+    public WaitStrategy withStartupTimeout(Duration startupTimeout) {
+        this.timeout = startupTimeout;
+        return this;
+    }
+}