Skip to content

Commit

Permalink
Unconditionally use Throwable.addSuppressed in Closer.
Browse files Browse the repository at this point in the history 
It was [added in API Level 19](https://developer.android.com/reference/java/lang/Throwable#addSuppressed(java.lang.Throwable)), so we can rely on it.

We could do further work to migrate our own code (e.g., `ByteSource`) off `Closer` entirely. I'll leave that for the future.

RELNOTES=n/a
PiperOrigin-RevId: 631431407
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed May 7, 2024
1 parent 61d3f25 commit 6342a23
Show file tree
Hide file tree
Showing 8 changed files with 136 additions and 522 deletions.
84 changes: 18 additions & 66 deletions android/guava-tests/test/com/google/common/io/ByteSourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.google.common.io.TestOption.READ_THROWS;
import static com.google.common.io.TestOption.SKIP_THROWS;
import static com.google.common.io.TestOption.WRITE_THROWS;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertThrows;

Expand All @@ -31,9 +32,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.hash.Hashing;
import com.google.common.io.Closer.LoggingSuppressor;
import com.google.common.primitives.UnsignedBytes;
import com.google.common.testing.TestLogHandler;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -378,61 +377,28 @@ public void testConcat_infiniteIterable() throws IOException {
ImmutableSet.of(BROKEN_CLOSE_SINK, BROKEN_OPEN_SINK, BROKEN_WRITE_SINK);

public void testCopyExceptions() {
if (Closer.create().suppressor instanceof LoggingSuppressor) {
// test that exceptions are logged

TestLogHandler logHandler = new TestLogHandler();
Closeables.logger.addHandler(logHandler);
try {
for (ByteSource in : BROKEN_SOURCES) {
runFailureTest(in, newNormalByteSink());
assertTrue(logHandler.getStoredLogRecords().isEmpty());

runFailureTest(in, BROKEN_CLOSE_SINK);
assertEquals((in == BROKEN_OPEN_SOURCE) ? 0 : 1, getAndResetRecords(logHandler));
}

for (ByteSink out : BROKEN_SINKS) {
runFailureTest(newNormalByteSource(), out);
assertTrue(logHandler.getStoredLogRecords().isEmpty());
// test that exceptions are suppressed

runFailureTest(BROKEN_CLOSE_SOURCE, out);
assertEquals(1, getAndResetRecords(logHandler));
}
for (ByteSource in : BROKEN_SOURCES) {
int suppressed = runSuppressionFailureTest(in, newNormalByteSink());
assertEquals(0, suppressed);

for (ByteSource in : BROKEN_SOURCES) {
for (ByteSink out : BROKEN_SINKS) {
runFailureTest(in, out);
assertTrue(getAndResetRecords(logHandler) <= 1);
}
}
} finally {
Closeables.logger.removeHandler(logHandler);
}
} else {
// test that exceptions are suppressed
suppressed = runSuppressionFailureTest(in, BROKEN_CLOSE_SINK);
assertEquals((in == BROKEN_OPEN_SOURCE) ? 0 : 1, suppressed);
}

for (ByteSource in : BROKEN_SOURCES) {
int suppressed = runSuppressionFailureTest(in, newNormalByteSink());
assertEquals(0, suppressed);
for (ByteSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(newNormalByteSource(), out);
assertEquals(0, suppressed);

suppressed = runSuppressionFailureTest(in, BROKEN_CLOSE_SINK);
assertEquals((in == BROKEN_OPEN_SOURCE) ? 0 : 1, suppressed);
}
suppressed = runSuppressionFailureTest(BROKEN_CLOSE_SOURCE, out);
assertEquals(1, suppressed);
}

for (ByteSource in : BROKEN_SOURCES) {
for (ByteSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(newNormalByteSource(), out);
assertEquals(0, suppressed);

suppressed = runSuppressionFailureTest(BROKEN_CLOSE_SOURCE, out);
assertEquals(1, suppressed);
}

for (ByteSource in : BROKEN_SOURCES) {
for (ByteSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(in, out);
assertTrue(suppressed <= 1);
}
int suppressed = runSuppressionFailureTest(in, out);
assertThat(suppressed).isAtMost(1);
}
}
}
Expand All @@ -441,20 +407,6 @@ public void testSlice_returnEmptySource() {
assertEquals(ByteSource.empty(), source.slice(0, 3).slice(4, 3));
}

private static int getAndResetRecords(TestLogHandler logHandler) {
int records = logHandler.getStoredLogRecords().size();
logHandler.clear();
return records;
}

private static void runFailureTest(ByteSource in, ByteSink out) {
try {
in.copyTo(out);
fail();
} catch (IOException expected) {
}
}

/**
* @return the number of exceptions that were suppressed on the expected thrown exception
*/
Expand All @@ -463,7 +415,7 @@ private static int runSuppressionFailureTest(ByteSource in, ByteSink out) {
in.copyTo(out);
fail();
} catch (IOException expected) {
return CloserTest.getSuppressed(expected).length;
return expected.getSuppressed().length;
}
throw new AssertionError(); // can't happen
}
Expand Down
86 changes: 19 additions & 67 deletions android/guava-tests/test/com/google/common/io/CharSourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@
import static com.google.common.io.TestOption.OPEN_THROWS;
import static com.google.common.io.TestOption.READ_THROWS;
import static com.google.common.io.TestOption.WRITE_THROWS;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.io.Closer.LoggingSuppressor;
import com.google.common.testing.TestLogHandler;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.Reader;
Expand Down Expand Up @@ -248,76 +247,29 @@ public void testConcat_infiniteIterable() throws IOException {
ImmutableSet.of(BROKEN_CLOSE_SINK, BROKEN_OPEN_SINK, BROKEN_WRITE_SINK);

public void testCopyExceptions() {
if (Closer.create().suppressor instanceof LoggingSuppressor) {
// test that exceptions are logged

TestLogHandler logHandler = new TestLogHandler();
Closeables.logger.addHandler(logHandler);
try {
for (CharSource in : BROKEN_SOURCES) {
runFailureTest(in, newNormalCharSink());
assertTrue(logHandler.getStoredLogRecords().isEmpty());

runFailureTest(in, BROKEN_CLOSE_SINK);
assertEquals((in == BROKEN_OPEN_SOURCE) ? 0 : 1, getAndResetRecords(logHandler));
}

for (CharSink out : BROKEN_SINKS) {
runFailureTest(newNormalCharSource(), out);
assertTrue(logHandler.getStoredLogRecords().isEmpty());

runFailureTest(BROKEN_CLOSE_SOURCE, out);
assertEquals(1, getAndResetRecords(logHandler));
}

for (CharSource in : BROKEN_SOURCES) {
for (CharSink out : BROKEN_SINKS) {
runFailureTest(in, out);
assertTrue(getAndResetRecords(logHandler) <= 1);
}
}
} finally {
Closeables.logger.removeHandler(logHandler);
}
} else {
// test that exceptions are suppressed

for (CharSource in : BROKEN_SOURCES) {
int suppressed = runSuppressionFailureTest(in, newNormalCharSink());
assertEquals(0, suppressed);
// test that exceptions are suppressed

suppressed = runSuppressionFailureTest(in, BROKEN_CLOSE_SINK);
assertEquals((in == BROKEN_OPEN_SOURCE) ? 0 : 1, suppressed);
}
for (CharSource in : BROKEN_SOURCES) {
int suppressed = runSuppressionFailureTest(in, newNormalCharSink());
assertEquals(0, suppressed);

for (CharSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(newNormalCharSource(), out);
assertEquals(0, suppressed);
suppressed = runSuppressionFailureTest(in, BROKEN_CLOSE_SINK);
assertEquals((in == BROKEN_OPEN_SOURCE) ? 0 : 1, suppressed);
}

suppressed = runSuppressionFailureTest(BROKEN_CLOSE_SOURCE, out);
assertEquals(1, suppressed);
}
for (CharSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(newNormalCharSource(), out);
assertEquals(0, suppressed);

for (CharSource in : BROKEN_SOURCES) {
for (CharSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(in, out);
assertTrue(suppressed <= 1);
}
}
suppressed = runSuppressionFailureTest(BROKEN_CLOSE_SOURCE, out);
assertEquals(1, suppressed);
}
}

private static int getAndResetRecords(TestLogHandler logHandler) {
int records = logHandler.getStoredLogRecords().size();
logHandler.clear();
return records;
}

private static void runFailureTest(CharSource in, CharSink out) {
try {
in.copyTo(out);
fail();
} catch (IOException expected) {
for (CharSource in : BROKEN_SOURCES) {
for (CharSink out : BROKEN_SINKS) {
int suppressed = runSuppressionFailureTest(in, out);
assertThat(suppressed).isAtMost(1);
}
}
}

Expand All @@ -329,7 +281,7 @@ private static int runSuppressionFailureTest(CharSource in, CharSink out) {
in.copyTo(out);
fail();
} catch (IOException expected) {
return CloserTest.getSuppressed(expected).length;
return expected.getSuppressed().length;
}
throw new AssertionError(); // can't happen
}
Expand Down
59 changes: 4 additions & 55 deletions android/guava-tests/test/com/google/common/io/CloserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.io.Closer.LoggingSuppressor;
import com.google.common.testing.TestLogHandler;
import java.io.Closeable;
import java.io.IOException;
import java.lang.reflect.Method;
import java.util.List;
import java.util.logging.LogRecord;
import junit.framework.TestCase;
import org.checkerframework.checker.nullness.qual.Nullable;

Expand All @@ -49,11 +45,6 @@ protected void setUp() throws Exception {
suppressor = new TestSuppressor();
}

@AndroidIncompatible // TODO(cpovirk): Look up Build.VERSION.SDK_INT reflectively.
public void testCreate() {
assertThat(Closer.create().suppressor).isInstanceOf(Closer.SuppressingSuppressor.class);
}

public void testNoExceptionsThrown() throws IOException {
Closer closer = new Closer(suppressor);

Expand Down Expand Up @@ -282,42 +273,8 @@ public void testErrors() throws IOException {
new Suppression(c1, c3Exception, c1Exception));
}

public static void testLoggingSuppressor() throws IOException {
TestLogHandler logHandler = new TestLogHandler();

Closeables.logger.addHandler(logHandler);
try {
Closer closer = new Closer(new Closer.LoggingSuppressor());

TestCloseable c1 = closer.register(TestCloseable.throwsOnClose(new IOException()));
TestCloseable c2 = closer.register(TestCloseable.throwsOnClose(new RuntimeException()));
try {
throw closer.rethrow(new IOException("thrown"), IOException.class);
} catch (IOException expected) {
}

assertTrue(logHandler.getStoredLogRecords().isEmpty());

closer.close();

assertEquals(2, logHandler.getStoredLogRecords().size());

LogRecord record = logHandler.getStoredLogRecords().get(0);
assertEquals("Suppressing exception thrown when closing " + c2, record.getMessage());

record = logHandler.getStoredLogRecords().get(1);
assertEquals("Suppressing exception thrown when closing " + c1, record.getMessage());
} finally {
Closeables.logger.removeHandler(logHandler);
}
}

public static void testSuppressingSuppressorIfPossible() throws IOException {
public static void testSuppressingSuppressor() throws IOException {
Closer closer = Closer.create();
// can't test the JDK7 suppressor when not running on JDK7
if (closer.suppressor instanceof LoggingSuppressor) {
return;
}

IOException thrownException = new IOException();
IOException c1Exception = new IOException();
Expand All @@ -331,7 +288,7 @@ public static void testSuppressingSuppressorIfPossible() throws IOException {
} catch (Throwable e) {
throw closer.rethrow(thrownException, IOException.class);
} finally {
assertThat(getSuppressed(thrownException)).isEmpty();
assertThat(thrownException.getSuppressed()).isEmpty();
closer.close();
}
} catch (IOException expected) {
Expand All @@ -341,7 +298,7 @@ public static void testSuppressingSuppressorIfPossible() throws IOException {
assertTrue(c1.isClosed());
assertTrue(c2.isClosed());

ImmutableSet<Throwable> suppressed = ImmutableSet.copyOf(getSuppressed(thrownException));
ImmutableSet<Throwable> suppressed = ImmutableSet.copyOf(thrownException.getSuppressed());
assertEquals(2, suppressed.size());

assertEquals(ImmutableSet.of(c1Exception, c2Exception), suppressed);
Expand All @@ -353,15 +310,6 @@ public void testNullCloseable() throws IOException {
closer.close();
}

static Throwable[] getSuppressed(Throwable throwable) {
try {
Method getSuppressed = Throwable.class.getDeclaredMethod("getSuppressed");
return (Throwable[]) getSuppressed.invoke(throwable);
} catch (Exception e) {
throw new AssertionError(e); // only called if running on JDK7
}
}

/**
* Asserts that an exception was thrown when trying to close each of the given throwables and that
* each such exception was suppressed because of the given thrown exception.
Expand All @@ -370,6 +318,7 @@ private void assertSuppressed(Suppression... expected) {
assertEquals(ImmutableList.copyOf(expected), suppressor.suppressions);
}

// TODO(cpovirk): Just use addSuppressed+getSuppressed now that we can rely on it.
/** Suppressor that records suppressions. */
private static class TestSuppressor implements Closer.Suppressor {

Expand Down

0 comments on commit 6342a23

Please sign in to comment.