Repository: square/retrofit
Issue: The callback Executor should check for cancellation before delivering the result

Currently if the callback `Executor` executes the callback on a different thread (the default on Android), then there is no check performed for `Call` cancellation, and the result may be delivered even after it has been cancelled. This is not a good design, and may confound expectations from the client code (as e.g. `AsyncTask` handles these cases properly).


Issue comments:

Pull request review comments:
Do we need this, though? If OkHttp throws an exception when canceled it will be automatically propagated to the callback. I think there is even a test for this already.

This change is for handling the case where the `Call` is cancelled after the response has been delivered by OkHttp, while still being delivered by the callback `Executor`. As mentioned in #1135, this would cause the result to be delivered to the callback even after cancellation.


Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit/ExecutorCallAdapterFactory.java b/retrofit/src/main/java/retrofit/ExecutorCallAdapterFactory.java
index 06a29ddae0..1e545d51be 100644
--- a/retrofit/src/main/java/retrofit/ExecutorCallAdapterFactory.java
+++ b/retrofit/src/main/java/retrofit/ExecutorCallAdapterFactory.java
@@ -47,14 +47,36 @@ public CallAdapter<Call<?>> get(Type returnType, Annotation[] annotations, Retro
   static final class ExecutorCallbackCall<T> implements Call<T> {
     private final Executor callbackExecutor;
     private final Call<T> delegate;
+    private volatile boolean canceled;
 
     ExecutorCallbackCall(Executor callbackExecutor, Call<T> delegate) {
       this.callbackExecutor = callbackExecutor;
       this.delegate = delegate;
     }
 
-    @Override public void enqueue(Callback<T> callback) {
-      delegate.enqueue(new ExecutorCallback<>(callbackExecutor, callback));
+    @Override public void enqueue(final Callback<T> callback) {
+      delegate.enqueue(new Callback<T>() {
+        @Override public void onResponse(final Response<T> response, final Retrofit retrofit) {
+          callbackExecutor.execute(new Runnable() {
+            @Override public void run() {
+              if (canceled) {
+                // Emulate OkHttp's behavior of throwing/delivering an IOException on cancelation
+                callback.onFailure(new IOException("Canceled"));
+              } else {
+                callback.onResponse(response, retrofit);
+              }
+            }
+          });
+        }
+
+        @Override public void onFailure(final Throwable t) {
+          callbackExecutor.execute(new Runnable() {
+            @Override public void run() {
+              callback.onFailure(t);
+            }
+          });
+        }
+      });
     }
 
     @Override public Response<T> execute() throws IOException {
@@ -63,6 +85,7 @@ static final class ExecutorCallbackCall<T> implements Call<T> {
 
     @Override public void cancel() {
       delegate.cancel();
+      canceled = true;
     }
 
     @SuppressWarnings("CloneDoesntCallSuperClone") // Performing deep clone.
@@ -70,30 +93,4 @@ static final class ExecutorCallbackCall<T> implements Call<T> {
       return new ExecutorCallbackCall<>(callbackExecutor, delegate.clone());
     }
   }
-
-  static final class ExecutorCallback<T> implements Callback<T> {
-    private final Executor callbackExecutor;
-    private final Callback<T> delegate;
-
-    ExecutorCallback(Executor callbackExecutor, Callback<T> delegate) {
-      this.callbackExecutor = callbackExecutor;
-      this.delegate = delegate;
-    }
-
-    @Override public void onResponse(final Response<T> response, final Retrofit retrofit) {
-      callbackExecutor.execute(new Runnable() {
-        @Override public void run() {
-          delegate.onResponse(response, retrofit);
-        }
-      });
-    }
-
-    @Override public void onFailure(final Throwable t) {
-      callbackExecutor.execute(new Runnable() {
-        @Override public void run() {
-          delegate.onFailure(t);
-        }
-      });
-    }
-  }
 }


Test patch: