ConstantTimeBackoff test and cleanup

- javadoc changed - the old one was copy-pasted from AgentShell
- start and stop method removed - they did the same as the overridden methods
- _counter removed as it was only written, but never read
- remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
- Tests created for the above scenarios.

Signed-off-by: Laszlo Hornyak <laszlo.hornyak@gmail.com>

agent/src/com/cloud/agent/AgentShell.java

@@ -19,14 +19,11 @@ package com.cloud.agent;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
-import java.io.FileOutputStream;
 import java.io.IOException;
-import java.io.InputStream;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Date;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.List;
@@ -39,9 +36,7 @@ import javax.naming.ConfigurationException;
 import org.apache.commons.daemon.Daemon;
 import org.apache.commons.daemon.DaemonContext;
 import org.apache.commons.daemon.DaemonInitException;
-import org.apache.commons.httpclient.HttpClient;
 import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager;
-import org.apache.commons.httpclient.methods.GetMethod;
 import org.apache.log4j.Logger;
 import org.apache.log4j.xml.DOMConfigurator;
 
@@ -56,7 +51,6 @@ import com.cloud.utils.PropertiesUtil;
 import com.cloud.utils.backoff.BackoffAlgorithm;
 import com.cloud.utils.backoff.impl.ConstantTimeBackoff;
 import com.cloud.utils.exception.CloudRuntimeException;
-import com.cloud.utils.script.Script;
 
 public class AgentShell implements IAgentShell, Daemon {
     private static final Logger s_logger = Logger.getLogger(AgentShell.class

utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java

@@ -19,16 +19,19 @@ package com.cloud.utils.backoff.impl;
 import java.util.Collection;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 
 import javax.ejb.Local;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.backoff.BackoffAlgorithm;
 import com.cloud.utils.component.AdapterBase;
 
 /**
- * Implementation of the Agent Manager.  This class controls the connection
+ * An implementation of BackoffAlgorithm that waits for some seconds.
+ * After the time the client can try to perform the operation again.
  * 
  * @config
  * {@table 
@@ -38,27 +41,29 @@ import com.cloud.utils.component.AdapterBase;
  **/ 
 @Local(value={BackoffAlgorithm.class})
 public class ConstantTimeBackoff extends AdapterBase implements BackoffAlgorithm, ConstantTimeBackoffMBean {
-    int _count = 0;
     long _time;
-    ConcurrentHashMap<String, Thread> _asleep = new ConcurrentHashMap<String, Thread>();
+    private final ConcurrentHashMap<String, Thread> _asleep = new ConcurrentHashMap<String, Thread>();
+    private final static Log LOG = LogFactory.getLog(ConstantTimeBackoff.class);
 
     @Override
     public void waitBeforeRetry() {
-        _count++;
+        Thread current = Thread.currentThread();
         try {
-            Thread current = Thread.currentThread();
             _asleep.put(current.getName(), current);
             Thread.sleep(_time);
+        } catch (InterruptedException e) {
+            // JMX or other threads may interrupt this thread, but let's log it
+            // anyway, no exception to log as this is not an error
+            LOG.info("Thread " + current.getName()
+                    + " interrupted while waiting for retry");
+        } finally {
             _asleep.remove(current.getName());
-        } catch(InterruptedException e) {
-            
         }
         return;
     }
 
     @Override
     public void reset() {
-        _count = 0;
     }
 
     @Override
@@ -71,7 +76,7 @@ public class ConstantTimeBackoff extends AdapterBase implements BackoffAlgorithm
     public Collection<String> getWaiters() {
         return _asleep.keySet();
     }
-    
+
     @Override
     public boolean wakeup(String threadName) {
         Thread th = _asleep.get(threadName);
@@ -83,17 +88,6 @@ public class ConstantTimeBackoff extends AdapterBase implements BackoffAlgorithm
         return false;
     }
 
-    @Override
-    public boolean start() {
-        _count = 0;
-        return true;
-    }
-
-    @Override
-    public boolean stop() {
-        return true;
-    }
-
     @Override
     public long getTimeToWait() {
         return _time;

utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java

@@ -0,0 +1,110 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// 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 com.cloud.utils.backoff.impl;
+
+import java.util.HashMap;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ConstantTimeBackoffTest {
+    final static private Log LOG = LogFactory
+            .getLog(ConstantTimeBackoffTest.class);
+
+    @Test
+    public void waitBeforeRetryWithInterrupt() throws InterruptedException {
+        final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
+        backoff.setTimeToWait(10);
+        Assert.assertTrue(backoff.getWaiters().isEmpty());
+        Thread waitThread = new Thread(new Runnable() {
+            @Override
+            public void run() {
+                backoff.waitBeforeRetry();
+            }
+        });
+        waitThread.start();
+        Thread.sleep(100);
+        Assert.assertFalse(backoff.getWaiters().isEmpty());
+        waitThread.interrupt();
+        Thread.sleep(100);
+        Assert.assertTrue(backoff.getWaiters().isEmpty());
+    }
+
+    @Test
+    public void waitBeforeRetry() throws InterruptedException {
+        final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
+        // let's not wait too much in a test
+        backoff.setTimeToWait(0);
+        // check if the list of waiters is empty
+        Assert.assertTrue(backoff.getWaiters().isEmpty());
+        // call the waitBeforeRetry which will wait 0 ms and return
+        backoff.waitBeforeRetry();
+        // on normal exit the list of waiters should be cleared
+        Assert.assertTrue(backoff.getWaiters().isEmpty());
+    }
+
+    @Test
+    public void configureEmpty() {
+        // at this point this is the only way rhe configure method gets invoked,
+        // therefore have to make sure it works correctly
+        final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
+        backoff.configure("foo", new HashMap<String, Object>());
+        Assert.assertEquals(5000, backoff.getTimeToWait());
+    }
+
+    @Test
+    public void configureWithValue() {
+        final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
+        HashMap<String, Object> params = new HashMap<String, Object>();
+        params.put("seconds", "100");
+        backoff.configure("foo", params);
+        Assert.assertEquals(100000, backoff.getTimeToWait());
+    }
+
+    /**
+     * Test that wakeup returns false when trying to wake a non existing thread.
+     */
+    @Test
+    public void wakeupNotExisting() {
+        final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
+        Assert.assertFalse(backoff.wakeup("NOT EXISTING THREAD"));
+    }
+
+    /**
+     * Test that wakeup will return true if the thread is waiting.
+     */
+    @Test
+    public void wakeupExisting() throws InterruptedException {
+        final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
+        backoff.setTimeToWait(10);
+        Thread thread = new Thread(new Runnable() {
+            @Override
+            public void run() {
+                LOG.debug("before");
+                backoff.waitBeforeRetry();
+                LOG.debug("after");
+            }
+        });
+        thread.start();
+        LOG.debug("thread started");
+        Thread.sleep(100);
+        LOG.debug("testing wakeup");
+        Assert.assertTrue(backoff.wakeup(thread.getName()));
+    }
+}