Merge pull request #2579 from metamx/closerIsCloser

Make CloserRule use guava's Closer

processing/src/test/java/io/druid/segment/CloserRule.java

@@ -19,15 +19,14 @@
 
 package io.druid.segment;
 
-import com.google.common.collect.Lists;
+import com.google.common.io.Closer;
 import com.metamx.common.logger.Logger;
 import org.junit.rules.TestRule;
 import org.junit.runner.Description;
 import org.junit.runners.model.Statement;
 
+import java.io.Closeable;
 import java.io.IOException;
-import java.util.LinkedList;
-import java.util.List;
 
 public class CloserRule implements TestRule
 {
@@ -38,8 +37,8 @@ public class CloserRule implements TestRule
     this.throwException = throwException;
   }
 
-  private static final Logger log = new Logger(CloserRule.class);
-  private final List<AutoCloseable> autoCloseables = new LinkedList<>();
+  private static final Logger LOG = new Logger(CloserRule.class);
+  private final Closer closer = Closer.create();
 
   @Override
   public Statement apply(
@@ -51,53 +50,38 @@ public class CloserRule implements TestRule
       @Override
       public void evaluate() throws Throwable
       {
-        Throwable baseThrown = null;
         try {
           base.evaluate();
         }
         catch (Throwable e) {
-          baseThrown = e;
+          throw closer.rethrow(e);
         }
         finally {
-          Throwable exception = null;
-          for (AutoCloseable autoCloseable : Lists.reverse(autoCloseables)) {
-            try {
-              autoCloseable.close();
-            }
-            catch (Exception e) {
-              exception = suppressOrSet(exception, e);
-            }
-          }
-          autoCloseables.clear();
-          if (exception != null) {
-            if (throwException && baseThrown == null) {
-              throw exception;
-            } else if (baseThrown != null) {
-              baseThrown.addSuppressed(exception);
-            } else {
-              log.error(exception, "Exception closing resources");
-            }
-          }
-          if (baseThrown != null) {
-            throw baseThrown;
-          }
+          closer.close();
         }
       }
     };
   }
 
-  private static Throwable suppressOrSet(Throwable prior, Throwable other)
+  public <T extends Closeable> T closeLater(final T closeable)
   {
-    if (prior == null) {
-      prior = new IOException("Error closing resources");
-    }
-    prior.addSuppressed(other);
-    return prior;
-  }
-
-  public <T extends AutoCloseable> T closeLater(T autoCloseable)
-  {
-    autoCloseables.add(autoCloseable);
-    return autoCloseable;
+    closer.register(new Closeable()
+    {
+      @Override
+      public void close() throws IOException
+      {
+        if (throwException) {
+          closeable.close();
+        } else {
+          try {
+            closeable.close();
+          }
+          catch (IOException e) {
+            LOG.warn(e, "Error closing [%s]", closeable);
+          }
+        }
+      }
+    });
+    return closeable;
   }
 }

processing/src/test/java/io/druid/segment/CloserRuleTest.java

@@ -19,18 +19,14 @@
 
 package io.druid.segment;
 
-import com.google.common.base.Function;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Lists;
 import com.google.common.util.concurrent.Runnables;
 import org.junit.Assert;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.runner.Description;
 import org.junit.runners.model.Statement;
 
-import javax.annotation.Nullable;
 import java.io.Closeable;
 import java.io.IOException;
 import java.util.Arrays;
@@ -42,7 +38,8 @@ import java.util.concurrent.atomic.AtomicLong;
 public class CloserRuleTest
 {
   @Rule
-  public final CloserRule closer = new CloserRule(true);
+  public final ExpectedException expectedException = ExpectedException.none();
+
   @Test
   public void testCloses() throws Throwable
   {
@@ -62,25 +59,6 @@ public class CloserRuleTest
     Assert.assertTrue(closed.get());
   }
 
-  @Test
-  public void testAutoCloses() throws Throwable
-  {
-    final CloserRule closer = new CloserRule(false);
-    final AtomicBoolean closed = new AtomicBoolean(false);
-    closer.closeLater(
-        new AutoCloseable()
-        {
-          @Override
-          public void close() throws Exception
-          {
-            closed.set(true);
-          }
-        }
-    );
-    run(closer, Runnables.doNothing());
-    Assert.assertTrue(closed.get());
-  }
-
   @Test
   public void testPreservesException() throws Throwable
   {
@@ -122,18 +100,19 @@ public class CloserRuleTest
 
 
   @Test
-  public void testAddsSuppressed() throws Throwable
+  public void testSuppressed() throws Throwable
   {
-    final CloserRule closer = new CloserRule(false);
+    final CloserRule closer = new CloserRule(true);
     final AtomicBoolean closed = new AtomicBoolean(false);
     final String ioExceptionMsg = "You can't triple stamp a double stamp!";
+    final IOException suppressed = new IOException(ioExceptionMsg);
     closer.closeLater(
         new Closeable()
         {
           @Override
           public void close() throws IOException
           {
-            throw new IOException(ioExceptionMsg);
+            throw suppressed;
           }
         }
     );
@@ -149,6 +128,8 @@ public class CloserRuleTest
     );
 
     final String msg = "You can't divide by zero, you can only take the limit of such!";
+    final ArithmeticException arithmeticException = new ArithmeticException(msg);
+
     Throwable ex = null;
     try {
       run(
@@ -157,7 +138,7 @@ public class CloserRuleTest
             @Override
             public void run()
             {
-              throw new ArithmeticException(msg);
+              throw arithmeticException;
             }
           }
       );
@@ -165,28 +146,10 @@ public class CloserRuleTest
     catch (Throwable e) {
       ex = e;
     }
-    Assert.assertTrue(closed.get());
+    Assert.assertEquals(arithmeticException, ex);
     Assert.assertNotNull(ex);
-    Assert.assertTrue(ex instanceof ArithmeticException);
-    Assert.assertEquals(msg, ex.getMessage());
-    Assert.assertEquals(
-        ImmutableList.of(ioExceptionMsg),
-        Lists.transform(
-            Arrays.asList(ex.getSuppressed()),
-            new Function<Throwable, String>()
-            {
-              @Nullable
-              @Override
-              public String apply(@Nullable Throwable input)
-              {
-                if (input == null) {
-                  return null;
-                }
-                return input.getSuppressed()[0].getMessage();
-              }
-            }
-        )
-    );
+    Assert.assertNotNull(ex.getSuppressed());
+    Assert.assertEquals(suppressed, ex.getSuppressed()[0]);
   }
 
   @Test
@@ -194,13 +157,14 @@ public class CloserRuleTest
   {
     final CloserRule closer = new CloserRule(true);
     final String ioExceptionMsg = "You can't triple stamp a double stamp!";
+    final IOException ioException = new IOException(ioExceptionMsg);
     closer.closeLater(
         new Closeable()
         {
           @Override
           public void close() throws IOException
           {
-            throw new IOException(ioExceptionMsg);
+            throw ioException;
           }
         }
     );
@@ -211,11 +175,7 @@ public class CloserRuleTest
     catch (Throwable throwable) {
       ex = throwable;
     }
-    Assert.assertNotNull(ex);
-    ex = ex.getSuppressed()[0];
-    Assert.assertNotNull(ex);
-    Assert.assertTrue(ex instanceof IOException);
-    Assert.assertEquals(ioExceptionMsg, ex.getMessage());
+    Assert.assertEquals(ioException, ex);
   }
 
 
@@ -263,10 +223,10 @@ public class CloserRuleTest
         }
     );
     closer.closeLater(
-        new AutoCloseable()
+        new Closeable()
         {
           @Override
-          public void close() throws Exception
+          public void close() throws IOException
           {
             throw new IOException(ioExceptionMsg);
           }
@@ -289,7 +249,7 @@ public class CloserRuleTest
         new IOException(ioExceptionMsg),
         null
     );
-    for(final IOException throwable : ioExceptions){
+    for (final IOException throwable : ioExceptions) {
       closer.closeLater(
           new Closeable()
           {
@@ -297,22 +257,7 @@ public class CloserRuleTest
             public void close() throws IOException
             {
               counter.incrementAndGet();
-              if(throwable != null){
-                throw throwable;
-              }
-            }
-          }
-      );
-    }
-    for(final IOException throwable : ioExceptions){
-      closer.closeLater(
-          new AutoCloseable()
-          {
-            @Override
-            public void close() throws Exception
-            {
-              counter.incrementAndGet();
-              if(throwable != null){
+              if (throwable != null) {
                 throw throwable;
               }
             }
@@ -322,28 +267,13 @@ public class CloserRuleTest
     Throwable ex = null;
     try {
       run(closer, Runnables.doNothing());
-    }catch (Throwable throwable) {
+    }
+    catch (Throwable throwable) {
       ex = throwable;
     }
     Assert.assertNotNull(ex);
-    Assert.assertEquals(ioExceptions.size() * 2, counter.get());
-    Assert.assertEquals(ioExceptions.size(), ex.getSuppressed().length);
-  }
-
-  @Ignore // This one doesn't quite work right, it will throw the IOException, but JUnit doesn't detect it properly and treats it as suppressed instead
-  @Test(expected = IOException.class)
-  public void testCloserException()
-  {
-    closer.closeLater(
-        new Closeable()
-        {
-          @Override
-          public void close() throws IOException
-          {
-            throw new IOException("This is a test");
-          }
-        }
-    );
+    Assert.assertEquals(ioExceptions.size(), counter.get());
+    Assert.assertEquals(2, ex.getSuppressed().length);
   }
 
   private void run(CloserRule closer, final Runnable runnable) throws Throwable