Issue 346: fixed executor doesn't close problem

core/src/main/java/org/jclouds/concurrent/config/ExecutorServiceModule.java

@@ -72,8 +72,10 @@ public class ExecutorServiceModule extends AbstractModule {
       }
    }
 
-   private final ExecutorService userExecutorFromConstructor;
+   @VisibleForTesting
-   private final ExecutorService ioExecutorFromConstructor;
+   final ExecutorService userExecutorFromConstructor;
+   @VisibleForTesting
+   final ExecutorService ioExecutorFromConstructor;
 
    @Inject
    public ExecutorServiceModule(@Named(Constants.PROPERTY_USER_THREADS) ExecutorService userThreads,
@@ -107,7 +109,7 @@ public class ExecutorServiceModule extends AbstractModule {
    @Named(Constants.PROPERTY_USER_THREADS)
    ExecutorService provideExecutorService(@Named(Constants.PROPERTY_USER_THREADS) int count, Closer closer) {
       if (userExecutorFromConstructor != null)
-         return shutdownOnClose(userExecutorFromConstructor, closer);
+         return userExecutorFromConstructor;
       return shutdownOnClose(newThreadPoolNamed("user thread %d", count), closer);
    }
 
@@ -116,7 +118,7 @@ public class ExecutorServiceModule extends AbstractModule {
    @Named(Constants.PROPERTY_IO_WORKER_THREADS)
    ExecutorService provideIOExecutor(@Named(Constants.PROPERTY_IO_WORKER_THREADS) int count, Closer closer) {
       if (ioExecutorFromConstructor != null)
-         return shutdownOnClose(ioExecutorFromConstructor, closer);
+         return ioExecutorFromConstructor;
       return shutdownOnClose(newThreadPoolNamed("i/o thread %d", count), closer);
    }
 

core/src/main/java/org/jclouds/lifecycle/Closer.java

@@ -23,15 +23,20 @@ import java.io.Closeable;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
-import java.util.concurrent.CopyOnWriteArrayList;
+
+import javax.inject.Singleton;
+
+import com.google.common.collect.Lists;
 
 /**
  * This will close objects in the reverse order that they were added.
  * 
  * @author Adrian Cole
  */
+@Singleton
 public class Closer implements Closeable {
-   List<Closeable> methodsToClose = new CopyOnWriteArrayList<Closeable>();
+   // guice is single threaded. no need to lock this
+   List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
 
    public void addToClose(Closeable toClose) {
       methodsToClose.add(toClose);

core/src/test/java/org/jclouds/concurrent/config/ExecutorServiceModuleTest.java

@@ -19,16 +19,24 @@
 
 package org.jclouds.concurrent.config;
 
+import static org.easymock.EasyMock.expect;
+import static org.easymock.classextension.EasyMock.createMock;
+import static org.easymock.classextension.EasyMock.replay;
+import static org.easymock.classextension.EasyMock.verify;
+import static org.testng.Assert.assertEquals;
+
 import java.io.IOException;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 
+import org.jclouds.Constants;
 import org.jclouds.lifecycle.Closer;
-import org.testng.annotations.BeforeTest;
 import org.testng.annotations.Test;
 
+import com.google.common.collect.ImmutableList;
 import com.google.inject.Guice;
 import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.name.Names;
 
 /**
  * 
@@ -37,20 +45,53 @@ import com.google.inject.Injector;
 @Test
 public class ExecutorServiceModuleTest {
 
-   private Closer closer;
+   @Test
-
+   public void testShutdownOnClose() throws IOException {
-   @BeforeTest
-   public void setUp() throws Exception {
       Injector i = Guice.createInjector();
-      closer = i.getInstance(Closer.class);
+
+      Closer closer = i.getInstance(Closer.class);
+      ExecutorService executor = createMock(ExecutorService.class);
+      ExecutorServiceModule.shutdownOnClose(executor, closer);
+
+      expect(executor.shutdownNow()).andReturn(ImmutableList.<Runnable> of()).atLeastOnce();
+
+      replay(executor);
+      closer.close();
+
+      verify(executor);
    }
 
    @Test
-   public void testShutdownOnClose() throws IOException {
+   public void testShutdownOnCloseThroughModule() throws IOException {
-      ExecutorService executor = Executors.newCachedThreadPool();
+
-      assert !executor.isShutdown();
+      ExecutorServiceModule module = new ExecutorServiceModule() {
-      ExecutorServiceModule.shutdownOnClose(executor, closer);
+
+         @Override
+         protected void configure() {
+            bindConstant().annotatedWith(Names.named(Constants.PROPERTY_IO_WORKER_THREADS)).to(1);
+            bindConstant().annotatedWith(Names.named(Constants.PROPERTY_USER_THREADS)).to(1);
+            super.configure();
+         }
+
+      };
+      Injector i = Guice.createInjector(module);
+      assertEquals(module.userExecutorFromConstructor, null);
+      assertEquals(module.ioExecutorFromConstructor, null);
+
+      Closer closer = i.getInstance(Closer.class);
+
+      ExecutorService user = i
+               .getInstance(Key.get(ExecutorService.class, Names.named(Constants.PROPERTY_USER_THREADS)));
+      ExecutorService io = i.getInstance(Key.get(ExecutorService.class, Names
+               .named(Constants.PROPERTY_IO_WORKER_THREADS)));
+
+      assert !user.isShutdown();
+      assert !io.isShutdown();
+
       closer.close();
-      assert executor.isShutdown();
+
+      assert user.isShutdown();
+      assert io.isShutdown();
+
    }
 }