Clean up reference to user callback on cancel or when call complete (#…

…413)
Shopify · Jul 11, 2017 · d5ef552 · d5ef552
1 parent c616a9b
commit d5ef552
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 43 deletions.
diff --git a/MobileBuy/buy3-pay-support/src/main/AndroidManifest.xml b/MobileBuy/buy3-pay-support/src/main/AndroidManifest.xml
@@ -1,8 +1,3 @@
-<manifest package="com.shopify.buy3.pay"
-    xmlns:android="http://schemas.android.com/apk/res/android">
+<manifest package="com.shopify.buy3.pay">
 
-    <application
-        android:allowBackup="false"
-        android:label="@string/app_name"
-        android:supportsRtl="true" />
 </manifest>
diff --git a/MobileBuy/buy3-pay-support/src/main/res/values/strings.xml b/MobileBuy/buy3-pay-support/src/main/res/values/strings.xml
diff --git a/MobileBuy/buy3/src/androidTest/java/com/shopify/buy3/GraphClientIntegrationTest.java b/MobileBuy/buy3/src/androidTest/java/com/shopify/buy3/GraphClientIntegrationTest.java
@@ -237,7 +237,7 @@ public class GraphClientIntegrationTest {
 
     countDownLatch.await(5, TimeUnit.SECONDS);
     assertThat(call.isCanceled()).isTrue();
-    assertThat(executedInCallbackHandler.get()).isFalse();
+    assertThat(executedInCallbackHandler.get()).isTrue();
   }
 
   @Test public void canceledErrorDuringExecute() throws Exception {

diff --git a/MobileBuy/buy3/src/main/java/com/shopify/buy3/RealGraphCall.java b/MobileBuy/buy3/src/main/java/com/shopify/buy3/RealGraphCall.java
@@ -37,6 +37,7 @@
 import java.io.IOException;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 import okhttp3.Call;
 import okhttp3.HttpUrl;
@@ -63,6 +64,7 @@ abstract class RealGraphCall<T extends AbstractResponse<T>> implements GraphCall
   private volatile Call httpCall;
   private volatile HttpCallbackWithRetry httpCallbackWithRetry;
   private volatile boolean canceled;
+  private CallbackProxy<T> responseCallback;
 
   RealGraphCall(final Query query, final HttpUrl serverUrl, final Call.Factory httpCallFactory,
     final ResponseDataConverter<T> responseDataConverter, final ScheduledExecutorService dispatcher,
@@ -89,6 +91,11 @@ abstract class RealGraphCall<T extends AbstractResponse<T>> implements GraphCall
   @Override public void cancel() {
     canceled = true;
 
+    CallbackProxy<T> callbackProxy = responseCallback;
+    if (callbackProxy != null) {
+      callbackProxy.cancel();
+    }
+
     HttpCallbackWithRetry httpCallbackWithRetry = this.httpCallbackWithRetry;
     if (httpCallbackWithRetry != null) {
       httpCallbackWithRetry.cancel();
@@ -123,11 +130,11 @@ abstract class RealGraphCall<T extends AbstractResponse<T>> implements GraphCall
     try {
       graphResponse = httpResponseParser.parse(response);
       if (graphResponse.hasErrors()) {
-        removeCachedResponse(httpCall.request());
+        removeCachedResponse();
       }
     } catch (Exception rethrow) {
       if (rethrow instanceof GraphParseError) {
-        removeCachedResponse(httpCall.request());
+        removeCachedResponse();
       }
       throw rethrow;
     } finally {
@@ -151,42 +158,23 @@ abstract class RealGraphCall<T extends AbstractResponse<T>> implements GraphCall
     if (!executed.compareAndSet(false, true)) {
       throw new IllegalStateException("Already Executed");
     }
-
     checkNotNull(retryHandler, "retryHandler == null");
 
-    if (canceled) {
-      callback.onFailure(new GraphCallCanceledError());
-      return this;
-    }
-
-    final Callback<T> proxyCallBack = new Callback<T>() {
-      @Override public void onResponse(@NonNull final GraphResponse<T> response) {
-        if (canceled) {
-          callback.onFailure(new GraphCallCanceledError());
-        } else {
-          if (response.hasErrors()) {
-            removeCachedResponse(httpCall.request());
-          }
-          callback.onResponse(response);
-        }
-      }
-
-      @Override public void onFailure(@NonNull final GraphError error) {
-        if (error instanceof GraphParseError) {
-          removeCachedResponse(httpCall.request());
-        }
+    responseCallback = new CallbackProxy<T>(this, callback, handler);
 
-        if (canceled) {
-          callback.onFailure(new GraphCallCanceledError());
+    dispatcher.execute(() -> {
+      if (canceled) {
+        if (handler != null) {
+          handler.post(() -> responseCallback.cancel());
         } else {
-          callback.onFailure(error);
+          responseCallback.cancel();
         }
+        return;
       }
-    };
 
-    dispatcher.execute(() -> {
       httpCall = httpCall();
-      httpCallbackWithRetry = new HttpCallbackWithRetry<>(httpCall, httpResponseParser, retryHandler, proxyCallBack, dispatcher, handler);
+      httpCallbackWithRetry = new HttpCallbackWithRetry<>(httpCall, httpResponseParser, retryHandler, responseCallback, dispatcher,
+        handler);
       httpCall.enqueue(httpCallbackWithRetry);
     });
 
@@ -215,8 +203,8 @@ private void checkIfCanceled() throws GraphCallCanceledError {
     }
   }
 
-  private void removeCachedResponse(@NonNull final Request request) {
-    String cacheKey = request.header(HttpCache.CACHE_KEY_HEADER);
+  private void removeCachedResponse() {
+    String cacheKey = httpCall != null ? httpCall.request().header(HttpCache.CACHE_KEY_HEADER) : null;
     if (httpCache == null || cacheKey == null || cacheKey.isEmpty()) {
       return;
     }
@@ -227,4 +215,50 @@ private void removeCachedResponse(@NonNull final Request request) {
   interface ResponseDataConverter<R extends AbstractResponse<R>> {
     R convert(TopLevelResponse response) throws SchemaViolationError;
   }
+
+  private static class CallbackProxy<T extends AbstractResponse<T>> implements Callback<T> {
+    final RealGraphCall<T> graphCall;
+    final AtomicReference<Callback<T>> originalCallbackRef;
+    final Handler handler;
+
+    CallbackProxy(final RealGraphCall<T> graphCall, final Callback<T> originalCallback, final Handler handler) {
+      this.graphCall = graphCall;
+      this.originalCallbackRef = new AtomicReference<>(originalCallback);
+      this.handler = handler;
+    }
+
+    @Override public void onResponse(@NonNull final GraphResponse<T> response) {
+      if (response.hasErrors()) {
+        graphCall.removeCachedResponse();
+      }
+
+      Callback<T> originalCallback = originalCallbackRef.get();
+      if (originalCallback == null || !originalCallbackRef.compareAndSet(originalCallback, null)) {
+        return;
+      }
+      originalCallback.onResponse(response);
+    }
+
+    @Override public void onFailure(@NonNull final GraphError error) {
+      if (error instanceof GraphParseError) {
+        graphCall.removeCachedResponse();
+      }
+
+      Callback<T> originalCallback = originalCallbackRef.get();
+      if (originalCallback != null && originalCallbackRef.compareAndSet(originalCallback, null)) {
+        originalCallback.onFailure(error);
+      }
+    }
+
+    void cancel() {
+      Callback<T> originalCallback = originalCallbackRef.getAndSet(null);
+      if (originalCallback != null) {
+        if (handler != null) {
+          handler.post(() -> originalCallback.onFailure(new GraphCallCanceledError()));
+        } else {
+          originalCallback.onFailure(new GraphCallCanceledError());
+        }
+      }
+    }
+  }
 }
diff --git a/MobileBuy/buy3/src/test/java/com/shopify/buy3/RetryTest.java b/MobileBuy/buy3/src/test/java/com/shopify/buy3/RetryTest.java
@@ -241,8 +241,9 @@ public class RetryTest {
 
     Thread.sleep(TimeUnit.SECONDS.toMillis(3));
     call.cancel();
+    Thread.sleep(TimeUnit.SECONDS.toMillis(2));
 
-    if (graphError.get() != null || graphResponse.get() != null) {
+    if (!(graphError.get() instanceof GraphCallCanceledError) || graphResponse.get() != null) {
       fail("Expected to cancel");
     }
   }