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.

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

Tests: avoid race-condition in leak detection #2806

Merged
merged 1 commit into from
Jul 13, 2023
Merged

Conversation

NachoSoto
Copy link
Contributor

Fixes https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/12839/workflows/2b1e24b9-428a-40ed-bcfd-a94d7591236d/jobs/89720/steps

I noticed when downloading the .xcresult that the tests had crashed with this:

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libswiftCore.dylib            	       0x10bc64589 swift::runtime::AccessSet::insert(swift::runtime::Access*, void*, void*, swift::ExclusivityFlags) + 73
1   libswiftCore.dylib            	       0x10bc647e2 swift_beginAccess + 66
2   BackendIntegrationTests       	       0x13ab37c76 default argument 1 of Expectation.toEventually(_:timeout:pollInterval:description:) + 54
3   BackendIntegrationTests       	       0x13ab37b44 closure #1 in BaseBackendIntegrationTests.verifyPurchasesDoesNotLeak() + 276 (BaseBackendIntegrationTests.swift:167)

Turns out that because of the way it was written, normally Nimble tries to print the instance during a failure:

BasePurchasesTests.swift:116: error: -[UnitTests.PurchasesConfiguringTests testSettingTheDelegateAfterInitializationSendsCachedCustomerInfo] : failed - Purchases has leaked
expected to eventually be nil, got <<RCPurchases: 0x125d13bf0>>

That crash points to a race condition inside Swift's runtime. To avoid that, this changes the expectation so Nimble would only try to print "expected true, got false", with our own provided description. Therefore making sure that it doesn't try to access the instance while it's being deallocated.

@NachoSoto NachoSoto added the test label Jul 13, 2023
@NachoSoto NachoSoto requested a review from a team July 13, 2023 15:07
expect(orchestrator)
.toEventually(beNil(), description: "PurchasesOrchestrator has leaked")
expect(deviceCache)
.toEventually(beNil(), description: "DeviceCache has leaked: \(self)")
Copy link
Contributor Author

@NachoSoto NachoSoto Jul 13, 2023

Choose a reason for hiding this comment

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

Bonus points for also not capturing self here.

// Note: this captures the boolean to avoid race conditions when Nimble tries
// to print the instances while they're being deallocated.
Copy link
Member

Choose a reason for hiding this comment

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

didn't know it was possible to capture this as a boolean, I thought this block would capture strongly instead. How does this work? like, doesn't it need the object in order to reference when re-evaluating?

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 references are weak though (defined above). The previous implementation used an @autoclosure, so expect(purchases) was converted to expect { purchases }.
Internally it was using reflection (possibly String.init(describing:)) to print a value for that object.

Now when it generates a description from the subject of the expectation, it's just a boolean.

Let me know if that makes sense.

Base automatically changed from revery-leak-detection to main July 13, 2023 19:42
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #2806 (7bc5165) into main (1c3bed0) will decrease coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 7bc5165 differs from pull request most recent head e14d7d2. Consider uploading reports for the commit e14d7d2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2806      +/-   ##
==========================================
- Coverage   86.56%   86.51%   -0.05%     
==========================================
  Files         215      215              
  Lines       15484    15472      -12     
==========================================
- Hits        13403    13386      -17     
- Misses       2081     2086       +5     

see 6 files with indirect coverage changes

Fixes https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/12839/workflows/2b1e24b9-428a-40ed-bcfd-a94d7591236d/jobs/89720/steps

I noticed when downloading the `.xcresult` that the tests had crashed with this:
```
Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libswiftCore.dylib            	       0x10bc64589 swift::runtime::AccessSet::insert(swift::runtime::Access*, void*, void*, swift::ExclusivityFlags) + 73
1   libswiftCore.dylib            	       0x10bc647e2 swift_beginAccess + 66
2   BackendIntegrationTests       	       0x13ab37c76 default argument 1 of Expectation.toEventually(_:timeout:pollInterval:description:) + 54
3   BackendIntegrationTests       	       0x13ab37b44 closure #1 in BaseBackendIntegrationTests.verifyPurchasesDoesNotLeak() + 276 (BaseBackendIntegrationTests.swift:167)
```

Turns out that because of the way it was written, normally `Nimble` tries to print the instance during a failure:
```
BasePurchasesTests.swift:116: error: -[UnitTests.PurchasesConfiguringTests testSettingTheDelegateAfterInitializationSendsCachedCustomerInfo] : failed - Purchases has leaked
expected to eventually be nil, got <<RCPurchases: 0x125d13bf0>>
```

That crash points to a race condition inside Swift's runtime. To avoid that, this changes the expectation so `Nimble` would only try to print "expected true, got false", with our own provided description.
Therefore making sure that it doesn't try to access the instance while it's being deallocated.
@NachoSoto NachoSoto enabled auto-merge (squash) July 13, 2023 19:57
@NachoSoto NachoSoto merged commit e2ecdd0 into main Jul 13, 2023
@NachoSoto NachoSoto deleted the leak-race-condition branch July 13, 2023 21:40
NachoSoto added a commit that referenced this pull request Jul 17, 2023
Looks like #2806 didn't work.
I still see this race-condition crash:
```
Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libswiftCore.dylib            	       0x10bc64589 swift::runtime::AccessSet::insert(swift::runtime::Access*, void*, void*, swift::ExclusivityFlags) + 73
1   libswiftCore.dylib            	       0x10bc647e2 swift_beginAccess + 66
2   BackendIntegrationTests       	       0x13ab37c76 default argument 1 of Expectation.toEventually(_:timeout:pollInterval:description:) + 54
3   BackendIntegrationTests       	       0x13ab37b44 closure #1 in BaseBackendIntegrationTests.verifyPurchasesDoesNotLeak() + 276 (BaseBackendIntegrationTests.swift:167)
```

This slight refactor matches the implementation in `BasePurchasesTests`, and I haven't the crash there. So hopefully this will work.
NachoSoto added a commit that referenced this pull request Jul 18, 2023
Looks like #2806 didn't work.
I still see this race-condition crash:
```
Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libswiftCore.dylib            	       0x10bc64589 swift::runtime::AccessSet::insert(swift::runtime::Access*, void*, void*, swift::ExclusivityFlags) + 73
1   libswiftCore.dylib            	       0x10bc647e2 swift_beginAccess + 66
2   BackendIntegrationTests       	       0x13ab37c76 default argument 1 of Expectation.toEventually(_:timeout:pollInterval:description:) + 54
3   BackendIntegrationTests       	       0x13ab37b44 closure #1 in BaseBackendIntegrationTests.verifyPurchasesDoesNotLeak() + 276 (BaseBackendIntegrationTests.swift:167)
```

This slight refactor matches the implementation in `BasePurchasesTests`,
and I haven't the crash there. So hopefully this will work.

I filed swiftlang/swift#67361 with the crash.
NachoSoto added a commit that referenced this pull request Jul 19, 2023
**This is an automatic release.**

### Dependency Updates
* Bump fastlane from 2.213.0 to 2.214.0 (#2824) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* `MainThreadMonitor`: don't crash if there is no test in progress
(#2838) via NachoSoto (@NachoSoto)
* `CI`: fixed Fastlane APITester lanes (#2836) via NachoSoto
(@NachoSoto)
* `Integration Tests`: workaround Swift runtime crash (#2826) via
NachoSoto (@NachoSoto)
* `@EnsureNonEmptyArrayDecodable` (#2831) via NachoSoto (@NachoSoto)
* `iOS 17`: added tests for simulating cancellations (#2597) via
NachoSoto (@NachoSoto)
* `CI`: make all `Codecov` jobs `informational` (#2828) via NachoSoto
(@NachoSoto)
* `MainThreadMonitor`: check deadlocks only ever N seconds (#2820) via
NachoSoto (@NachoSoto)
* New `@NonEmptyStringDecodable` (#2819) via NachoSoto (@NachoSoto)
* `MockDeviceCache`: avoid using real `UserDefaults` (#2814) via
NachoSoto (@NachoSoto)
* `throwAssertion`: fixed Xcode 15 compilation (#2813) via NachoSoto
(@NachoSoto)
* `CustomEntitlementsComputation`: fixed API testers (#2815) via
NachoSoto (@NachoSoto)
* `PackageTypeTests`: fixed iOS 12 (#2807) via NachoSoto (@NachoSoto)
* `Tests`: avoid race-condition in leak detection (#2806) via NachoSoto
(@NachoSoto)
* Revert "`Unit Tests`: removed leak detection" (#2805) via NachoSoto
(@NachoSoto)
* `PackageType: Codable` implementation (#2797) via NachoSoto
(@NachoSoto)
* `SystemInfo.init` no longer `throws` (#2803) via NachoSoto
(@NachoSoto)
* `Trusted Entitlements`: add support for signing `POST` body (#2753)
via NachoSoto (@NachoSoto)
* `Tests`: unified default timeouts (#2801) via NachoSoto (@NachoSoto)
* `Tests`: removed forced-unwrap (#2799) via NachoSoto (@NachoSoto)
* `Tests`: added missing `super.setUp()` (#2804) via NachoSoto
(@NachoSoto)
* Replaced `FatalErrorUtil` with `Nimble` (#2802) via NachoSoto
(@NachoSoto)
* `Tests`: fixed another flaky test (#2795) via NachoSoto (@NachoSoto)
* `TimingUtil`: improved tests by using `Clock` (#2794) via NachoSoto
(@NachoSoto)
* `IgnoreDecodeErrors`: log decoding error (#2778) via NachoSoto
(@NachoSoto)
* `TestLogHandler`: changed all tests to explicitly deinitialize it
(#2784) via NachoSoto (@NachoSoto)
* `LocalReceiptParserStoreKitTests`: fixed flaky test failure (#2785)
via NachoSoto (@NachoSoto)
* `Unit Tests`: removed leak detection (#2792) via NachoSoto
(@NachoSoto)
* `Tests`: fixed another flaky failure with asynchronous check (#2786)
via NachoSoto (@NachoSoto)

---------

Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
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.

3 participants