Skip to content

BasePurchasesTests: added assertion to ensure Purchases does not leak#2104

Merged
NachoSoto merged 2 commits into
mainfrom
purchase-leaking-test
Dec 9, 2022
Merged

BasePurchasesTests: added assertion to ensure Purchases does not leak#2104
NachoSoto merged 2 commits into
mainfrom
purchase-leaking-test

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

To continue debugging CSDK-517, this adds an assertion to make sure that we don't leak Purchases, either because of a retain cycle, or due to any XCTest issue.

@NachoSoto NachoSoto added the test label Nov 30, 2022
@NachoSoto NachoSoto requested a review from a team November 30, 2022 00:07
NachoSoto added a commit that referenced this pull request Nov 30, 2022
Follow up to #2104 and #2105. This ensures that `Purchases` is re-created for each test.

self.deviceCache = nil
self.purchases = nil
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Smart!

…leak

To continue debugging [CSDK-517], this adds an assertion to make sure that we don't leak `Purchases`, either because of a retain cycle, or due to any `XCTest` issue.
@NachoSoto NachoSoto force-pushed the purchase-leaking-test branch from ec006ee to 425de93 Compare December 9, 2022 01:24
@NachoSoto NachoSoto enabled auto-merge (squash) December 9, 2022 01:24
@NachoSoto NachoSoto merged commit 1d3e7cb into main Dec 9, 2022
@NachoSoto NachoSoto deleted the purchase-leaking-test branch December 9, 2022 01:36
NachoSoto added a commit that referenced this pull request Dec 12, 2022
…2105)

Follow up to #2104. See also #2101. This has uncovered a huge source of
leaks across many tests. Fixing this provides many benefits:

- Faster test runs
- Lower memory overhead
- Isolated state across each test run
- Ensure that these types don't leak outside of tests too
NachoSoto added a commit that referenced this pull request May 23, 2023
While working on #2533, I knew that my proof of concept had a retain cycle. I however was very confused when it wasn't detected by this mechanism introduced first introduced in #2104.

Turns out that this had been wrong the whole time: the current implementation, as explained by the comment, was executing those blocks in LIFO order, which meant that the references were being set to `nil` before the assertions could be created.
This meant that by the time the `except { [weak purchases = self.purchases] ... }` lines were called, `self.purchases` itself was `nil`, so nothing was being checked.

This simplifies the implementation by inlining the checks, and saving a `weak` reference before clearing the singleton and all the local references.

Luckily there was only one failing test that had to do with how `Purchases` was being created, and implementation details of `expectFatalError`. To avoid this, I changed the test to use the `Purchases.init` method directly.
NachoSoto added a commit that referenced this pull request May 24, 2023
While working on #2533, I knew that my proof of concept had a retain
cycle. I however was very confused when it wasn't detected by this
mechanism introduced first introduced in #2104.

Turns out that this had been wrong the whole time: the current
implementation, as explained by the comment, was executing those blocks
in LIFO order, which meant that the references were being set to `nil`
before the assertions could be created.
This meant that by the time the `except { [weak purchases =
self.purchases] ... }` lines were called, `self.purchases` itself was
`nil`, so nothing was being checked.

This simplifies the implementation by inlining the checks, and saving a
`weak` reference before clearing the singleton and all the local
references.

Luckily there was only one failing test that had to do with how
`Purchases` was being created, and implementation details of
`expectFatalError`. To avoid this, I changed the test to use the
`Purchases.init` method directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants