From ed24fd0c4b39b4f33e9b9fcc14404be8951325b9 Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Fri, 8 Nov 2024 10:48:03 -0400 Subject: [PATCH] Utilize re-entrant locking for in app payments instead of synchronized blocks. --- ...nternalDonorErrorConfigurationViewModel.kt | 5 +++-- .../subscription/InAppPaymentsRepository.kt | 6 ++++-- .../RecurringInAppPaymentRepository.kt | 2 +- .../model/InAppPaymentSubscriberRecord.kt | 4 +++- .../jobs/BackupSubscriptionCheckJob.kt | 3 ++- .../jobs/DonationReceiptRedemptionJob.java | 7 ++++++- .../jobs/ExternalLaunchDonationJob.kt | 2 +- .../jobs/InAppPaymentAuthCheckJob.kt | 5 +++-- .../jobs/InAppPaymentKeepAliveJob.kt | 3 ++- .../jobs/InAppPaymentPurchaseTokenJob.kt | 5 +++-- .../jobs/InAppPaymentRecurringContextJob.kt | 3 ++- .../jobs/InAppPaymentRedemptionJob.kt | 3 ++- .../jobs/SubscriptionKeepAliveJob.java | 7 ++++++- ...SubscriptionReceiptRequestResponseJob.java | 7 ++++++- .../securesms/keyvalue/InAppPaymentValues.kt | 5 +++-- .../api/services/DonationsService.java | 19 ++++++++++++++----- 16 files changed, 61 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/donor/InternalDonorErrorConfigurationViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/donor/InternalDonorErrorConfigurationViewModel.kt index 70ed3386d6..2cfbb0eb60 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/donor/InternalDonorErrorConfigurationViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/donor/InternalDonorErrorConfigurationViewModel.kt @@ -20,6 +20,7 @@ import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.util.rx.RxStore import org.whispersystems.signalservice.api.subscriptions.ActiveSubscription import java.util.Locale +import kotlin.concurrent.withLock class InternalDonorErrorConfigurationViewModel : ViewModel() { @@ -101,7 +102,7 @@ class InternalDonorErrorConfigurationViewModel : ViewModel() { fun save(): Completable { val snapshot = store.state val saveState = Completable.fromAction { - synchronized(InAppPaymentSubscriberRecord.Type.DONATION) { + InAppPaymentSubscriberRecord.Type.DONATION.lock.withLock { when { snapshot.selectedBadge?.isGift() == true -> handleGiftExpiration(snapshot) snapshot.selectedBadge?.isBoost() == true -> handleBoostExpiration(snapshot) @@ -116,7 +117,7 @@ class InternalDonorErrorConfigurationViewModel : ViewModel() { fun clearErrorState(): Completable { return Completable.fromAction { - synchronized(InAppPaymentSubscriberRecord.Type.DONATION) { + InAppPaymentSubscriberRecord.Type.DONATION.lock.withLock { SignalStore.inAppPayments.setExpiredBadge(null) SignalStore.inAppPayments.setExpiredGiftBadge(null) SignalStore.inAppPayments.unexpectedSubscriptionCancelationReason = null diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/InAppPaymentsRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/InAppPaymentsRepository.kt index a0b005d13e..32b093d580 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/InAppPaymentsRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/InAppPaymentsRepository.kt @@ -56,6 +56,7 @@ import org.whispersystems.signalservice.internal.push.exceptions.InAppPaymentPro import java.security.SecureRandom import java.util.Currency import java.util.Optional +import java.util.concurrent.locks.Lock import kotlin.jvm.optionals.getOrNull import kotlin.time.Duration import kotlin.time.Duration.Companion.days @@ -231,10 +232,11 @@ object InAppPaymentsRepository { /** * Returns the object to utilize as a mutex for recurring subscriptions. */ - fun resolveMutex(inAppPaymentId: InAppPaymentTable.InAppPaymentId): Any { + @WorkerThread + fun resolveLock(inAppPaymentId: InAppPaymentTable.InAppPaymentId): Lock { val payment = SignalDatabase.inAppPayments.getById(inAppPaymentId) ?: error("Not found") - return payment.type.requireSubscriberType() + return payment.type.requireSubscriberType().lock } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/RecurringInAppPaymentRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/RecurringInAppPaymentRepository.kt index af08accd45..df3e57325a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/RecurringInAppPaymentRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/subscription/RecurringInAppPaymentRepository.kt @@ -214,7 +214,7 @@ object RecurringInAppPaymentRepository { subscriptionLevel, subscriber.currency.currencyCode, levelUpdateOperation.idempotencyKey.serialize(), - subscriberType + subscriberType.lock ) } .flatMapCompletable { diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/InAppPaymentSubscriberRecord.kt b/app/src/main/java/org/thoughtcrime/securesms/database/model/InAppPaymentSubscriberRecord.kt index 522ee8f552..d312683597 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/InAppPaymentSubscriberRecord.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/InAppPaymentSubscriberRecord.kt @@ -9,6 +9,8 @@ import org.signal.donations.InAppPaymentType import org.thoughtcrime.securesms.database.model.databaseprotos.InAppPaymentData import org.whispersystems.signalservice.api.subscriptions.SubscriberId import java.util.Currency +import java.util.concurrent.locks.Lock +import java.util.concurrent.locks.ReentrantLock /** * Represents a SubscriberId and metadata that can be used for a recurring @@ -24,7 +26,7 @@ data class InAppPaymentSubscriberRecord( /** * Serves as the mutex by which to perform mutations to subscriptions. */ - enum class Type(val code: Int, val jobQueue: String, val inAppPaymentType: InAppPaymentType) { + enum class Type(val code: Int, val jobQueue: String, val inAppPaymentType: InAppPaymentType, val lock: Lock = ReentrantLock()) { /** * A recurring donation */ diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupSubscriptionCheckJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupSubscriptionCheckJob.kt index 46aa4f8eb3..9b58baac76 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupSubscriptionCheckJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupSubscriptionCheckJob.kt @@ -22,6 +22,7 @@ import org.thoughtcrime.securesms.jobmanager.Job import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraint import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.util.RemoteConfig +import kotlin.concurrent.withLock /** * Checks and rectifies state pertaining to backups subscriptions. @@ -85,7 +86,7 @@ class BackupSubscriptionCheckJob private constructor(parameters: Parameters) : C val purchase: BillingPurchaseResult = AppDependencies.billingApi.queryPurchases() val hasActivePurchase = purchase is BillingPurchaseResult.Success && purchase.isAcknowledged && purchase.isWithinTheLastMonth() - synchronized(InAppPaymentSubscriberRecord.Type.BACKUP) { + InAppPaymentSubscriberRecord.Type.BACKUP.lock.withLock { val inAppPayment = SignalDatabase.inAppPayments.getLatestInAppPaymentByType(InAppPaymentType.RECURRING_BACKUP) if (inAppPayment?.state == InAppPaymentTable.State.PENDING) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/DonationReceiptRedemptionJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/DonationReceiptRedemptionJob.java index b0cf582d7d..4f1d01554b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/DonationReceiptRedemptionJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/DonationReceiptRedemptionJob.java @@ -29,6 +29,7 @@ import org.whispersystems.signalservice.internal.ServiceResponse; import java.io.IOException; import java.util.Collections; import java.util.Objects; +import java.util.concurrent.locks.Lock; /** * Job to redeem a verified donation receipt. It is up to the Job prior in the chain to specify a valid @@ -157,8 +158,12 @@ public class DonationReceiptRedemptionJob extends BaseJob { @Override protected void onRun() throws Exception { if (isForSubscription()) { - synchronized (InAppPaymentSubscriberRecord.Type.DONATION) { + Lock lock = InAppPaymentSubscriberRecord.Type.DONATION.getLock(); + lock.lock(); + try { doRun(); + } finally { + lock.unlock(); } } else { doRun(); diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/ExternalLaunchDonationJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/ExternalLaunchDonationJob.kt index 1145327eeb..f9f6d34d20 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/ExternalLaunchDonationJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/ExternalLaunchDonationJob.kt @@ -167,7 +167,7 @@ class ExternalLaunchDonationJob private constructor( subscriptionLevel, subscriber.currency.currencyCode, levelUpdateOperation.idempotencyKey.serialize(), - subscriber.type + subscriber.type.lock ) getResultOrThrow(updateSubscriptionLevelResponse, doOnApplicationError = { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentAuthCheckJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentAuthCheckJob.kt index 108f002f07..ca84584e0c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentAuthCheckJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentAuthCheckJob.kt @@ -32,6 +32,7 @@ import org.thoughtcrime.securesms.subscription.LevelUpdate import org.thoughtcrime.securesms.subscription.LevelUpdateOperation import org.thoughtcrime.securesms.util.Environment import org.whispersystems.signalservice.internal.ServiceResponse +import kotlin.concurrent.withLock import kotlin.time.Duration.Companion.days /** @@ -77,7 +78,7 @@ class InAppPaymentAuthCheckJob private constructor(parameters: Parameters) : Bas var hasRetry = false for (payment in unauthorizedInAppPayments) { val verificationStatus: CheckResult = if (payment.type.recurring) { - synchronized(payment.type.requireSubscriberType().inAppPaymentType) { + payment.type.requireSubscriberType().lock.withLock { checkRecurringPayment(payment) } } else { @@ -244,7 +245,7 @@ class InAppPaymentAuthCheckJob private constructor(parameters: Parameters) : Bas level, subscriber.currency.currencyCode, updateOperation.idempotencyKey.serialize(), - subscriber.type + subscriber.type.lock ) val updateLevelResult = checkResult(updateLevelResponse) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentKeepAliveJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentKeepAliveJob.kt index 4d2a275ead..6f1c8de657 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentKeepAliveJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentKeepAliveJob.kt @@ -27,6 +27,7 @@ import org.whispersystems.signalservice.api.subscriptions.ActiveSubscription import org.whispersystems.signalservice.internal.EmptyResponse import org.whispersystems.signalservice.internal.ServiceResponse import java.util.Locale +import kotlin.concurrent.withLock import kotlin.jvm.optionals.getOrNull import kotlin.time.Duration import kotlin.time.Duration.Companion.days @@ -90,7 +91,7 @@ class InAppPaymentKeepAliveJob private constructor( } override fun onRun() { - synchronized(type) { + type.lock.withLock { doRun() } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentPurchaseTokenJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentPurchaseTokenJob.kt index fa6b36345f..65b879228f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentPurchaseTokenJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentPurchaseTokenJob.kt @@ -18,6 +18,7 @@ import org.thoughtcrime.securesms.dependencies.AppDependencies import org.thoughtcrime.securesms.jobmanager.Job import org.thoughtcrime.securesms.jobmanager.JobManager.Chain import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraint +import kotlin.concurrent.withLock /** * Submits a purchase token to the server to link it with a subscriber id. @@ -76,7 +77,7 @@ class InAppPaymentPurchaseTokenJob private constructor( } override fun onRun() { - synchronized(InAppPaymentsRepository.resolveMutex(inAppPaymentId)) { + InAppPaymentsRepository.resolveLock(inAppPaymentId).withLock { doRun() } } @@ -87,7 +88,7 @@ class InAppPaymentPurchaseTokenJob private constructor( val response = AppDependencies.donationsService.linkGooglePlayBillingPurchaseTokenToSubscriberId( inAppPayment.subscriberId!!, inAppPayment.data.redemption!!.googlePlayBillingPurchaseToken!!, - InAppPaymentSubscriberRecord.Type.BACKUP + InAppPaymentSubscriberRecord.Type.BACKUP.lock ) if (response.applicationError.isPresent) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRecurringContextJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRecurringContextJob.kt index 1cbb4dfdcc..3081292233 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRecurringContextJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRecurringContextJob.kt @@ -32,6 +32,7 @@ import org.whispersystems.signalservice.api.subscriptions.ActiveSubscription.Sub import org.whispersystems.signalservice.internal.ServiceResponse import java.io.IOException import java.util.Currency +import kotlin.concurrent.withLock import kotlin.time.Duration.Companion.days import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds @@ -128,7 +129,7 @@ class InAppPaymentRecurringContextJob private constructor( } override fun onRun() { - synchronized(InAppPaymentsRepository.resolveMutex(inAppPaymentId)) { + InAppPaymentsRepository.resolveLock(inAppPaymentId).withLock { doRun() } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRedemptionJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRedemptionJob.kt index de05a96061..b8b16566fc 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRedemptionJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/InAppPaymentRedemptionJob.kt @@ -25,6 +25,7 @@ import org.thoughtcrime.securesms.util.hasGiftBadge import org.thoughtcrime.securesms.util.requireGiftBadge import org.whispersystems.signalservice.internal.ServiceResponse import java.io.IOException +import kotlin.concurrent.withLock /** * Takes a ReceiptCredentialResponse and submits it to the server for redemption. @@ -181,7 +182,7 @@ class InAppPaymentRedemptionJob private constructor( } if (inAppPayment.type.recurring) { - synchronized(inAppPayment.type.requireSubscriberType()) { + inAppPayment.type.requireSubscriberType().lock.withLock { performInAppPaymentRedemption(inAppPayment) } } else { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionKeepAliveJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionKeepAliveJob.java index 6f740d9793..54a851d159 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionKeepAliveJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionKeepAliveJob.java @@ -20,6 +20,7 @@ import org.whispersystems.signalservice.internal.ServiceResponse; import java.io.IOException; import java.util.Locale; import java.util.Objects; +import java.util.concurrent.locks.Lock; import okio.ByteString; @@ -57,8 +58,12 @@ public class SubscriptionKeepAliveJob extends BaseJob { @Override protected void onRun() throws Exception { - synchronized (InAppPaymentSubscriberRecord.Type.DONATION) { + Lock lock = InAppPaymentSubscriberRecord.Type.DONATION.getLock(); + lock.lock(); + try { doRun(); + } finally { + lock.unlock(); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionReceiptRequestResponseJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionReceiptRequestResponseJob.java index a9c06c9d53..5af69163ac 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionReceiptRequestResponseJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/SubscriptionReceiptRequestResponseJob.java @@ -39,6 +39,7 @@ import org.whispersystems.signalservice.internal.ServiceResponse; import java.io.IOException; import java.util.Locale; import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; import okio.ByteString; @@ -140,8 +141,12 @@ public class SubscriptionReceiptRequestResponseJob extends BaseJob { @Override protected void onRun() throws Exception { - synchronized (InAppPaymentSubscriberRecord.Type.DONATION) { + Lock lock = InAppPaymentSubscriberRecord.Type.DONATION.getLock(); + lock.lock(); + try { doRun(); + } finally { + lock.unlock(); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/InAppPaymentValues.kt b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/InAppPaymentValues.kt index f74bfed2e3..753813c3be 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/InAppPaymentValues.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/InAppPaymentValues.kt @@ -33,6 +33,7 @@ import java.util.Currency import java.util.Locale import java.util.Optional import java.util.concurrent.TimeUnit +import kotlin.concurrent.withLock /** * Key-Value store for in app payment related values. Note that most of this file will be deprecated after the release of @@ -449,7 +450,7 @@ class InAppPaymentValues internal constructor(store: KeyValueStore) : SignalStor */ @WorkerThread fun updateLocalStateForManualCancellation(subscriberType: InAppPaymentSubscriberRecord.Type) { - synchronized(subscriberType) { + subscriberType.lock.withLock { Log.d(TAG, "[updateLocalStateForManualCancellation] Clearing donation values.") clearLevelOperations() @@ -493,7 +494,7 @@ class InAppPaymentValues internal constructor(store: KeyValueStore) : SignalStor */ @WorkerThread fun updateLocalStateForLocalSubscribe(subscriberType: InAppPaymentSubscriberRecord.Type) { - synchronized(subscriberType) { + subscriberType.lock.withLock { clearLevelOperations() if (subscriberType == InAppPaymentSubscriberRecord.Type.DONATION) { diff --git a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/services/DonationsService.java b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/services/DonationsService.java index 0e4ee458d4..01181596c7 100644 --- a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/services/DonationsService.java +++ b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/services/DonationsService.java @@ -27,6 +27,7 @@ import java.util.Locale; import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; import io.reactivex.rxjava3.annotations.NonNull; @@ -167,27 +168,35 @@ public class DonationsService { * @param level The new level to subscribe to * @param currencyCode The currencyCode the user is using for payment * @param idempotencyKey url-safe-base64-encoded random 16-byte value (see description) - * @param mutex A mutex to lock on to avoid a situation where this subscription update happens *as* we are trying to get a credential receipt. + * @param lock A lock to lock on to avoid a situation where this subscription update happens *as* we are trying to get a credential receipt. */ public ServiceResponse updateSubscriptionLevel(SubscriberId subscriberId, String level, String currencyCode, String idempotencyKey, - Object mutex + Lock lock ) { return wrapInServiceResponse(() -> { - synchronized (mutex) { + lock.lock(); + + try { pushServiceSocket.updateSubscriptionLevel(subscriberId.serialize(), level, currencyCode, idempotencyKey); + } finally { + lock.unlock(); } + return new Pair<>(EmptyResponse.INSTANCE, 200); }); } - public ServiceResponse linkGooglePlayBillingPurchaseTokenToSubscriberId(SubscriberId subscriberId, String purchaseToken, Object mutex) { + public ServiceResponse linkGooglePlayBillingPurchaseTokenToSubscriberId(SubscriberId subscriberId, String purchaseToken, Lock lock) { return wrapInServiceResponse(() -> { - synchronized (mutex) { + lock.lock(); + try { pushServiceSocket.linkPlayBillingPurchaseToken(subscriberId.serialize(), purchaseToken); + } finally { + lock.unlock(); } return new Pair<>(EmptyResponse.INSTANCE, 200);