From a64979463f4b326b3a5ac0131259f5040dc53ee4 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Wed, 2 Mar 2016 09:11:14 -0800 Subject: [PATCH] Make CloserRule use guava's Closer --- .../java/io/druid/segment/CloserRule.java | 66 ++++------ .../java/io/druid/segment/CloserRuleTest.java | 118 ++++-------------- 2 files changed, 49 insertions(+), 135 deletions(-) diff --git a/processing/src/test/java/io/druid/segment/CloserRule.java b/processing/src/test/java/io/druid/segment/CloserRule.java index 1a64aa2d0ec..8e23e1a3681 100644 --- a/processing/src/test/java/io/druid/segment/CloserRule.java +++ b/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 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 closeLater(final T closeable) { - if (prior == null) { - prior = new IOException("Error closing resources"); - } - prior.addSuppressed(other); - return prior; - } - - public 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; } } diff --git a/processing/src/test/java/io/druid/segment/CloserRuleTest.java b/processing/src/test/java/io/druid/segment/CloserRuleTest.java index 81a8ee8a3a5..c17e2677f01 100644 --- a/processing/src/test/java/io/druid/segment/CloserRuleTest.java +++ b/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() - { - @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