From f4ae92aa624923e9a632fb677730a2d6e04595e5 Mon Sep 17 00:00:00 2001 From: Matt Jacobs Date: Wed, 25 Jun 2014 15:49:03 -0700 Subject: [PATCH 1/3] CompositeException does not mutate nested exceptions by attaching stacks, but instead aggregates them at print-time. Since nothing is being mutated, there's no chance of accidentally creating a cycle in the Exception chain. --- .../rx/exceptions/CompositeException.java | 159 ++++++++++++------ .../rx/exceptions/CompositeExceptionTest.java | 65 ++++--- 2 files changed, 147 insertions(+), 77 deletions(-) diff --git a/rxjava-core/src/main/java/rx/exceptions/CompositeException.java b/rxjava-core/src/main/java/rx/exceptions/CompositeException.java index 5bf6bffadd..5e4344155d 100644 --- a/rxjava-core/src/main/java/rx/exceptions/CompositeException.java +++ b/rxjava-core/src/main/java/rx/exceptions/CompositeException.java @@ -15,17 +15,22 @@ */ package rx.exceptions; +import java.io.PrintStream; +import java.io.PrintWriter; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; /** * Exception that is a composite of 1 or more other exceptions. - *

- * Use getMessage() to retrieve a concatenation of the composite exceptions. + * A CompositeException does not modify the structure of any exception it wraps, but at print-time + * iterates through the list of contained Throwables to print them all. + * + * Its invariant is to contains an immutable, ordered (by insertion order), unique list of non-composite exceptions. + * This list may be queried by {@code #getExceptions()} */ public final class CompositeException extends RuntimeException { @@ -33,20 +38,21 @@ public final class CompositeException extends RuntimeException { private final List exceptions; private final String message; - private final Throwable cause; public CompositeException(String messagePrefix, Collection errors) { + Set deDupedExceptions = new LinkedHashSet(); List _exceptions = new ArrayList(); - CompositeExceptionCausalChain _cause = new CompositeExceptionCausalChain(); - int count = 0; - for (Throwable e : errors) { - count++; - attachCallingThreadStack(_cause, e); - _exceptions.add(e); + for (Throwable ex: errors) { + if (ex instanceof CompositeException) { + deDupedExceptions.addAll(((CompositeException) ex).getExceptions()); + } else { + deDupedExceptions.add(ex); + } } + + _exceptions.addAll(deDupedExceptions); this.exceptions = Collections.unmodifiableList(_exceptions); - this.message = count + " exceptions occurred. See them in causal chain below."; - this.cause = _cause; + this.message = exceptions.size() + " exceptions occurred. See them in causal chain below."; } public CompositeException(Collection errors) { @@ -70,57 +76,106 @@ public String getMessage() { @Override public synchronized Throwable getCause() { - return cause; + return null; } - @SuppressWarnings("unused") - // useful when debugging but don't want to make part of publicly supported API - private static String getStackTraceAsString(StackTraceElement[] stack) { - StringBuilder s = new StringBuilder(); - boolean firstLine = true; - for (StackTraceElement e : stack) { - if (e.toString().startsWith("java.lang.Thread.getStackTrace")) { - // we'll ignore this one - continue; - } - if (!firstLine) { - s.append("\n\t"); - } - s.append(e.toString()); - firstLine = false; - } - return s.toString(); + /** + * All of the following printStackTrace functionality is derived from JDK Throwable printStackTrace. + * In particular, the PrintStreamOrWriter abstraction is copied wholesale. + * + * Changes from the official JDK implementation: + * * No infinite loop detection + * * Smaller critical section holding printStream lock + * * Explicit knowledge about exceptions List that this loops through + */ + @Override + public void printStackTrace() { + printStackTrace(System.err); } - /* package-private */ static void attachCallingThreadStack(Throwable e, Throwable cause) { - Set seenCauses = new HashSet(); + @Override + public void printStackTrace(PrintStream s) { + printStackTrace(new WrappedPrintStream(s)); + } - while (e.getCause() != null) { - e = e.getCause(); - if (seenCauses.contains(e.getCause())) { - break; - } else { - seenCauses.add(e.getCause()); - } + @Override + public void printStackTrace(PrintWriter s) { + printStackTrace(new WrappedPrintWriter(s)); + } + + /** + * Special handling for printing out a CompositeException + * Loop through all inner exceptions and print them out + * @param s stream to print to + */ + private void printStackTrace(PrintStreamOrWriter s) { + StringBuilder bldr = new StringBuilder(); + bldr.append(this).append("\n"); + for (StackTraceElement myStackElement: getStackTrace()) { + bldr.append("\tat ").append(myStackElement).append("\n"); + } + int i = 1; + for (Throwable ex: exceptions) { + bldr.append(" ComposedException ").append(i).append(" :").append("\n"); + appendStackTrace(bldr, ex, "\t"); + i++; } - // we now have 'e' as the last in the chain - try { - e.initCause(cause); - } catch (Throwable t) { - // ignore - // the javadocs say that some Throwables (depending on how they're made) will never - // let me call initCause without blowing up even if it returns null + synchronized (s.lock()) { + s.println(bldr.toString()); } } - /* package-private */ final static class CompositeExceptionCausalChain extends RuntimeException { - private static final long serialVersionUID = 3875212506787802066L; - /* package-private */ static String MESSAGE = "Chain of Causes for CompositeException In Order Received =>"; + private void appendStackTrace(StringBuilder bldr, Throwable ex, String prefix) { + bldr.append(prefix).append(ex).append("\n"); + for (StackTraceElement stackElement: ex.getStackTrace()) { + bldr.append("\t\tat ").append(stackElement).append("\n"); + } + if (ex.getCause() != null) { + bldr.append("\tCaused by: "); + appendStackTrace(bldr, ex.getCause(), ""); + } + } + + private abstract static class PrintStreamOrWriter { + /** Returns the object to be locked when using this StreamOrWriter */ + abstract Object lock(); + + /** Prints the specified string as a line on this StreamOrWriter */ + abstract void println(Object o); + } + + /** + * Same abstraction and implementation as in JDK to allow PrintStream and PrintWriter to share implementation + */ + private static class WrappedPrintStream extends PrintStreamOrWriter { + private final PrintStream printStream; + + WrappedPrintStream(PrintStream printStream) { + this.printStream = printStream; + } + + Object lock() { + return printStream; + } - @Override - public String getMessage() { - return MESSAGE; + void println(Object o) { + printStream.println(o); } } + private static class WrappedPrintWriter extends PrintStreamOrWriter { + private final PrintWriter printWriter; + + WrappedPrintWriter(PrintWriter printWriter) { + this.printWriter = printWriter; + } + + Object lock() { + return printWriter; + } + + void println(Object o) { + printWriter.println(o); + } + } } diff --git a/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java b/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java index 5cc1e661b8..81b6a8085e 100644 --- a/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java +++ b/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java @@ -16,7 +16,11 @@ package rx.exceptions; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.io.StringWriter; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -30,7 +34,6 @@ public class CompositeExceptionTest { private final Throwable ex3 = new Throwable("Ex3", ex2); public CompositeExceptionTest() { - ex1.initCause(ex2); } private CompositeException getNewCompositeExceptionWithEx123() { @@ -50,46 +53,58 @@ public void testMultipleWithSameCause() { CompositeException ce = new CompositeException("3 failures with same root cause", Arrays.asList(e1, e2, e3)); assertEquals(3, ce.getExceptions().size()); - + assertNoCircularReferences(ce); } @Test(timeout = 1000) - public void testAttachCallingThreadStackParentThenChild() { - CompositeException.attachCallingThreadStack(ex1, ex2); - assertEquals("Ex2", ex1.getCause().getMessage()); + public void testCompositeExceptionFromParentThenChild() { + CompositeException cex = new CompositeException(Arrays.asList(ex1, ex2)); + assertNoCircularReferences(cex); + assertEquals(2, cex.getExceptions().size()); } @Test(timeout = 1000) - public void testAttachCallingThreadStackChildThenParent() { - CompositeException.attachCallingThreadStack(ex2, ex1); - assertEquals("Ex1", ex2.getCause().getMessage()); + public void testCompositeExceptionFromChildThenParent() { + CompositeException cex = new CompositeException(Arrays.asList(ex2, ex1)); + assertNoCircularReferences(cex); + assertEquals(2, cex.getExceptions().size()); } @Test(timeout = 1000) - public void testAttachCallingThreadStackAddComposite() { - CompositeException.attachCallingThreadStack(ex1, getNewCompositeExceptionWithEx123()); - assertEquals("Ex2", ex1.getCause().getMessage()); + public void testCompositeExceptionFromChildAndComposite() { + CompositeException cex = new CompositeException(Arrays.asList(ex1, getNewCompositeExceptionWithEx123())); + assertNoCircularReferences(cex); + assertEquals(3, cex.getExceptions().size()); } @Test(timeout = 1000) - public void testAttachCallingThreadStackAddToComposite() { - CompositeException compositeEx = getNewCompositeExceptionWithEx123(); - CompositeException.attachCallingThreadStack(compositeEx, ex1); - assertEquals(CompositeException.CompositeExceptionCausalChain.MESSAGE, compositeEx.getCause().getMessage()); + public void testCompositeExceptionFromCompositeAndChild() { + CompositeException cex = new CompositeException(Arrays.asList(getNewCompositeExceptionWithEx123(), ex1)); + assertNoCircularReferences(cex); + assertEquals(3, cex.getExceptions().size()); } @Test(timeout = 1000) - public void testAttachCallingThreadStackAddCompositeToItself() { - CompositeException compositeEx = getNewCompositeExceptionWithEx123(); - CompositeException.attachCallingThreadStack(compositeEx, compositeEx); - assertEquals(CompositeException.CompositeExceptionCausalChain.MESSAGE, compositeEx.getCause().getMessage()); + public void testCompositeExceptionFromTwoDuplicateComposites() { + List exs = new ArrayList(); + exs.add(getNewCompositeExceptionWithEx123()); + exs.add(getNewCompositeExceptionWithEx123()); + CompositeException cex = new CompositeException(exs); + assertNoCircularReferences(cex); + assertEquals(3, cex.getExceptions().size()); } - @Test(timeout = 1000) - public void testAttachCallingThreadStackAddExceptionsToEachOther() { - CompositeException.attachCallingThreadStack(ex1, ex2); - CompositeException.attachCallingThreadStack(ex2, ex1); - assertEquals("Ex2", ex1.getCause().getMessage()); - assertEquals("Ex1", ex2.getCause().getMessage()); + /** + * This hijacks the Throwable.printStackTrace() output and puts it in a string, where we can look for + * "CIRCULAR REFERENCE" (a String added by Throwable.printEnclosedStackTrace) + */ + private static void assertNoCircularReferences(Throwable ex) { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + PrintStream printStream = new PrintStream(baos); + StringWriter writer = new StringWriter(); + + ex.printStackTrace(); + //ex.printStackTrace(printStream); + //assertFalse(baos.toString().contains("CIRCULAR REFERENCE")); } } \ No newline at end of file From 3b95e2fb1f2b1e7db3330acae8725e3aad9539da Mon Sep 17 00:00:00 2001 From: Matt Jacobs Date: Wed, 25 Jun 2014 15:53:03 -0700 Subject: [PATCH 2/3] Removed unused import --- .../src/test/java/rx/exceptions/CompositeExceptionTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java b/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java index 81b6a8085e..1e3b348be1 100644 --- a/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java +++ b/rxjava-core/src/test/java/rx/exceptions/CompositeExceptionTest.java @@ -16,7 +16,6 @@ package rx.exceptions; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import java.io.ByteArrayOutputStream; import java.io.PrintStream; From 8e27cdcf85eb07e2417a37947c8708980b83845c Mon Sep 17 00:00:00 2001 From: Matt Jacobs Date: Wed, 25 Jun 2014 16:19:52 -0700 Subject: [PATCH 3/3] Fixed up SafeObserverTest w.r.t. CompositeException changes --- .../java/rx/observers/SafeObserverTest.java | 67 ++++++++++--------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/rxjava-core/src/test/java/rx/observers/SafeObserverTest.java b/rxjava-core/src/test/java/rx/observers/SafeObserverTest.java index f4316d4698..584c6ee117 100644 --- a/rxjava-core/src/test/java/rx/observers/SafeObserverTest.java +++ b/rxjava-core/src/test/java/rx/observers/SafeObserverTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.util.List; import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; @@ -109,15 +110,16 @@ public void onErrorFailureSafe() { Throwable e2 = e.getCause(); assertTrue(e2 instanceof CompositeException); - assertEquals("Chain of Causes for CompositeException In Order Received =>", e2.getCause().getMessage()); + List innerExceptions = ((CompositeException) e2).getExceptions(); + assertEquals(2, innerExceptions.size()); - Throwable e3 = e2.getCause(); - assertTrue(e3.getCause() instanceof SafeObserverTestException); - assertEquals("error!", e3.getCause().getMessage()); + Throwable e3 = innerExceptions.get(0); + assertTrue(e3 instanceof SafeObserverTestException); + assertEquals("error!", e3.getMessage()); - Throwable e4 = e3.getCause(); - assertTrue(e4.getCause() instanceof SafeObserverTestException); - assertEquals("onErrorFail", e4.getCause().getMessage()); + Throwable e4 = innerExceptions.get(1); + assertTrue(e4 instanceof SafeObserverTestException); + assertEquals("onErrorFail", e4.getMessage()); } } @@ -157,15 +159,16 @@ public void onNextOnErrorFailureSafe() { Throwable e2 = e.getCause(); assertTrue(e2 instanceof CompositeException); - assertEquals("Chain of Causes for CompositeException In Order Received =>", e2.getCause().getMessage()); + List innerExceptions = ((CompositeException) e2).getExceptions(); + assertEquals(2, innerExceptions.size()); - Throwable e3 = e2.getCause(); - assertTrue(e3.getCause() instanceof SafeObserverTestException); - assertEquals("onNextFail", e3.getCause().getMessage()); + Throwable e3 = innerExceptions.get(0); + assertTrue(e3 instanceof SafeObserverTestException); + assertEquals("onNextFail", e3.getMessage()); - Throwable e4 = e3.getCause(); - assertTrue(e4.getCause() instanceof SafeObserverTestException); - assertEquals("onErrorFail", e4.getCause().getMessage()); + Throwable e4 = innerExceptions.get(1); + assertTrue(e4 instanceof SafeObserverTestException); + assertEquals("onErrorFail", e4.getMessage()); } } @@ -251,19 +254,20 @@ public void call() { Throwable e2 = e.getCause(); assertTrue(e2 instanceof CompositeException); - assertEquals("Chain of Causes for CompositeException In Order Received =>", e2.getCause().getMessage()); + List innerExceptions = ((CompositeException) e2).getExceptions(); + assertEquals(3, innerExceptions.size()); - Throwable e3 = e2.getCause(); - assertTrue(e3.getCause() instanceof SafeObserverTestException); - assertEquals("onError failure", e3.getCause().getMessage()); + Throwable e3 = innerExceptions.get(0); + assertTrue(e3 instanceof SafeObserverTestException); + assertEquals("onError failure", e3.getMessage()); - Throwable e4 = e3.getCause(); - assertTrue(e4.getCause() instanceof SafeObserverTestException); - assertEquals("onErrorFail", e4.getCause().getMessage()); + Throwable e4 = innerExceptions.get(1); + assertTrue(e4 instanceof SafeObserverTestException); + assertEquals("onErrorFail", e4.getMessage()); - Throwable e5 = e4.getCause(); - assertTrue(e5.getCause() instanceof SafeObserverTestException); - assertEquals("failure from unsubscribe", e5.getCause().getMessage()); + Throwable e5 = innerExceptions.get(2); + assertTrue(e5 instanceof SafeObserverTestException); + assertEquals("failure from unsubscribe", e5.getMessage()); } } @@ -292,15 +296,16 @@ public void call() { Throwable e2 = e.getCause(); assertTrue(e2 instanceof CompositeException); - assertEquals("Chain of Causes for CompositeException In Order Received =>", e2.getCause().getMessage()); + List innerExceptions = ((CompositeException) e2).getExceptions(); + assertEquals(2, innerExceptions.size()); - Throwable e3 = e2.getCause(); - assertTrue(e3.getCause() instanceof SafeObserverTestException); - assertEquals("error!", e3.getCause().getMessage()); + Throwable e3 = innerExceptions.get(0); + assertTrue(e3 instanceof SafeObserverTestException); + assertEquals("error!", e3.getMessage()); - Throwable e4 = e3.getCause(); - assertTrue(e4.getCause() instanceof SafeObserverTestException); - assertEquals("failure from unsubscribe", e4.getCause().getMessage()); + Throwable e4 = innerExceptions.get(1); + assertTrue(e4 instanceof SafeObserverTestException); + assertEquals("failure from unsubscribe", e4.getMessage()); } }