[EXTERNAL] Lock RateLimiter.shouldProceed() entirely to avoid race conditions (#4635) via @nguyenhuy

[JayShortway]

Checklist

• If applicable, unit tests
• If applicable, create follow-up issues for purchases-android and hybrids

Motivation

Introduced in #3709, RateLimiter is used exclusively by Purchases.syncAttributesAndOfferingsIfNeeded(completion:). The documentation of this method doesn't mention any threading requirement, so it's reasonable to assume that it can be called on any thread. This means RateLimiter needs to be thread safe, otherwise clients will run into race conditions as reported in #4440.

Description

Fortunately the RateLimiter class is quite simple.

Its contants don't need to be atomic/locked since they are thread safe by definition. Turning timestamps and index vars into atomics doesn't help much in this case because shouldProceed() needs multiple reads and writes in a "transaction".

Using a Lock to wrap the whole method is the simplest and self-contained approach so I picked it. Android is doing the same thing (here).

The lock is a non-recursive lock because there is no need for reentrant. And in principle I avoid recursive locks as much as possible since one can easily lose control over the locks, especially in more complicated use cases/codebases. This is a big problem that we have had for a long time in AsyncDisplayKit/Texture.

Unit tests remain the same because testing for race conditions is rather difficult and unpredictable. Texture has a bunch of thrash tests that kind of work but not 100%. Since the subject under test in this case is rather simple and the fix is straightforward it should work. Otherwise, we can have follow up PRs for both iOS and Android.

This should resolve #4440.

This PR also renames RateLimiterRests.swift to RateLimiterTests.swift.

Contribution

Original PR #4635 by @nguyenhuy

[nguyenhuy]

Linter failed:

/Users/distiller/purchases-ios/Sources/Misc/RateLimiter.swift
Formatting Test
Lines should not have trailing whitespace
Error:Line:20

Let me run the linter locally and report back if anything else needs to be fixed.

[nguyenhuy]

Confirmed that that's the only linting issue.

Sorry I didn't find it earlier. Didn't notice that fastlane setup_dev failed so the pre-commit git hook wasn't installed correctly. Ended up manually running ./scripts/pre-commit.sh on staged files myself to verify :)

[JayShortway]

@nguyenhuy That's alright, no worries! It's the nature of the system where CI doesn't run on forks (for good reasons). Thanks for suggesting the fix!

[joshdholtz]

Glad to have one less potential bug written by me in this codebase 🙃