Merge pull request #1401 from samhendley/onnextvalue

OnError while emitting onNext value: object.toString

Author: benjchristensen

rxjava-core/src/main/java/rx/exceptions/OnErrorThrowable.java

@@ -116,7 +116,7 @@ public static class OnNextValue extends RuntimeException {
          *         the item that the Observable was trying to emit at the time of the exception
          */
         public OnNextValue(Object value) {
-            super("OnError while emitting onNext value: " + value);
+            super("OnError while emitting onNext value: " + renderValue(value));
             this.value = value;
         }
 
@@ -129,5 +129,25 @@ public Object getValue() {
             return value;
         }
 
+        /**
+         * Render the object if it is a basic type. This avoids the library making potentially expensive
+         * or calls to toString() which may throw exceptions. See PR #1401 for details.
+         *
+         * @param value
+         *        the item that the Observable was trying to emit at the time of the exception
+         * @return a string version of the object if primitive, otherwise the classname of the object
+         */
+        private static String renderValue(Object value){
+            if(value == null){
+                return "null";
+            }
+            if(value.getClass().isPrimitive()){
+                return value.toString();
+            }
+            if(value instanceof String){
+                return (String)value;
+            }
+            return value.getClass().getSimpleName() + ".class";
+        }
     }
 }

rxjava-core/src/test/java/rx/exceptions/OnNextValueTest.java

@@ -0,0 +1,106 @@
+package rx.exceptions;
+
+import org.junit.Test;
+import rx.Observable;
+import rx.Observer;
+import rx.functions.Func1;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * ```java
+ * public OnNextValue(Object value) {
+ *    super("OnError while emitting onNext value: " + value);
+ *    this.value = value;
+ * }
+ * ```
+ * I know this is probably a helpful error message in some cases but this can be a really costly operation when an objects toString is an expensive call or contains alot of output. I don't think we should be printing this in any case but if so it should be on demand (overload of getMessage()) rather than eagerly.
+ * <p/>
+ * In my case it is causing a toString of a large context object that is normally only used for debugging purposes which makes the exception logs hard to use and they are rolling over the log files very quickly.
+ * <p/>
+ * There is an added danger that if there is a bug in the toString method it will cause inconsistent exception creation. If the object throws an exception while rendering a string it will actually end up not seeing the real exception.
+ */
+public final class OnNextValueTest {
+    private static class BadToString {
+
+        private final boolean throwDuringToString;
+
+        private BadToString(boolean throwDuringToString) {
+            this.throwDuringToString = throwDuringToString;
+        }
+
+        @Override
+        public String toString() {
+            if (throwDuringToString) {
+                throw new IllegalArgumentException("Error Making toString");
+            } else {
+                return "BadToString";
+            }
+        }
+    }
+
+    private static class BadToStringObserver implements Observer<BadToString> {
+        @Override
+        public void onCompleted() {
+            System.out.println("On Complete");
+            fail("OnComplete shouldn't be reached");
+        }
+
+        @Override
+        public void onError(Throwable e) {
+            String trace = stackTraceAsString(e);
+            System.out.println("On Error: " + trace);
+
+            assertTrue(trace, trace.contains("OnNextValue"));
+
+            assertTrue("No Cause on throwable" + e, e.getCause() != null);
+            assertTrue(e.getCause().getClass().getSimpleName() + " no OnNextValue",
+                    e.getCause() instanceof OnErrorThrowable.OnNextValue);
+        }
+
+        @Override
+        public void onNext(BadToString badToString) {
+            System.out.println("On Next");
+            fail("OnNext shouldn't be reached");
+
+        }
+    }
+
+    public static String stackTraceAsString(Throwable e) {
+        StringWriter sw = new StringWriter();
+        e.printStackTrace(new PrintWriter(sw));
+        return sw.toString();
+    }
+
+    @Test
+    public void addOnNextValueExceptionAdded() throws Exception {
+        Observer<BadToString> observer = new BadToStringObserver();
+
+        Observable.from(new BadToString(false))
+                .map(new Func1<BadToString, BadToString>() {
+                    @Override
+                    public BadToString call(BadToString badToString) {
+                        throw new IllegalArgumentException("Failure while handling");
+                    }
+                }).subscribe(observer);
+
+    }
+
+    @Test
+    public void addOnNextValueExceptionNotAddedWithBadString() throws Exception {
+        Observer<BadToString> observer = new BadToStringObserver();
+
+        Observable.from(new BadToString(true))
+                .map(new Func1<BadToString, BadToString>() {
+                    @Override
+                    public BadToString call(BadToString badToString) {
+                        throw new IllegalArgumentException("Failure while handling");
+                    }
+                }).subscribe(observer);
+
+    }
+}