Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize SDK initialization when requests executed before any activity starts #1204

Merged
merged 6 commits into from
Aug 22, 2023
Merged

Optimize SDK initialization when requests executed before any activity starts #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a02e52f
Call callbacks without jittering delay if posterior request does not …
tonidero Aug 18, 2023
2cb7961
Several optimizations to SDK initialization and support jittered call…
tonidero Aug 18, 2023
6aa761f
Some cleanup from review
tonidero Aug 21, 2023
c87689b
Improve logs
tonidero Aug 21, 2023
6015d47
Add tests to verify offerings cache timestamp behavior
tonidero Aug 21, 2023
088de84
Add basic tests to verify behavior for background aware callbacks
tonidero Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions purchases/src/main/kotlin/com/revenuecat/purchases/amazon/AmazonBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.revenuecat.purchases.amazon

import com.revenuecat.purchases.PurchasesError
import com.revenuecat.purchases.common.BackendHelper
import com.revenuecat.purchases.common.Delay
import com.revenuecat.purchases.common.networking.Endpoint
import org.json.JSONObject

Expand Down Expand Up @@ -34,6 +35,7 @@ internal class AmazonBackend(
Endpoint.GetAmazonReceipt(storeUserID, receiptId),
body = null,
postFieldsToSign = null,
delay = Delay.NONE,
{ error ->
synchronized(this@AmazonBackend) {
postAmazonReceiptCallbacks.remove(cacheKey)
Expand Down
56 changes: 48 additions & 8 deletions purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
SHOULD_NOT_CONSUME,
}

@Suppress("TooManyFunctions")
internal class Backend(
private val appConfig: AppConfig,
private val dispatcher: Dispatcher,
Expand All @@ -77,13 +78,13 @@
get() = httpClient.signingManager.signatureVerificationMode

@get:Synchronized @set:Synchronized
@Volatile var callbacks = mutableMapOf<CallbackCacheKey, MutableList<CustomerInfoCallback>>()
@Volatile var callbacks = mutableMapOf<BackgroundAwareCallbackCacheKey, MutableList<CustomerInfoCallback>>()

@get:Synchronized @set:Synchronized
@Volatile var postReceiptCallbacks = mutableMapOf<CallbackCacheKey, MutableList<PostReceiptCallback>>()

@get:Synchronized @set:Synchronized
@Volatile var offeringsCallbacks = mutableMapOf<String, MutableList<OfferingsCallback>>()
@Volatile var offeringsCallbacks = mutableMapOf<BackgroundAwareCallbackCacheKey, MutableList<OfferingsCallback>>()

@get:Synchronized @set:Synchronized
@Volatile var identifyCallbacks = mutableMapOf<CallbackCacheKey, MutableList<IdentifyCallback>>()
Expand Down Expand Up @@ -112,9 +113,9 @@
// If it did, future `getCustomerInfo` would receive a cached value
// instead of an up-to-date `CustomerInfo` after those post receipt operations finish.
if (postReceiptCallbacks.isEmpty()) {
listOf(path)
BackgroundAwareCallbackCacheKey(listOf(path), appInBackground)
} else {
listOf(path) + "${callbacks.count()}"
BackgroundAwareCallbackCacheKey(listOf(path) + "${callbacks.count()}", appInBackground)
}
}
val call = object : Dispatcher.AsyncCall() {
Expand Down Expand Up @@ -160,7 +161,7 @@
}
synchronized(this@Backend) {
val delay = if (appInBackground) Delay.DEFAULT else Delay.NONE
callbacks.addCallback(call, dispatcher, cacheKey, onSuccess to onError, delay)
callbacks.addBackgroundAwareCallback(call, dispatcher, cacheKey, onSuccess to onError, delay)
}
}

Expand Down Expand Up @@ -281,6 +282,7 @@
) {
val endpoint = Endpoint.GetOfferings(appUserID)
val path = endpoint.getPath()
val cacheKey = BackgroundAwareCallbackCacheKey(listOf(path), appInBackground)
val call = object : Dispatcher.AsyncCall() {
override fun call(): HTTPResult {
return httpClient.performRequest(
Expand All @@ -295,15 +297,15 @@
override fun onError(error: PurchasesError) {
val isServerError = false
synchronized(this@Backend) {
offeringsCallbacks.remove(path)
offeringsCallbacks.remove(cacheKey)

Check warning on line 300 in purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt

View check run for this annotation

Codecov / codecov/patch

purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt#L300 

Added line #L300 was not covered by tests
}?.forEach { (_, onError) ->
onError(error, isServerError)
}
}

override fun onCompletion(result: HTTPResult) {
synchronized(this@Backend) {
offeringsCallbacks.remove(path)
offeringsCallbacks.remove(cacheKey)
}?.forEach { (onSuccess, onError) ->
if (result.isSuccessful()) {
try {
Expand All @@ -323,7 +325,7 @@
}
synchronized(this@Backend) {
val delay = if (appInBackground) Delay.DEFAULT else Delay.NONE
offeringsCallbacks.addCallback(call, dispatcher, path, onSuccess to onError, delay)
offeringsCallbacks.addBackgroundAwareCallback(call, dispatcher, cacheKey, onSuccess to onError, delay)
}
}

Expand Down Expand Up @@ -510,6 +512,39 @@
PostReceiptErrorHandlingBehavior.SHOULD_BE_CONSUMED
}

@Synchronized
private fun <S, E> MutableMap<BackgroundAwareCallbackCacheKey, MutableList<Pair<S, E>>>.addBackgroundAwareCallback(
call: Dispatcher.AsyncCall,
dispatcher: Dispatcher,
cacheKey: BackgroundAwareCallbackCacheKey,
functions: Pair<S, E>,
delay: Delay = Delay.NONE,

Check warning on line 521 in purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt

View check run for this annotation

Codecov / codecov/patch

purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt#L521 

Added line #L521 was not covered by tests
) {
val foregroundCacheKey = cacheKey.copy(appInBackground = false)
val foregroundCallAlreadyInPlace = containsKey(foregroundCacheKey)
val cacheKeyToUse = if (cacheKey.appInBackground && foregroundCallAlreadyInPlace) {
warnLog(NetworkStrings.SAME_CALL_SCHEDULED_WITHOUT_JITTER.format(foregroundCacheKey))
foregroundCacheKey
} else {
cacheKey
}
addCallback(call, dispatcher, cacheKeyToUse, functions, delay)
// In case we have a request with a jittered delay queued, and we perform the same request without
// jittered delay, we want to call the callback using the unjittered request
val backgroundedCacheKey = cacheKey.copy(appInBackground = true)
val backgroundCallAlreadyInPlace = containsKey(foregroundCacheKey)
if (!cacheKey.appInBackground && backgroundCallAlreadyInPlace) {
warnLog(NetworkStrings.SAME_CALL_SCHEDULED_WITH_JITTER.format(foregroundCacheKey))
remove(backgroundedCacheKey)?.takeIf { it.isNotEmpty() }?.let { backgroundedCallbacks ->
if (containsKey(cacheKey)) {
this[cacheKey]?.addAll(backgroundedCallbacks)
Comment on lines +532 to +540
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

} else {
this[cacheKey] = backgroundedCallbacks

Check warning on line 542 in purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt

View check run for this annotation

Codecov / codecov/patch

purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt#L542 

Added line #L542 was not covered by tests
}
}
}
}

private fun <K, S, E> MutableMap<K, MutableList<Pair<S, E>>>.addCallback(
call: Dispatcher.AsyncCall,
dispatcher: Dispatcher,
Expand All @@ -527,6 +562,11 @@
}
}

internal data class BackgroundAwareCallbackCacheKey(
val cacheKey: List<String>,
val appInBackground: Boolean,
)

internal fun PricingPhase.toMap(): Map<String, Any?> {
return mapOf(
"billingPeriod" to this.billingPeriod.iso8601,
Expand Down
3 changes: 3 additions & 0 deletions purchases/src/main/kotlin/com/revenuecat/purchases/common/BackendHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ internal class BackendHelper(
) {
internal val authenticationHeaders = mapOf("Authorization" to "Bearer ${this.apiKey}")

@Suppress("LongParameterList")
fun performRequest(
endpoint: Endpoint,
body: Map<String, Any?>?,
postFieldsToSign: List<Pair<String, String>>?,
delay: Delay,
onError: (PurchasesError) -> Unit,
onCompleted: (PurchasesError?, Int, JSONObject) -> Unit,
) {
Expand Down Expand Up @@ -47,6 +49,7 @@ internal class BackendHelper(
}
},
dispatcher,
delay,
)
}

Expand Down
6 changes: 4 additions & 2 deletions purchases/src/main/kotlin/com/revenuecat/purchases/common/offerings/OfferingsManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,12 @@ internal class OfferingsManager(
onError: ((PurchasesError) -> Unit)? = null,
onSuccess: ((Offerings) -> Unit)? = null,
) {
offeringsCache.setOfferingsCacheTimestampToNow()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall exactly why this was here before 🤔 But I remember there was a reason

Brain dump:

Before your changes, if there was another request to fetch offerings, the request considers the cache to be recent and it would return that outdated offerings cache instead, but that's incorrect right? I think that's what you optimized.

Also, if there was an issue getting the offerings, we would clear that newly set timestamp, and the following request would retry, with your changes, the behavior is the same

Anyways, I think the optimization makes sense, I am just trying to remember why this was done that way lol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there was another request to fetch offerings, the request considers the cache to be recent and it would return that outdated offerings cache instead, but that's incorrect right?

Mostly yes. However, the scenario I was trying to fix was the following:

  • User calls getOfferings BEFOREactivityonStart`. We mark timestamp of the cache to now before the requests starts
  • Activity onStart happens, however, the offerings cache already has a recent timestamp so we don't even try to get it (since it happens in the background, we don't expect a result, so we don't even check if there is a cached value or not).
  • We remain only with the request that had the jittering, rendering the main changes in this PR useless.

Also, if there was an issue getting the offerings, we would clear that newly set timestamp, and the following request would retry, with your changes, the behavior is the same

That's correct, and that remains that way for now, though it probably wouldn't be needed now. I've just made it so marking the offering cache timestamp happens only after a success.

The main consequence of this change is that multiple offerings request may happen before we receive a successful response, however, we rely on the request callback cache in the Backend to avoid duplicate requests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a recent conversation about this on iOS as well. Using the "set timestamp to now" workaround is effective but definitely not intuitive and confusing for maintainers.

I think the ultimate solution to it is to have 2 values - "last updated" and "is currently updating", and use the combination of those 2 as a more expressive way of determining state. We're currently encoding both of those into a single boolean value.

The cache key came later, though, so maybe it removes the need for that workaround entirely? Does it de-duplicate requests with the same key? If so it effectively removes the need for the "set timestamp to now" workaround and even having an "is currently updating" value as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache key came later, though, so maybe it removes the need for that workaround entirely? Does it de-duplicate requests with the same key?

Yes, the callback cache mechanism deduplicates requests while the request "is currently updating". However, we still need to store the last time the cache was updated to know whether it's stale, so we still need to store the timestamp on a success.

We had a recent conversation about this on iOS as well.

Yes, iOS did a similar change not too long ago and it relies on the request cache as well. I think that makes the most sense to handle deduplicating requests while it's in progress.

backend.getOfferings(
appUserID,
appInBackground,
{ createAndCacheOfferings(it, onError, onSuccess) },
{
createAndCacheOfferings(it, onError, onSuccess)
},
{ backendError, isServerError ->
if (isServerError) {
val cachedOfferingsResponse = offeringsCache.cachedOfferingsResponse
Expand Down Expand Up @@ -94,6 +95,7 @@ internal class OfferingsManager(
},
onSuccess = { offerings ->
offeringsCache.cacheOfferings(offerings, offeringsJSON)
offeringsCache.setOfferingsCacheTimestampToNow()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another optimization I had to do... If we left it where it was before, we wouldn't use the new mechanisms to optimize the requests trying to use the request that is supposed to finish earlier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these two lines should just be a part of cacheOfferings in offeringsCache and guarantee atomicity there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like it'd be conceptually cleaner and slightly easier to make it synchronized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly, these 2 could be joined (this is the only place they are both called) I will do so in a different PR.

dispatch {
onSuccess?.invoke(offerings)
}
Expand Down
4 changes: 4 additions & 0 deletions purchases/src/main/kotlin/com/revenuecat/purchases/strings/NetworkStrings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ internal object NetworkStrings {
"Retrying call with a new ETag"
const val ETAG_CALL_ALREADY_RETRIED = "We can't find the cached response, but call has already been retried. " +
"Returning result from backend: %s"
const val SAME_CALL_SCHEDULED_WITHOUT_JITTER = "Request already scheduled without jitter delay, adding " +
"callbacks to unjittered request with key: %s"
const val SAME_CALL_SCHEDULED_WITH_JITTER = "Request already scheduled with jitter delay, adding existing " +
"callbacks to unjittered request with key: %s"
const val SAME_CALL_ALREADY_IN_PROGRESS = "Same call already in progress, adding to callbacks map with key: %s"
const val PROBLEM_CONNECTING = "Unable to start a network connection due to a network configuration issue: %s"
const val VERIFICATION_MISSING_SIGNATURE = "Verification: Request to '%s' requires a signature but none provided."
Expand Down
2 changes: 2 additions & 0 deletions ...c/main/kotlin/com/revenuecat/purchases/subscriberattributes/SubscriberAttributesPoster.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.revenuecat.purchases.subscriberattributes
import com.revenuecat.purchases.PurchasesError
import com.revenuecat.purchases.PurchasesErrorCode
import com.revenuecat.purchases.common.BackendHelper
import com.revenuecat.purchases.common.Delay
import com.revenuecat.purchases.common.SubscriberAttributeError
import com.revenuecat.purchases.common.networking.Endpoint
import com.revenuecat.purchases.common.networking.RCHTTPStatusCodes
Expand All @@ -25,6 +26,7 @@ internal class SubscriberAttributesPoster(
Endpoint.PostAttributes(appUserID),
mapOf("attributes" to attributes),
postFieldsToSign = null,
delay = Delay.DEFAULT,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another change that I thought was fine but lmk if you have any concerns... Basically, this will add jittering to the subscriber attributes requests, which didn't exist before. I don't think it's critical so it made sense to me to delay it in favor of other requests.

{ error ->
onErrorHandler(error, false, emptyList())
},
Expand Down
115 changes: 112 additions & 3 deletions purchases/src/test/java/com/revenuecat/purchases/common/BackendTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import io.mockk.every
import io.mockk.mockk
import io.mockk.mockkObject
import io.mockk.slot
import io.mockk.spyk
import io.mockk.unmockkObject
import io.mockk.verify
import org.assertj.core.api.Assertions.assertThat
Expand Down Expand Up @@ -87,7 +88,7 @@ class BackendTest {
every { diagnosticsURL } returns mockDiagnosticsBaseURL
every { customEntitlementComputation } returns false
}
private val dispatcher = SyncDispatcher()
private val dispatcher = spyk(SyncDispatcher())
private val backendHelper = BackendHelper(API_KEY, dispatcher, mockAppConfig, mockClient)
private var backend: Backend = Backend(
mockAppConfig,
Expand All @@ -96,15 +97,15 @@ class BackendTest {
mockClient,
backendHelper
)
private val asyncDispatcher = Dispatcher(
private val asyncDispatcher = spyk(Dispatcher(
ThreadPoolExecutor(
1,
2,
0,
TimeUnit.MILLISECONDS,
LinkedBlockingQueue()
)
)
))
private var asyncBackendHelper: BackendHelper = BackendHelper(API_KEY, asyncDispatcher, mockAppConfig, mockClient)
private var asyncBackend: Backend = Backend(
mockAppConfig,
Expand Down Expand Up @@ -335,6 +336,60 @@ class BackendTest {
}
}

@Test
fun `given getCustomerInfo call on foreground, then one in background, only one request without delay is triggered`() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to do this because it'd be tricky to cancel the existing request for the background, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in this case I think it makes sense to reuse the existing callback cache, since we would get the result earlier without the jittering.

On the opposite side though (first request in background, then one in foreground), ideally we would cancel the background request, but cancelling exising requests would be very tricky yeah. However, since I think this case shouldn't be that common, I think it's better to just leave the request even if it's duplicated.

mockResponse(
Endpoint.GetCustomerInfo(appUserID),
null,
200,
null,
null,
true
)
val lock = CountDownLatch(2)
asyncBackend.getCustomerInfo(appUserID, appInBackground = false, onSuccess = {
lock.countDown()
}, onError = onReceiveCustomerInfoErrorHandler)
asyncBackend.getCustomerInfo(appUserID, appInBackground = true, onSuccess = {
lock.countDown()
}, onError = onReceiveCustomerInfoErrorHandler)
lock.await(defaultTimeout, TimeUnit.MILLISECONDS)
assertThat(lock.count).isEqualTo(0)
verify(exactly = 1) {
asyncDispatcher.enqueue(any(), Delay.NONE)
}
}

@Test
fun `given getCustomerInfo call on background, then one in foreground, both are executed`() {
mockResponse(
Endpoint.GetCustomerInfo(appUserID),
null,
200,
null,
null,
true
)
val lock = CountDownLatch(2)
asyncBackend.getCustomerInfo(appUserID, appInBackground = true, onSuccess = {
lock.countDown()
}, onError = onReceiveCustomerInfoErrorHandler)
asyncBackend.getCustomerInfo(appUserID, appInBackground = false, onSuccess = {
lock.countDown()
}, onError = onReceiveCustomerInfoErrorHandler)
lock.await(defaultTimeout, TimeUnit.MILLISECONDS)
assertThat(lock.count).isEqualTo(0)
verify(exactly = 2) {
mockClient.performRequest(
mockBaseURL,
Endpoint.GetCustomerInfo(appUserID),
body = null,
postFieldsToSign = null,
any()
)
}
}

@Test
fun `customer info call is enqueued with delay if on background`() {
dispatcher.calledDelay = null
Expand Down Expand Up @@ -1322,6 +1377,60 @@ class BackendTest {
}
}

@Test
fun `given getOfferings call on foreground, then one in background, only one request without delay is triggered`() {
mockResponse(
Endpoint.GetOfferings(appUserID),
null,
200,
null,
noOfferingsResponse,
true
)
val lock = CountDownLatch(2)
asyncBackend.getOfferings(appUserID, appInBackground = false, onSuccess = {
lock.countDown()
}, onError = onReceiveOfferingsErrorHandler)
asyncBackend.getOfferings(appUserID, appInBackground = true, onSuccess = {
lock.countDown()
}, onError = onReceiveOfferingsErrorHandler)
lock.await(defaultTimeout, TimeUnit.MILLISECONDS)
assertThat(lock.count).isEqualTo(0)
verify(exactly = 1) {
asyncDispatcher.enqueue(any(), Delay.NONE)
}
}

@Test
fun `given getOfferings call on background, then one in foreground, both are executed`() {
mockResponse(
Endpoint.GetOfferings(appUserID),
null,
200,
null,
noOfferingsResponse,
true
)
val lock = CountDownLatch(2)
asyncBackend.getOfferings(appUserID, appInBackground = true, onSuccess = {
lock.countDown()
}, onError = onReceiveOfferingsErrorHandler)
asyncBackend.getOfferings(appUserID, appInBackground = false, onSuccess = {
lock.countDown()
}, onError = onReceiveOfferingsErrorHandler)
lock.await(defaultTimeout, TimeUnit.MILLISECONDS)
assertThat(lock.count).isEqualTo(0)
verify(exactly = 2) {
mockClient.performRequest(
mockBaseURL,
Endpoint.GetOfferings(appUserID),
body = null,
postFieldsToSign = null,
any()
)
}
}

@Test
fun `offerings call is enqueued with delay if on background`() {
mockResponse(Endpoint.GetOfferings(appUserID), null, 200, null, noOfferingsResponse)
Expand Down
Loading