Handle StackOverflowError from both pendingToString and appendUserObj…

…ect. Adds a bunch of tests to validate error handling in toString code - Verify that a future completing during the toString call results in a done formatted string - Verify that an exception thrown by pendingToString doesn't cause toString to fail - Verify that cycles don't cause toString to fail - Verify that deep chains of SetFuture don't cause toString to fail RELNOTES=Catch StackOverflowError in AbstractFuture.toString to prevent long chains of futures from failing toString calls. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=277570829
google · Oct 31, 2019 · a2e6acc · a2e6acc
1 parent 90989fb
commit a2e6acc
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 76 deletions.
diff --git a/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java b/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java
@@ -19,6 +19,7 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 
+import com.google.common.annotations.GwtIncompatible;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Range;
 import com.google.common.collect.Sets;
@@ -232,6 +233,19 @@ public String pendingToString() {
     }
   }
 
+  public void testToString_completesDuringToString() throws Exception {
+    AbstractFuture<Object> testFuture =
+        new AbstractFuture<Object>() {
+          @Override
+          public String pendingToString() {
+            // Complete ourselves during the toString calculation
+            this.set(true);
+            return "cause=[Because this test isn't done]";
+          }
+        };
+    assertThat(testFuture.toString()).matches("[^\\[]+\\[status=SUCCESS, result=\\[true\\]\\]");
+  }
+
   /**
    * This test attempts to cause a future to wait for longer than it was requested to from a timed
    * get() call. As measurements of time are prone to flakiness, it tries to assert based on ranges
@@ -783,6 +797,23 @@ public void testSetFuture_stackOverflow() {
     assertTrue(orig.isDone());
   }
 
+  // Verify that StackOverflowError in a long chain of SetFuture doesn't cause the entire toString
+  // call to fail
+  @GwtIncompatible
+  @AndroidIncompatible
+  public void testSetFutureToString_stackOverflow() {
+    SettableFuture<String> orig = SettableFuture.create();
+    SettableFuture<String> prev = orig;
+    for (int i = 0; i < 100000; i++) {
+      SettableFuture<String> curr = SettableFuture.create();
+      prev.setFuture(curr);
+      prev = curr;
+    }
+    // orig represents the 'outermost' future
+    assertThat(orig.toString())
+        .contains("Exception thrown from implementation: class java.lang.StackOverflowError");
+  }
+
   public void testSetFuture_misbehavingFutureThrows() throws Exception {
     SettableFuture<String> future = SettableFuture.create();
     ListenableFuture<String> badFuture =
@@ -920,11 +951,8 @@ public String toString() {
             return orig.toString();
           }
         });
-    try {
-      orig.toString();
-      fail();
-    } catch (StackOverflowError expected) {
-    }
+    assertThat(orig.toString())
+        .contains("Exception thrown from implementation: class java.lang.StackOverflowError");
   }
 
   // Regression test for a case where we would fail to execute listeners immediately on done futures

diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java
@@ -1088,43 +1088,46 @@ protected String pendingToString() {
   }
 
   private void addPendingString(StringBuilder builder) {
-    // Be careful not to append to the builder with anything that might be invalidated
+    // Capture current builder length so it can be truncated if this future ends up completing while
+    // the toString is being calculated
+    int truncateLength = builder.length();
 
-    String pendingDescription;
-    try {
-      pendingDescription = Strings.emptyToNull(pendingToString());
-    } catch (RuntimeException e) {
-      // Don't call getMessage or toString() on the exception, in case the exception thrown by the
-      // subclass is implemented with bugs similar to the subclass.
-      pendingDescription = "Exception thrown from implementation: " + e.getClass();
-    }
+    builder.append("PENDING");
 
-    String setFutureString = null;
     Object localValue = value;
     if (localValue instanceof SetFuture) {
-      setFutureString = userObjectToString(((SetFuture) localValue).future);
+      builder.append(", setFuture=[");
+      appendUserObject(builder, ((SetFuture) localValue).future);
+      builder.append("]");
+    } else {
+      String pendingDescription;
+      try {
+        pendingDescription = Strings.emptyToNull(pendingToString());
+      } catch (RuntimeException | StackOverflowError e) {
+        // Don't call getMessage or toString() on the exception, in case the exception thrown by the
+        // subclass is implemented with bugs similar to the subclass.
+        pendingDescription = "Exception thrown from implementation: " + e.getClass();
+      }
+      if (pendingDescription != null) {
+        builder.append(", info=[").append(pendingDescription).append("]");
+      }
     }
 
-    // The future may complete before we reach this point, so we check once more to see if the
+    // The future may complete while calculating the toString, so we check once more to see if the
     // future is done
     if (isDone()) {
+      // Truncate anything that was appended before realizing this future is done
+      builder.delete(truncateLength, builder.length());
       addDoneString(builder);
-      return;
-    }
-
-    builder.append("PENDING");
-    if (pendingDescription != null) {
-      builder.append(", info=[").append(pendingDescription).append("]");
-    }
-    if (setFutureString != null) {
-      builder.append(", setFuture=[").append(setFutureString).append("]");
     }
   }
 
   private void addDoneString(StringBuilder builder) {
     try {
       V value = getUninterruptibly(this);
-      builder.append("SUCCESS, result=[").append(userObjectToString(value)).append("]");
+      builder.append("SUCCESS, result=[");
+      appendUserObject(builder, value);
+      builder.append("]");
     } catch (ExecutionException e) {
       builder.append("FAILURE, cause=[").append(e.getCause()).append("]");
     } catch (CancellationException e) {
@@ -1135,20 +1138,21 @@ private void addDoneString(StringBuilder builder) {
   }
 
   /** Helper for printing user supplied objects into our toString method. */
-  private String userObjectToString(Object o) {
-    // This is some basic recursion detection for when people create cycles via set/setFuture
-    // This is however only partial protection though since it only detects self loops.  We could
-    // detect arbitrary cycles using a thread local or possibly by catching StackOverflowExceptions
-    // but this should be a good enough solution (it is also what jdk collections do in these cases)
-    if (o == this) {
-      return "this future";
-    }
+  private void appendUserObject(StringBuilder builder, Object o) {
+    // This is some basic recursion detection for when people create cycles via set/setFuture or
+    // when deep chains of futures exist resulting in a StackOverflowException. We could detect
+    // arbitrary cycles using a thread local but this should be a good enough solution (it is also
+    // what jdk collections do in these cases)
     try {
-      return String.valueOf(o);
-    } catch (RuntimeException e) {
+      if (o == this) {
+        builder.append("this future");
+      } else {
+        builder.append(o);
+      }
+    } catch (RuntimeException | StackOverflowError e) {
       // Don't call getMessage or toString() on the exception, in case the exception thrown by the
       // user object is implemented with bugs similar to the user object.
-      return "Exception thrown from implementation: " + e.getClass();
+      builder.append("Exception thrown from implementation: ").append(e.getClass());
     }
   }
 

diff --git a/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java b/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java
@@ -19,6 +19,7 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 
+import com.google.common.annotations.GwtIncompatible;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Range;
 import com.google.common.collect.Sets;
@@ -232,6 +233,19 @@ public String pendingToString() {
     }
   }
 
+  public void testToString_completesDuringToString() throws Exception {
+    AbstractFuture<Object> testFuture =
+        new AbstractFuture<Object>() {
+          @Override
+          public String pendingToString() {
+            // Complete ourselves during the toString calculation
+            this.set(true);
+            return "cause=[Because this test isn't done]";
+          }
+        };
+    assertThat(testFuture.toString()).matches("[^\\[]+\\[status=SUCCESS, result=\\[true\\]\\]");
+  }
+
   /**
    * This test attempts to cause a future to wait for longer than it was requested to from a timed
    * get() call. As measurements of time are prone to flakiness, it tries to assert based on ranges
@@ -783,6 +797,23 @@ public void testSetFuture_stackOverflow() {
     assertTrue(orig.isDone());
   }
 
+  // Verify that StackOverflowError in a long chain of SetFuture doesn't cause the entire toString
+  // call to fail
+  @GwtIncompatible
+  @AndroidIncompatible
+  public void testSetFutureToString_stackOverflow() {
+    SettableFuture<String> orig = SettableFuture.create();
+    SettableFuture<String> prev = orig;
+    for (int i = 0; i < 100000; i++) {
+      SettableFuture<String> curr = SettableFuture.create();
+      prev.setFuture(curr);
+      prev = curr;
+    }
+    // orig represents the 'outermost' future
+    assertThat(orig.toString())
+        .contains("Exception thrown from implementation: class java.lang.StackOverflowError");
+  }
+
   public void testSetFuture_misbehavingFutureThrows() throws Exception {
     SettableFuture<String> future = SettableFuture.create();
     ListenableFuture<String> badFuture =
@@ -920,11 +951,8 @@ public String toString() {
             return orig.toString();
           }
         });
-    try {
-      orig.toString();
-      fail();
-    } catch (StackOverflowError expected) {
-    }
+    assertThat(orig.toString())
+        .contains("Exception thrown from implementation: class java.lang.StackOverflowError");
   }
 
   // Regression test for a case where we would fail to execute listeners immediately on done futures

diff --git a/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/guava/src/com/google/common/util/concurrent/AbstractFuture.java
@@ -1087,43 +1087,46 @@ public String toString() {
   }
 
   private void addPendingString(StringBuilder builder) {
-    // Be careful not to append to the builder with anything that might be invalidated
+    // Capture current builder length so it can be truncated if this future ends up completing while
+    // the toString is being calculated
+    int truncateLength = builder.length();
 
-    String pendingDescription;
-    try {
-      pendingDescription = Strings.emptyToNull(pendingToString());
-    } catch (RuntimeException e) {
-      // Don't call getMessage or toString() on the exception, in case the exception thrown by the
-      // subclass is implemented with bugs similar to the subclass.
-      pendingDescription = "Exception thrown from implementation: " + e.getClass();
-    }
+    builder.append("PENDING");
 
-    String setFutureString = null;
     Object localValue = value;
     if (localValue instanceof SetFuture) {
-      setFutureString = userObjectToString(((SetFuture) localValue).future);
+      builder.append(", setFuture=[");
+      appendUserObject(builder, ((SetFuture) localValue).future);
+      builder.append("]");
+    } else {
+      String pendingDescription;
+      try {
+        pendingDescription = Strings.emptyToNull(pendingToString());
+      } catch (RuntimeException | StackOverflowError e) {
+        // Don't call getMessage or toString() on the exception, in case the exception thrown by the
+        // subclass is implemented with bugs similar to the subclass.
+        pendingDescription = "Exception thrown from implementation: " + e.getClass();
+      }
+      if (pendingDescription != null) {
+        builder.append(", info=[").append(pendingDescription).append("]");
+      }
     }
 
-    // The future may complete before we reach this point, so we check once more to see if the
+    // The future may complete while calculating the toString, so we check once more to see if the
     // future is done
     if (isDone()) {
+      // Truncate anything that was appended before realizing this future is done
+      builder.delete(truncateLength, builder.length());
       addDoneString(builder);
-      return;
-    }
-
-    builder.append("PENDING");
-    if (pendingDescription != null) {
-      builder.append(", info=[").append(pendingDescription).append("]");
-    }
-    if (setFutureString != null) {
-      builder.append(", setFuture=[").append(setFutureString).append("]");
     }
   }
 
   private void addDoneString(StringBuilder builder) {
     try {
       V value = getUninterruptibly(this);
-      builder.append("SUCCESS, result=[").append(userObjectToString(value)).append("]");
+      builder.append("SUCCESS, result=[");
+      appendUserObject(builder, value);
+      builder.append("]");
     } catch (ExecutionException e) {
       builder.append("FAILURE, cause=[").append(e.getCause()).append("]");
     } catch (CancellationException e) {
@@ -1134,20 +1137,21 @@ private void addDoneString(StringBuilder builder) {
   }
 
   /** Helper for printing user supplied objects into our toString method. */
-  private String userObjectToString(Object o) {
-    // This is some basic recursion detection for when people create cycles via set/setFuture
-    // This is however only partial protection though since it only detects self loops.  We could
-    // detect arbitrary cycles using a thread local or possibly by catching StackOverflowExceptions
-    // but this should be a good enough solution (it is also what jdk collections do in these cases)
-    if (o == this) {
-      return "this future";
-    }
+  private void appendUserObject(StringBuilder builder, Object o) {
+    // This is some basic recursion detection for when people create cycles via set/setFuture or
+    // when deep chains of futures exist resulting in a StackOverflowException. We could detect
+    // arbitrary cycles using a thread local but this should be a good enough solution (it is also
+    // what jdk collections do in these cases)
     try {
-      return String.valueOf(o);
-    } catch (RuntimeException e) {
+      if (o == this) {
+        builder.append("this future");
+      } else {
+        builder.append(o);
+      }
+    } catch (RuntimeException | StackOverflowError e) {
       // Don't call getMessage or toString() on the exception, in case the exception thrown by the
       // user object is implemented with bugs similar to the user object.
-      return "Exception thrown from implementation: " + e.getClass();
+      builder.append("Exception thrown from implementation: ").append(e.getClass());
     }
   }