From 6a8382773ed710d979e9c535e794445804ebe802 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 10 Sep 2010 17:47:24 -0700 Subject: [PATCH] Issue 346: fixed executor doesn't close problem --- .../config/ExecutorServiceModule.java | 10 +-- .../java/org/jclouds/lifecycle/Closer.java | 9 ++- .../config/ExecutorServiceModuleTest.java | 65 +++++++++++++++---- 3 files changed, 66 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/jclouds/concurrent/config/ExecutorServiceModule.java b/core/src/main/java/org/jclouds/concurrent/config/ExecutorServiceModule.java index d0580eb557..9d31244ab2 100644 --- a/core/src/main/java/org/jclouds/concurrent/config/ExecutorServiceModule.java +++ b/core/src/main/java/org/jclouds/concurrent/config/ExecutorServiceModule.java @@ -72,8 +72,10 @@ public class ExecutorServiceModule extends AbstractModule { } } - private final ExecutorService userExecutorFromConstructor; - private final ExecutorService ioExecutorFromConstructor; + @VisibleForTesting + 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); } diff --git a/core/src/main/java/org/jclouds/lifecycle/Closer.java b/core/src/main/java/org/jclouds/lifecycle/Closer.java index f7f3fe48ee..6346c295c0 100644 --- a/core/src/main/java/org/jclouds/lifecycle/Closer.java +++ b/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 methodsToClose = new CopyOnWriteArrayList(); + // guice is single threaded. no need to lock this + List methodsToClose = Lists. newArrayList(); public void addToClose(Closeable toClose) { methodsToClose.add(toClose); diff --git a/core/src/test/java/org/jclouds/concurrent/config/ExecutorServiceModuleTest.java b/core/src/test/java/org/jclouds/concurrent/config/ExecutorServiceModuleTest.java index d053a9eac7..2a90e04f0b 100644 --- a/core/src/test/java/org/jclouds/concurrent/config/ExecutorServiceModuleTest.java +++ b/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; - - @BeforeTest - public void setUp() throws Exception { + @Test + public void testShutdownOnClose() throws IOException { 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. of()).atLeastOnce(); + + replay(executor); + closer.close(); + + verify(executor); } @Test - public void testShutdownOnClose() throws IOException { - ExecutorService executor = Executors.newCachedThreadPool(); - assert !executor.isShutdown(); - ExecutorServiceModule.shutdownOnClose(executor, closer); + public void testShutdownOnCloseThroughModule() throws IOException { + + ExecutorServiceModule module = new ExecutorServiceModule() { + + @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(); + } }