Merge pull request #98 from alasdairhodge/Issue-631

#631 Avoid confusion over time units, in particular don't mistake millise

compute/src/main/java/org/jclouds/compute/predicates/RetryIfSocketNotYetOpen.java

@@ -45,41 +45,61 @@ public class RetryIfSocketNotYetOpen implements Predicate<IPSocket> {
    @Resource
    @Named(ComputeServiceConstants.COMPUTE_LOGGER)
    private Logger logger = Logger.NULL;
-
    private final SocketOpen socketTester;
-   private long seconds;
+   private long timeoutValue;
+   private TimeUnit timeoutUnits;
 
-   public RetryIfSocketNotYetOpen seconds(long seconds) {
-      this.seconds = seconds;
-      return this;
+
+   public RetryIfSocketNotYetOpen(SocketOpen socketTester, Logger logger, long timeoutValue, TimeUnit timeoutUnits) {
+      this.socketTester = socketTester;
+      this.logger = logger;
+      this.timeoutValue = timeoutValue;
+      this.timeoutUnits = timeoutUnits;
+   }
+   
+   public RetryIfSocketNotYetOpen(SocketOpen socketTester, Logger logger) {
+      this(socketTester, logger, 0, TimeUnit.MILLISECONDS);
    }
 
    @Inject
    public RetryIfSocketNotYetOpen(SocketOpen socketTester, Timeouts timeouts) {
-      this.socketTester = socketTester;
-      this.seconds = timeouts.portOpen;
+       this(socketTester, Logger.NULL, timeouts.portOpen, TimeUnit.MILLISECONDS);
+   }
+   
+   /** @deprecated in favor of specifying explicit time units */
+   @Deprecated
+   public RetryIfSocketNotYetOpen(SocketOpen socketTester, Logger logger, long seconds) {
+      this(socketTester, logger, seconds, TimeUnit.SECONDS);
    }
 
-   public RetryIfSocketNotYetOpen(SocketOpen socketTester, Logger logger, long seconds) {
-      this.socketTester = socketTester;
-      this.logger = logger;
-      this.seconds = seconds;
+   public RetryIfSocketNotYetOpen milliseconds(long milliseconds) {
+       this.timeoutValue = milliseconds;
+       this.timeoutUnits = TimeUnit.MILLISECONDS;
+       return this;
+   }
+   
+   public RetryIfSocketNotYetOpen seconds(long seconds) {
+      this.timeoutValue = seconds;
+      this.timeoutUnits = TimeUnit.SECONDS;
+      return this;
    }
 
    @Override
    public String toString() {
-      return "retryIfSocketNotYetOpen(" + seconds + ")";
+      return "retryIfSocketNotYetOpen(" + timeoutValue + " "+ timeoutUnits + ")";
    }
 
    @Override
    public boolean apply(IPSocket socket) {
-      logger.debug(">> blocking on socket %s for %d seconds", socket, seconds);
-      RetryablePredicate<IPSocket> tester = new RetryablePredicate<IPSocket>(socketTester, seconds, 1, TimeUnit.SECONDS);
+      logger.debug(">> blocking on socket %s for %d %s", socket, timeoutValue, timeoutUnits);
+      // Specify a retry period of 1s, expressed in the same time units.
+      long period = timeoutUnits.convert(1, TimeUnit.SECONDS);
+      RetryablePredicate<IPSocket> tester = new RetryablePredicate<IPSocket>(socketTester, timeoutValue, period, timeoutUnits);
       boolean passed = tester.apply(socket);
       if (passed)
          logger.debug("<< socket %s opened", socket);
       else
-         logger.warn("<< socket %s didn't open after %d seconds", socket, seconds);
+         logger.warn("<< socket %s didn't open after %d %s", socket, timeoutValue, timeoutUnits);
       return passed;
    }
 }

compute/src/test/java/org/jclouds/compute/predicates/RetryIfSocketNotYetOpenTest.java

@@ -0,0 +1,83 @@
+/**
+ * Licensed to jclouds, Inc. (jclouds) under one or more
+ * contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  jclouds licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.jclouds.compute.predicates;
+
+import static org.testng.Assert.*;
+import static org.jclouds.compute.predicates.SocketOpenPredicates.*;
+
+import java.util.concurrent.TimeUnit;
+
+import org.jclouds.compute.reference.ComputeServiceConstants.Timeouts;
+import org.jclouds.logging.Logger;
+import org.jclouds.net.IPSocket;
+import org.testng.annotations.Test;
+
+
+/**
+ * Tests timeout behavior of {@link RetryIfSocketNotYetOpen} predicate.
+ */
+public class RetryIfSocketNotYetOpenTest {
+
+    private static final Logger logger = Logger.NULL;
+    private static final IPSocket socket = new IPSocket("dummy", 0);
+    
+    @Test
+    public void fromConstructor() {
+        RetryIfSocketNotYetOpen fromSeconds = new RetryIfSocketNotYetOpen(alwaysFail, logger, 2000, TimeUnit.MILLISECONDS);
+        doAsserts(fromSeconds, socket, false, 2000, 3000);
+    }
+    
+    @Test
+    public void fromSeconds() {
+        RetryIfSocketNotYetOpen fromSeconds = new RetryIfSocketNotYetOpen(alwaysFail, logger).seconds(2);
+        doAsserts(fromSeconds, socket, false, 2000, 3000);
+    }
+
+    @Test
+    public void fromMilliseconds() {
+        RetryIfSocketNotYetOpen fromMilliseconds = new RetryIfSocketNotYetOpen(alwaysFail, logger).milliseconds(2000);
+        doAsserts(fromMilliseconds, socket, false, 2000, 3000);
+    }
+
+    @Test
+    public void fromTimeouts() {
+        Timeouts timeouts = new Timeouts() { { portOpen = 2 * 1000; } };
+        RetryIfSocketNotYetOpen fromTimeouts = new RetryIfSocketNotYetOpen(alwaysFail, timeouts);
+        doAsserts(fromTimeouts, socket, false, 2000, 3000);
+    }
+
+    @Test
+    public void timeoutUnset() {
+        // With no timeout specified, predicate should return immediately.
+        RetryIfSocketNotYetOpen uninitialised = new RetryIfSocketNotYetOpen(alwaysFail, logger);
+        doAsserts(uninitialised, socket, false, 0, 500);
+    }
+
+    private void doAsserts(RetryIfSocketNotYetOpen predicate, IPSocket socket, boolean expectedResult, long minMilliseconds, long maxMilliseconds) {
+        long startTime = System.currentTimeMillis();
+        boolean result = predicate.apply(socket);
+        long elapsedTime = System.currentTimeMillis() - startTime;
+        
+        assertEquals(result, expectedResult);
+        assertTrue(elapsedTime >= minMilliseconds && elapsedTime < maxMilliseconds,
+                "apply() returned after ~"+elapsedTime+"ms, expected it to take between "+
+                minMilliseconds+" and "+maxMilliseconds);
+    }
+
+}

compute/src/test/java/org/jclouds/compute/predicates/SocketOpenPredicates.java

@@ -0,0 +1,38 @@
+/**
+ * Licensed to jclouds, Inc. (jclouds) under one or more
+ * contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  jclouds licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.jclouds.compute.predicates;
+
+import org.jclouds.net.IPSocket;
+import org.jclouds.predicates.SocketOpen;
+
+
+/**
+ * For use in unit tests, e.g. {@link RetryIfSocketNotYetOpenTest}.
+ */
+public class SocketOpenPredicates {
+
+    public static final SocketOpen alwaysSucceed = new SocketOpen() {
+        @Override public boolean apply(IPSocket socket) { return true; }
+    };
+
+    public static final SocketOpen alwaysFail = new SocketOpen() {
+        @Override public boolean apply(IPSocket socket) { return false; }
+    };
+
+}