Merge pull request #1388 from mattrjacobs/eliminate-circular-exception-creation

benjchristensen · benjchristensen · commit b40e89f12465 · 2014-06-25T16:29:26.000-07:00
CompositeException stops mutating nested Exceptions
diff --git a/rxjava-core/src/main/java/rx/exceptions/CompositeException.java b/rxjava-core/src/main/java/rx/exceptions/CompositeException.java
@@ -15,38 +15,44 @@
  */
 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.
- * <p>
- * Use <code>getMessage()</code> 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 {
 
     private static final long serialVersionUID = 3026362227162912146L;
 
     private final List<Throwable> exceptions;
     private final String message;
-    private final Throwable cause;
 
     public CompositeException(String messagePrefix, Collection<Throwable> errors) {
+        Set<Throwable> deDupedExceptions = new LinkedHashSet<Throwable>();
         List<Throwable> _exceptions = new ArrayList<Throwable>();
-        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<Throwable> 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<Throwable> seenCauses = new HashSet<Throwable>();
+    @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
@@ -17,6 +17,9 @@
 
 import static org.junit.Assert.assertEquals;
 
+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 +33,6 @@ public class CompositeExceptionTest {
     private final Throwable ex3 = new Throwable("Ex3", ex2);
 
     public CompositeExceptionTest() {
-        ex1.initCause(ex2);
     }
 
     private CompositeException getNewCompositeExceptionWithEx123() {
@@ -50,46 +52,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<Throwable> exs = new ArrayList<Throwable>();
+        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"));
     }
 }
diff --git a/rxjava-core/src/test/java/rx/observers/SafeObserverTest.java b/rxjava-core/src/test/java/rx/observers/SafeObserverTest.java
