Don't catch fatal errors. (#2566)

* Don't catch fatal errors.

* Fix more errors slipping in.

* Add permalink.

* Call errors fatal instead of unrecoverable.

* Rename caught exception variable.

* Don't catch AssertionErrors in tests.

retrofit/src/main/java/retrofit2/OkHttpCall.java

@@ -27,6 +27,7 @@ import okio.ForwardingSource;
 import okio.Okio;
 
 import static retrofit2.Utils.checkNotNull;
+import static retrofit2.Utils.throwIfFatal;
 
 final class OkHttpCall<T> implements Call<T> {
   private final ServiceMethod<T, ?> serviceMethod;
@@ -36,8 +37,8 @@ final class OkHttpCall<T> implements Call<T> {
 
   @GuardedBy("this")
   private @Nullable okhttp3.Call rawCall;
-  @GuardedBy("this")
-  private @Nullable Throwable creationFailure; // Either a RuntimeException, Error, or IOException.
+  @GuardedBy("this") // Either a RuntimeException, non-fatal Error, or IOException.
+  private @Nullable Throwable creationFailure;
   @GuardedBy("this")
   private boolean executed;
 
@@ -68,6 +69,7 @@ final class OkHttpCall<T> implements Call<T> {
     try {
       return (rawCall = createRawCall()).request();
     } catch (RuntimeException | Error e) {
+      throwIfFatal(e); // Do not assign a fatal error to creationFailure.
       creationFailure = e;
       throw e;
     } catch (IOException e) {
@@ -92,6 +94,7 @@ final class OkHttpCall<T> implements Call<T> {
         try {
           call = rawCall = createRawCall();
         } catch (Throwable t) {
+          throwIfFatal(t);
           failure = creationFailure = t;
         }
       }
@@ -167,6 +170,7 @@ final class OkHttpCall<T> implements Call<T> {
         try {
           call = rawCall = createRawCall();
         } catch (IOException | RuntimeException | Error e) {
+          throwIfFatal(e); //  Do not assign a fatal error to creationFailure.
           creationFailure = e;
           throw e;
         }

retrofit/src/main/java/retrofit2/Utils.java

@@ -494,4 +494,16 @@ final class Utils {
       return "? extends " + typeToString(upperBound);
     }
   }
+
+  // https://github.com/ReactiveX/RxJava/blob/6a44e5d0543a48f1c378dc833a155f3f71333bc2/
+  // src/main/java/io/reactivex/exceptions/Exceptions.java#L66
+  static void throwIfFatal(Throwable t) {
+    if (t instanceof VirtualMachineError) {
+      throw (VirtualMachineError) t;
+    } else if (t instanceof ThreadDeath) {
+      throw (ThreadDeath) t;
+    } else if (t instanceof LinkageError) {
+      throw (LinkageError) t;
+    }
+  }
 }

retrofit/src/test/java/retrofit2/CallTest.java

@@ -19,6 +19,7 @@ import java.io.IOException;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Type;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import okhttp3.Interceptor;
@@ -768,7 +769,7 @@ public final class CallTest {
     assertThat(writeCount.get()).isEqualTo(1);
   }
 
-  @Test public void requestThrowingErrorBeforeExecuteFailsExecute() throws IOException {
+  @Test public void requestThrowingNonFatalErrorBeforeExecuteFailsExecute() throws IOException {
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl(server.url("/"))
         .addConverterFactory(new ToStringConverterFactory())
@@ -781,7 +782,7 @@ public final class CallTest {
     Object a = new Object() {
       @Override public String toString() {
         writeCount.incrementAndGet();
-        throw new OutOfMemoryError("Broken!");
+        throw new Error("Broken!");
       }
     };
     Call<String> call = service.postRequestBody(a);
@@ -789,7 +790,7 @@ public final class CallTest {
     try {
       call.request();
       fail();
-    } catch (OutOfMemoryError e) {
+    } catch (Error e) {
       assertThat(e).hasMessage("Broken!");
     }
     assertThat(writeCount.get()).isEqualTo(1);
@@ -797,7 +798,7 @@ public final class CallTest {
     try {
       call.execute();
       fail();
-    } catch (OutOfMemoryError e) {
+    } catch (Error e) {
       assertThat(e).hasMessage("Broken!");
     }
     assertThat(writeCount.get()).isEqualTo(1);
@@ -863,7 +864,7 @@ public final class CallTest {
     assertThat(writeCount.get()).isEqualTo(1);
   }
 
-  @Test public void requestAfterExecuteThrowingAlsoThrowsForErrors() throws IOException {
+  @Test public void requestAfterExecuteThrowingAlsoThrowsForNonFatalErrors() throws IOException {
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl(server.url("/"))
         .addConverterFactory(new ToStringConverterFactory())
@@ -876,7 +877,7 @@ public final class CallTest {
     Object a = new Object() {
       @Override public String toString() {
         writeCount.incrementAndGet();
-        throw new OutOfMemoryError("Broken!");
+        throw new Error("Broken!");
       }
     };
     Call<String> call = service.postRequestBody(a);
@@ -884,7 +885,7 @@ public final class CallTest {
     try {
       call.execute();
       fail();
-    } catch (OutOfMemoryError e) {
+    } catch (Error e) {
       assertThat(e).hasMessage("Broken!");
     }
     assertThat(writeCount.get()).isEqualTo(1);
@@ -892,7 +893,7 @@ public final class CallTest {
     try {
       call.request();
       fail();
-    } catch (OutOfMemoryError e) {
+    } catch (Error e) {
       assertThat(e).hasMessage("Broken!");
     }
     assertThat(writeCount.get()).isEqualTo(1);
@@ -973,7 +974,7 @@ public final class CallTest {
     assertTrue(latch.await(10, SECONDS));
   }
 
-  @Test public void requestThrowingErrorBeforeEnqueueFailsEnqueue()
+  @Test public void requestThrowingNonFatalErrorBeforeEnqueueFailsEnqueue()
       throws IOException, InterruptedException {
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl(server.url("/"))
@@ -987,7 +988,7 @@ public final class CallTest {
     Object a = new Object() {
       @Override public String toString() {
         writeCount.incrementAndGet();
-        throw new OutOfMemoryError("Broken!");
+        throw new Error("Broken!");
       }
     };
     Call<String> call = service.postRequestBody(a);
@@ -995,7 +996,7 @@ public final class CallTest {
     try {
       call.request();
       fail();
-    } catch (OutOfMemoryError e) {
+    } catch (Error e) {
       assertThat(e).hasMessage("Broken!");
     }
     assertThat(writeCount.get()).isEqualTo(1);
@@ -1006,7 +1007,7 @@ public final class CallTest {
       }
 
       @Override public void onFailure(Call<String> call, Throwable t) {
-        assertThat(t).isExactlyInstanceOf(OutOfMemoryError.class).hasMessage("Broken!");
+        assertThat(t).isExactlyInstanceOf(Error.class).hasMessage("Broken!");
         assertThat(writeCount.get()).isEqualTo(1);
         latch.countDown();
       }
@@ -1090,7 +1091,7 @@ public final class CallTest {
     assertThat(writeCount.get()).isEqualTo(1);
   }
 
-  @Test public void requestAfterEnqueueFailingThrowsForErrors() throws IOException,
+  @Test public void requestAfterEnqueueFailingThrowsForNonFatalErrors() throws IOException,
       InterruptedException {
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl(server.url("/"))
@@ -1104,7 +1105,7 @@ public final class CallTest {
     Object a = new Object() {
       @Override public String toString() {
         writeCount.incrementAndGet();
-        throw new OutOfMemoryError("Broken!");
+        throw new Error("Broken!");
       }
     };
     Call<String> call = service.postRequestBody(a);
@@ -1115,19 +1116,133 @@ public final class CallTest {
       }
 
       @Override public void onFailure(Call<String> call, Throwable t) {
-        assertThat(t).isExactlyInstanceOf(OutOfMemoryError.class).hasMessage("Broken!");
+        assertThat(t).isExactlyInstanceOf(Error.class).hasMessage("Broken!");
         assertThat(writeCount.get()).isEqualTo(1);
         latch.countDown();
       }
     });
     assertTrue(latch.await(10, SECONDS));
 
+    try {
+      call.request();
+      fail();
+    } catch (Error e) {
+      assertThat(e).hasMessage("Broken!");
+    }
+    assertThat(writeCount.get()).isEqualTo(1);
+  }
+
+  @Test public void fatalErrorsAreNotCaughtRequest() throws Exception {
+    Retrofit retrofit = new Retrofit.Builder()
+        .baseUrl(server.url("/"))
+        .addConverterFactory(new ToStringConverterFactory())
+        .build();
+    Service service = retrofit.create(Service.class);
+
+    server.enqueue(new MockResponse());
+
+    final AtomicInteger writeCount = new AtomicInteger();
+    Object a = new Object() {
+      @Override public String toString() {
+        writeCount.incrementAndGet();
+        throw new OutOfMemoryError("Broken!");
+      }
+    };
+    Call<String> call = service.postRequestBody(a);
+
+    try {
+      call.request();
+      fail();
+    } catch (OutOfMemoryError e) {
+      assertThat(e).hasMessage("Broken!");
+    }
+    assertThat(writeCount.get()).isEqualTo(1);
+
+    try {
+      call.request();
+      fail();
+    } catch (OutOfMemoryError e) {
+      assertThat(e).hasMessage("Broken!");
+    }
+    assertThat(writeCount.get()).isEqualTo(2);
+  }
+
+  @Test public void fatalErrorsAreNotCaughtEnqueue() throws Exception {
+    Retrofit retrofit = new Retrofit.Builder()
+        .baseUrl(server.url("/"))
+        .addConverterFactory(new ToStringConverterFactory())
+        .build();
+    Service service = retrofit.create(Service.class);
+
+    server.enqueue(new MockResponse());
+
+    final AtomicInteger writeCount = new AtomicInteger();
+    Object a = new Object() {
+      @Override public String toString() {
+        writeCount.incrementAndGet();
+        throw new OutOfMemoryError("Broken!");
+      }
+    };
+    Call<String> call = service.postRequestBody(a);
+
+    try {
+      final AtomicBoolean callsFailureSynchronously = new AtomicBoolean();
+      call.enqueue(new Callback<String>() {
+        @Override public void onResponse(Call<String> call, Response<String> response) {
+        }
+
+        @Override public void onFailure(Call<String> call, Throwable t) {
+          callsFailureSynchronously.set(true); // Will not be called for fatal errors.
+        }
+      });
+      assertThat(callsFailureSynchronously.get()).isFalse();
+      fail();
+    } catch (OutOfMemoryError e) {
+      assertThat(e).hasMessage("Broken!");
+    }
+    assertThat(writeCount.get()).isEqualTo(1);
+
     try {
       call.request();
       fail();
     } catch (OutOfMemoryError e) {
       assertThat(e).hasMessage("Broken!");
     }
+    assertThat(writeCount.get()).isEqualTo(2);
+  }
+
+  @Test public void fatalErrorsAreNotCaughtExecute() throws Exception {
+    Retrofit retrofit = new Retrofit.Builder()
+        .baseUrl(server.url("/"))
+        .addConverterFactory(new ToStringConverterFactory())
+        .build();
+    Service service = retrofit.create(Service.class);
+
+    server.enqueue(new MockResponse());
+
+    final AtomicInteger writeCount = new AtomicInteger();
+    Object a = new Object() {
+      @Override public String toString() {
+        writeCount.incrementAndGet();
+        throw new OutOfMemoryError("Broken!");
+      }
+    };
+    Call<String> call = service.postRequestBody(a);
+
+    try {
+      call.execute();
+      fail();
+    } catch (OutOfMemoryError e) {
+      assertThat(e).hasMessage("Broken!");
+    }
     assertThat(writeCount.get()).isEqualTo(1);
+
+    try {
+      call.request();
+      fail();
+    } catch (OutOfMemoryError e) {
+      assertThat(e).hasMessage("Broken!");
+    }
+    assertThat(writeCount.get()).isEqualTo(2);
   }
 }