Skip to content

Commit

Permalink
Merge pull request apache#2579 from metamx/closerIsCloser
Browse files Browse the repository at this point in the history 
Make CloserRule use guava's Closer
  • Loading branch information
@drcrallen
drcrallen committed Mar 15, 2016
2 parents 5ec5ac9 + a649794 commit 2ac8a22
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 135 deletions.
66 changes: 25 additions & 41 deletions processing/src/test/java/io/druid/segment/CloserRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -38,8 +37,8 @@ public CloserRule(boolean throwException)
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(
Expand All @@ -51,53 +50,38 @@ public Statement apply(
@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;
}
}
118 changes: 24 additions & 94 deletions processing/src/test/java/io/druid/segment/CloserRuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,7 +38,8 @@
public class CloserRuleTest
{
@Rule
public final CloserRule closer = new CloserRule(true);
public final ExpectedException expectedException = ExpectedException.none();

@Test
public void testCloses() throws Throwable
{
Expand All @@ -62,25 +59,6 @@ public void close() throws IOException
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
{
Expand Down Expand Up @@ -122,18 +100,19 @@ public void run()


@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;
}
}
);
Expand All @@ -149,6 +128,8 @@ public void close() throws IOException
);

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(
Expand All @@ -157,50 +138,33 @@ closer, new Runnable()
@Override
public void run()
{
throw new ArithmeticException(msg);
throw arithmeticException;
}
}
);
}
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
public void testThrowsCloseException()
{
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;
}
}
);
Expand All @@ -211,11 +175,7 @@ public void close() throws IOException
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);
}


Expand Down Expand Up @@ -263,10 +223,10 @@ public void close() throws IOException
}
);
closer.closeLater(
new AutoCloseable()
new Closeable()
{
@Override
public void close() throws Exception
public void close() throws IOException
{
throw new IOException(ioExceptionMsg);
}
Expand All @@ -289,30 +249,15 @@ public void testClosesEverything()
new IOException(ioExceptionMsg),
null
);
for(final IOException throwable : ioExceptions){
for (final IOException throwable : ioExceptions) {
closer.closeLater(
new Closeable()
{
@Override
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;
}
}
Expand All @@ -322,28 +267,13 @@ public void close() throws Exception
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
Expand Down

0 comments on commit 2ac8a22

Please sign in to comment.