Skip to content

Commit

Permalink
Use strong references to callbacks in Stripe and CustomerSession (#863)
Browse files Browse the repository at this point in the history 
**Summary**
`Stripe` and `CustomerSession` methods accept callback objects
that typically hold references to `Activity` objects. The best
practice in Android is that references to `Activity` objects
should be weakly held for long-running background tasks, because
the `Activity` may go away (e.g. user leaves activity before task
is complete). The change was first introduced in #666.

After this change, users began reporting that callback objects
would mysteriously be garbage-collected by the OS. I was able
to reproduce this as well, and found a work-around by moving
anonymous inline classes to instance variables - see 2b74328.

Android will GC objects that are no longer strongly referenced.
When inlined, the callback objects were no longer strongly
referenced once the method completed (e.g. in 2b74328, the
left-side of `AddSourceActivity#onActionSave`); moving these
objects to be instance variables meant that they were now
strongly-referenced by the Activity. This explains why the GC
issues were resolved with this change.

In conclusion, the correct solution is to have the callback
objects weakly-reference `Activity` objects, rather than
weakly-reference the callback objects themselves.

**Motivation**
ANDROID-340
  • Loading branch information
@mshafrir-stripe
mshafrir-stripe committed Apr 11, 2019
1 parent 6d5daac commit 9804bcd
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 46 deletions.
41 changes: 17 additions & 24 deletions stripe/src/main/java/com/stripe/android/CustomerSession.java
View file
Expand Up @@ -17,8 +17,6 @@
import com.stripe.android.model.ShippingInformation;
import com.stripe.android.model.Source;

import java.lang.ref.Reference;
import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Calendar;
Expand Down Expand Up @@ -79,10 +77,10 @@ public class CustomerSession
@Nullable private Customer mCustomer;
private long mCustomerCacheTime;
@Nullable private Context mContext;
@NonNull private final Map<String, WeakReference<CustomerRetrievalListener>>
mCustomerRetrievalListenerRefs = new HashMap<>();
@NonNull private final Map<String, WeakReference<SourceRetrievalListener>>
mSourceRetrievalListenerRefs = new HashMap<>();
@NonNull private final Map<String, CustomerRetrievalListener> mCustomerRetrievalListeners =
new HashMap<>();
@NonNull private final Map<String, SourceRetrievalListener> mSourceRetrievalListeners =
new HashMap<>();

@NonNull private final EphemeralKeyManager mEphemeralKeyManager;
@NonNull private final Handler mUiThreadHandler;
Expand Down Expand Up @@ -134,8 +132,8 @@ public static void endCustomerSession() {
@VisibleForTesting
static void clearInstance() {
if (mInstance != null) {
mInstance.mCustomerRetrievalListenerRefs.clear();
mInstance.mSourceRetrievalListenerRefs.clear();
mInstance.mCustomerRetrievalListeners.clear();
mInstance.mSourceRetrievalListeners.clear();
}
cancelCallbacks();
setInstance(null);
Expand Down Expand Up @@ -202,7 +200,7 @@ public void retrieveCurrentCustomer(@NonNull CustomerRetrievalListener listener)
mCustomer = null;

final String operationId = UUID.randomUUID().toString();
mCustomerRetrievalListenerRefs.put(operationId, new WeakReference<>(listener));
mCustomerRetrievalListeners.put(operationId, listener);
mEphemeralKeyManager.retrieveEphemeralKey(operationId, null, null);
}
}
Expand All @@ -217,7 +215,7 @@ public void updateCurrentCustomer(@NonNull CustomerRetrievalListener listener) {
mCustomer = null;

final String operationId = UUID.randomUUID().toString();
mCustomerRetrievalListenerRefs.put(operationId, new WeakReference<>(listener));
mCustomerRetrievalListeners.put(operationId, listener);
mEphemeralKeyManager.retrieveEphemeralKey(operationId, null, null);
}

Expand Down Expand Up @@ -256,7 +254,7 @@ public void addCustomerSource(

final String operationId = UUID.randomUUID().toString();
if (listener != null) {
mSourceRetrievalListenerRefs.put(operationId, new WeakReference<>(listener));
mSourceRetrievalListeners.put(operationId, listener);
}
mEphemeralKeyManager.retrieveEphemeralKey(operationId, ACTION_ADD_SOURCE, arguments);
}
Expand All @@ -278,7 +276,7 @@ public void deleteCustomerSource(

final String operationId = UUID.randomUUID().toString();
if (listener != null) {
mSourceRetrievalListenerRefs.put(operationId, new WeakReference<>(listener));
mSourceRetrievalListeners.put(operationId, listener);
}
mEphemeralKeyManager.retrieveEphemeralKey(operationId, ACTION_DELETE_SOURCE, arguments);
}
Expand Down Expand Up @@ -319,7 +317,7 @@ public void setCustomerDefaultSource(

final String operationId = UUID.randomUUID().toString();
if (listener != null) {
mCustomerRetrievalListenerRefs.put(operationId, new WeakReference<>(listener));
mCustomerRetrievalListeners.put(operationId, listener);
}
mEphemeralKeyManager.retrieveEphemeralKey(operationId, ACTION_SET_DEFAULT_SOURCE,
arguments);
Expand Down Expand Up @@ -629,16 +627,15 @@ public void handleMessage(@NonNull Message msg) {
private void handleRetrievalError(@Nullable String operationId,
@NonNull StripeException exception,
int errorType) {
final WeakReference<? extends RetrievalListener> listenerRef;
final RetrievalListener listener;
if (errorType == SOURCE_ERROR) {
listenerRef = mSourceRetrievalListenerRefs.remove(operationId);
listener = mSourceRetrievalListeners.remove(operationId);
} else if (errorType == CUSTOMER_ERROR) {
listenerRef = mCustomerRetrievalListenerRefs.remove(operationId);
listener = mCustomerRetrievalListeners.remove(operationId);
} else {
listenerRef = null;
listener = null;
}

final RetrievalListener listener = listenerRef != null ? listenerRef.get() : null;
if (listener != null) {
final int errorCode = exception.getStatusCode() == null
? 400
Expand Down Expand Up @@ -758,16 +755,12 @@ private void sendErrorIntent(@NonNull StripeException exception) {

@Nullable
private CustomerRetrievalListener getCustomerRetrievalListener(@Nullable String operationId) {
final Reference<CustomerRetrievalListener> listenerRef =
mCustomerRetrievalListenerRefs.remove(operationId);
return listenerRef != null ? listenerRef.get() : null;
return mCustomerRetrievalListeners.remove(operationId);
}

@Nullable
private SourceRetrievalListener getSourceRetrievalListener(@Nullable String operationId) {
final Reference<SourceRetrievalListener> listenerRef =
mSourceRetrievalListenerRefs.remove(operationId);
return listenerRef != null ? listenerRef.get() : null;
return mSourceRetrievalListeners.remove(operationId);
}

public interface CustomerRetrievalListener extends RetrievalListener {
Expand Down
38 changes: 16 additions & 22 deletions stripe/src/main/java/com/stripe/android/Stripe.java
View file
Expand Up @@ -947,7 +947,7 @@ private static class CreateSourceTask extends AsyncTask<Void, Void, ResponseWrap
@NonNull private final SourceParams mSourceParams;
@NonNull private final String mPublishableKey;
@Nullable private final String mStripeAccount;
@NonNull private final WeakReference<SourceCallback> mSourceCallbackRef;
@NonNull private final SourceCallback mSourceCallback;

CreateSourceTask(@NonNull Context context,
@NonNull StripeApiHandler apiHandler,
Expand All @@ -960,7 +960,7 @@ private static class CreateSourceTask extends AsyncTask<Void, Void, ResponseWrap
mSourceParams = sourceParams;
mPublishableKey = publishableKey;
mStripeAccount = stripeAccount;
mSourceCallbackRef = new WeakReference<>(sourceCallback);
mSourceCallback = sourceCallback;
}

@Override
Expand All @@ -981,13 +981,10 @@ protected ResponseWrapper doInBackground(Void... params) {

@Override
protected void onPostExecute(@NonNull ResponseWrapper responseWrapper) {
final SourceCallback sourceCallback = mSourceCallbackRef.get();
if (sourceCallback != null) {
if (responseWrapper.source != null) {
sourceCallback.onSuccess(responseWrapper.source);
} else if (responseWrapper.error != null) {
sourceCallback.onError(responseWrapper.error);
}
if (responseWrapper.source != null) {
mSourceCallback.onSuccess(responseWrapper.source);
} else if (responseWrapper.error != null) {
mSourceCallback.onError(responseWrapper.error);
}
}
}
Expand All @@ -999,7 +996,7 @@ private static class CreateTokenTask extends AsyncTask<Void, Void, ResponseWrapp
@NonNull private final String mPublishableKey;
@Nullable private final String mStripeAccount;
@NonNull @Token.TokenType private final String mTokenType;
@NonNull private final WeakReference<TokenCallback> mCallbackRef;
@NonNull private final TokenCallback mCallback;
@Nullable private final StripeApiHandler.LoggingResponseListener mLoggingResponseListener;

CreateTokenTask(@NonNull Context context,
Expand All @@ -1008,7 +1005,7 @@ private static class CreateTokenTask extends AsyncTask<Void, Void, ResponseWrapp
@NonNull final String publishableKey,
@Nullable final String stripeAccount,
@NonNull @Token.TokenType final String tokenType,
@Nullable final TokenCallback callback,
@NonNull final TokenCallback callback,
@Nullable final StripeApiHandler.LoggingResponseListener loggingResponseListener) {
mContextRef = new WeakReference<>(context);
mApiHandler = apiHandler;
Expand All @@ -1017,7 +1014,7 @@ private static class CreateTokenTask extends AsyncTask<Void, Void, ResponseWrapp
mStripeAccount = stripeAccount;
mTokenType = tokenType;
mLoggingResponseListener = loggingResponseListener;
mCallbackRef = new WeakReference<>(callback);
mCallback = callback;
}

@Override
Expand All @@ -1043,16 +1040,13 @@ protected void onPostExecute(@NonNull ResponseWrapper result) {
}

private void tokenTaskPostExecution(@NonNull ResponseWrapper result) {
final TokenCallback callback = mCallbackRef.get();
if (callback != null) {
if (result.token != null) {
callback.onSuccess(result.token);
} else if (result.error != null) {
callback.onError(result.error);
} else {
callback.onError(new RuntimeException("Somehow got neither a token response or"
+ " an error response"));
}
if (result.token != null) {
mCallback.onSuccess(result.token);
} else if (result.error != null) {
mCallback.onError(result.error);
} else {
mCallback.onError(new RuntimeException(
"Somehow got neither a token response or an error response"));
}
}
}
Expand Down

0 comments on commit 9804bcd

Please sign in to comment.