paypal / paypal-ios

One merchant integration point for all of PayPal's services
Apache License 2.0
59 stars 27 forks source link

change on-disk to in-memory cache HTTP performRequest #154

Closed KunJeongPark closed 1 year ago

KunJeongPark commented 1 year ago

Reason for changes

-clientID should not persist on disk in between app sessions.

Summary of changes

Checklist

~- [ ] Added a changelog entry~

Authors

@KunJeongPark @jcnoriega

scannillo commented 1 year ago

@KunJeongPark @jaxdesmarais Ah, you know what - we have to revisit the changes made in this PR.

Prior, we were using URLCache.shared which used a singleton cache instance throughout the SDK. But now, we have a singular cache instances every time a client is created. This really defeats the point of a shared cache b/w clients for the purpose of caching clientID for analytics.

jaxdesmarais commented 1 year ago

Ah, great catch @scannillo!

I think we can set the shared instance to our custom cache with something like:

let sharedCache = URLCache(memoryCapacity: 4 * 1024 * 1024, diskCapacity: 0)
URLCache.shared = sharedCache

// OR

URLCache.shared = {
    URLCache(memoryCapacity: 4 * 1024 * 1024, diskCapacity: 0)
}()

Not sure if that'd have the same problem but probably worth doing some testing

scannillo commented 1 year ago

That's a really cool idea! I wonder though if a merchant uses URLCache.shared on their own for other parts of their app, we would be changing behavior they may rely on (such as cache size)?

(Also potentially wiping it when our line of code is executed)

jaxdesmarais commented 1 year ago

Yeah it looks like whatever class is called first overrides the values of URLCache.shared. Here is what I threw in a playground:

class CacheOne {
    func accessCache() {
        URLCache.shared = {
            URLCache(memoryCapacity: 4 * 1024 * 1024, diskCapacity: 0)
        }()

        print(URLCache.shared.diskCapacity)
    }
}

class CacheTwo {
    func accessCache() {
        let cacheTwo = URLCache.shared
        print(cacheTwo.diskCapacity)
    }
}

let cacheOne = CacheOne()
let cacheTwo = CacheTwo()

cacheOne.accessCache()
cacheTwo.accessCache()

The above always sets the diskCapacity to 0 for both classes. If you swap the last 2 methods being called it takes the first one then sets it to 0. But we can't really rely on merchants to always call their reference to URLCache.shared before us.

KunJeongPark commented 1 year ago

I just skimmed this, I need to read more carefully but I meant to ask, how big a hit is it to not cache clientID, but hit the network request every time.

jaxdesmarais commented 1 year ago

@KunJeongPark we will want to cache the client ID. For smaller merchants, not a big increase but even then if we send 2-3 analytics events per requests, that'd be 2-3 extra network requests. But if we are talking about larger merchants (Uber, DoorDash, etc size - eventually) we'd be looking at thousands of network requests for analytics a minute - which would be a lot.

KunJeongPark commented 1 year ago

So use custom cache that is a singleton?

jaxdesmarais commented 1 year ago

We don't want to set URLCache.shared to a custom cache. This would mean that if a merchant is using URLCache.shared they would instead use our memory only cache when they may want to use disk cache. If we want to use a memory only cache, we should likely create our own singleton instance to be used across the SDK instead of using URLCache.shared. Up to you if you just want to revert this PR as a quick fix or open a new PR to use our own singleton until we move from access tokens to client IDs.

This will all soon be a non issue once we move from access tokens to client IDs as we will no longer need to fetch (or store in memory) client tokens. Merchants will pass them to us directly so we will have access to them anywhere we need a client ID vs storing them in a cache.