Configuration Cleanup

[jaxdesmarais]

Summary of changes

• Cleanup comments left post merge from Sammy in PR: Fix ConfigurationCallbackStorage Crash #1385

Checklist

• [ ] Added a changelog entry

Authors

• @jaxdesmarais

[jaxdesmarais]

@KunJeongPark would love your thoughts on these changes to make sure I didn't miss anything! I went through several tests with triggering a memory warning, but would love a second set of eyes if you're able to pull down this branch and run through some tests. :teamwork:

[KunJeongPark]

I think Sammy also had concerns whether @mainactor annotations were necessary in two places.

[jaxdesmarais]

I think Sammy also had concerns whether @mainactor annotations were necessary in two places.

I addressed those in the original PR: #1385 (comment)

[jaxdesmarais]

I tested 6.23.3 with @mainactor annotation as it is currently on main wit no changes, 6.23.3 without @mainactor annotation but with await MainActor.run on setHTTPCredentials and completion calls and 6.23.2 with no changes.

Both Task { @MainActor in and MainActor.run around the completion/setupHTTPCredentials do the exact same thing so these will return the same results. I don't think it's worth overcomplicating the callsite as this method will also soon be removed with DTMOBILES-980. Additionally since both behave the same we can keep the callsite simple for now until we move to only using the async config call.

I found that there are no duplicate calls to v1/configuration in any of them, they are all fetching from cache

You'll need to ensure you're letting the cache clear, it's valid for 5 minutes, but first fetch will be false and subsequent will be true. In any case moving to a singular fetch config method will remove this as a grey area if you're unable to replicate due to setup issues.

[KunJeongPark]

Both Task { @MainActor in and MainActor.run around the completion/setupHTTPCredentials do the exact same thing so these will return the same results. I don't think it's worth overcomplicating the callsite as this method will also soon be removed with DTMOBILES-980. Additionally since both behave the same we can keep the callsite simple for now until we move to only using the async config call.

I'm fine with it, but I don't think they're same and we should keep an eye on latency, although I don't think it will be noticeable.

I found that there are no duplicate calls to v1/configuration in any of them, they are all fetching from cache

You'll need to ensure you're letting the cache clear, it's valid for 5 minutes, but first fetch will be false and subsequent will be true. In any case moving to a singular fetch config method will remove this as a grey area if you're unable to replicate due to setup issues.

I did try it, clearing cache and I got same result. I made note on ticket about low memory setting and NSCache.
I'm good to ✅ this ticket after running one test. Thank you for the clean up!

As I mentioned in the ticket, I just wanted to make sure that there were no inherent problems elsewhere without the @mainactor annotation, if that annotation is masking something, if that makes sense. Not holding this one back.

[KunJeongPark]

Thank you for the work! Tests look good! 🎉
Did few more tests with low memory, looks good.

[agedd]

🚀