From 34ef0d4783cc8d0fe8eeb8c561c527c9aeadeae0 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 11:17:34 +0200 Subject: [PATCH 1/9] feat: add failing test for ProductInfoController.inflightRequest - crash --- .../ProductsInfoControllerTests.swift | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift index 64a92106..637b2ebc 100644 --- a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift +++ b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift @@ -23,6 +23,7 @@ // THE SOFTWARE. import XCTest +import Foundation @testable import SwiftyStoreKit class TestInAppProductRequest: InAppProductRequest { @@ -50,8 +51,14 @@ class TestInAppProductRequest: InAppProductRequest { class TestInAppProductRequestBuilder: InAppProductRequestBuilder { var requests: [ TestInAppProductRequest ] = [] + var os_unfair_lock_s = os_unfair_lock() func request(productIds: Set, callback: @escaping InAppProductRequestCallback) -> InAppProductRequest { + os_unfair_lock_lock(&self.os_unfair_lock_s) + defer { + os_unfair_lock_unlock(&self.os_unfair_lock_s) + } + let request = TestInAppProductRequest(productIds: productIds, callback: callback) requests.append(request) return request @@ -68,6 +75,36 @@ class TestInAppProductRequestBuilder: InAppProductRequestBuilder { class ProductsInfoControllerTests: XCTestCase { let sampleProductIdentifiers: Set = ["com.iap.purchase1"] + let testProducts: Set = ["com.iap.purchase01", + "com.iap.purchase02", + "com.iap.purchase03", + "com.iap.purchase04", + "com.iap.purchase05", + "com.iap.purchase06", + "com.iap.purchase07", + "com.iap.purchase08", + "com.iap.purchase09", + "com.iap.purchase10", + "com.iap.purchase11", + "com.iap.purchase12", + "com.iap.purchase13", + "com.iap.purchase14", + "com.iap.purchase15", + "com.iap.purchase16", + "com.iap.purchase17", + "com.iap.purchase18", + "com.iap.purchase19", + "com.iap.purchase20", + "com.iap.purchase21", + "com.iap.purchase22", + "com.iap.purchase23", + "com.iap.purchase24", + "com.iap.purchase25", + "com.iap.purchase26", + "com.iap.purchase27", + "com.iap.purchase28", + "com.iap.purchase29", + "com.iap.purchase30",] func testRetrieveProductsInfo_when_calledOnce_then_completionCalledOnce() { @@ -117,4 +154,30 @@ class ProductsInfoControllerTests: XCTestCase { requestBuilder.fireCallbacks() XCTAssertEqual(completionCount, 2) } + + func testRetrieveProductsInfo_when_calledConcurrentlyInDifferentThreads_then_eachcompletionCalledOnce_noCrashes() { + let requestBuilder = TestInAppProductRequestBuilder() + let productInfoController = ProductsInfoController(inAppProductRequestBuilder: requestBuilder) + + var completionCalledSet: Set = [] + + let expectation = XCTestExpectation(description: "Expect downloads of product informations") + let group = DispatchGroup() + + for product in testProducts { + DispatchQueue.global().async { + group.enter() + productInfoController.retrieveProductsInfo([product]) { _ in + completionCalledSet.insert(product) + group.leave() + } + } + } + + group.notify(queue: DispatchQueue.global()) { + expectation.fulfill() + } + + XCTAssertEqual(completionCalledSet, testProducts) + } } From 511f4b1d45f6727c7f85dc58fc87b25bd60c4dd9 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 11:20:59 +0200 Subject: [PATCH 2/9] fix: reduce the amount of products --- .../ProductsInfoControllerTests.swift | 22 +------------------ 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift index 637b2ebc..ac8131e1 100644 --- a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift +++ b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift @@ -84,27 +84,7 @@ class ProductsInfoControllerTests: XCTestCase { "com.iap.purchase07", "com.iap.purchase08", "com.iap.purchase09", - "com.iap.purchase10", - "com.iap.purchase11", - "com.iap.purchase12", - "com.iap.purchase13", - "com.iap.purchase14", - "com.iap.purchase15", - "com.iap.purchase16", - "com.iap.purchase17", - "com.iap.purchase18", - "com.iap.purchase19", - "com.iap.purchase20", - "com.iap.purchase21", - "com.iap.purchase22", - "com.iap.purchase23", - "com.iap.purchase24", - "com.iap.purchase25", - "com.iap.purchase26", - "com.iap.purchase27", - "com.iap.purchase28", - "com.iap.purchase29", - "com.iap.purchase30",] + "com.iap.purchase10"] func testRetrieveProductsInfo_when_calledOnce_then_completionCalledOnce() { From 947382fcb91d136f0e5b59af012c4a30d335a805 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 11:21:13 +0200 Subject: [PATCH 3/9] docs: add comments to the test --- SwiftyStoreKitTests/ProductsInfoControllerTests.swift | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift index ac8131e1..77d5d7d2 100644 --- a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift +++ b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift @@ -54,6 +54,7 @@ class TestInAppProductRequestBuilder: InAppProductRequestBuilder { var os_unfair_lock_s = os_unfair_lock() func request(productIds: Set, callback: @escaping InAppProductRequestCallback) -> InAppProductRequest { + // add locks to make sure the test does not fail in preparation os_unfair_lock_lock(&self.os_unfair_lock_s) defer { os_unfair_lock_unlock(&self.os_unfair_lock_s) @@ -75,6 +76,7 @@ class TestInAppProductRequestBuilder: InAppProductRequestBuilder { class ProductsInfoControllerTests: XCTestCase { let sampleProductIdentifiers: Set = ["com.iap.purchase1"] + // Set of in app purchases to ask in different threads let testProducts: Set = ["com.iap.purchase01", "com.iap.purchase02", "com.iap.purchase03", @@ -141,9 +143,14 @@ class ProductsInfoControllerTests: XCTestCase { var completionCalledSet: Set = [] + // Create the expectation not to let the test finishes before the other threads complete let expectation = XCTestExpectation(description: "Expect downloads of product informations") + + // Create the dispatch group to let the test verifies the assert only when + // everything else finishes. let group = DispatchGroup() + // Dispatch a request for every product in a different thread for product in testProducts { DispatchQueue.global().async { group.enter() @@ -154,6 +161,7 @@ class ProductsInfoControllerTests: XCTestCase { } } + // Fullfil the expectation when every thread finishes group.notify(queue: DispatchQueue.global()) { expectation.fulfill() } From 76e83e0182fbda21f4619d83d41844ecfb0f3977 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 11:53:54 +0200 Subject: [PATCH 4/9] fix: make the test works properly, calling the fireCallbakcs method. --- .../ProductsInfoControllerTests.swift | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift index 77d5d7d2..0d356a34 100644 --- a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift +++ b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift @@ -141,7 +141,7 @@ class ProductsInfoControllerTests: XCTestCase { let requestBuilder = TestInAppProductRequestBuilder() let productInfoController = ProductsInfoController(inAppProductRequestBuilder: requestBuilder) - var completionCalledSet: Set = [] + var completionCallbackCount = 0 // Create the expectation not to let the test finishes before the other threads complete let expectation = XCTestExpectation(description: "Expect downloads of product informations") @@ -155,17 +155,21 @@ class ProductsInfoControllerTests: XCTestCase { DispatchQueue.global().async { group.enter() productInfoController.retrieveProductsInfo([product]) { _ in - completionCalledSet.insert(product) + completionCallbackCount += 1 group.leave() } } } - + DispatchQueue.global().asyncAfter(deadline: .now() + 1) { + requestBuilder.fireCallbacks() + } // Fullfil the expectation when every thread finishes group.notify(queue: DispatchQueue.global()) { + + XCTAssertEqual(completionCallbackCount, self.testProducts.count) expectation.fulfill() } - XCTAssertEqual(completionCalledSet, testProducts) + wait(for: [expectation], timeout: 10.0) } } From 72f88b03ca567399c1ee1360bb0f02a28ed74720 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 11:54:19 +0200 Subject: [PATCH 5/9] fix: implement OS agnostic locking. --- SwiftyStoreKit/ProductsInfoController.swift | 28 +++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/SwiftyStoreKit/ProductsInfoController.swift b/SwiftyStoreKit/ProductsInfoController.swift index cb5bff48..33ebbfe9 100644 --- a/SwiftyStoreKit/ProductsInfoController.swift +++ b/SwiftyStoreKit/ProductsInfoController.swift @@ -44,8 +44,18 @@ class ProductsInfoController: NSObject { } let inAppProductRequestBuilder: InAppProductRequestBuilder + + private var unfairLock: os_unfair_lock_s! + private var spinLock: OSSpinLock! + init(inAppProductRequestBuilder: InAppProductRequestBuilder = InAppProductQueryRequestBuilder()) { self.inAppProductRequestBuilder = inAppProductRequestBuilder + if #available(iOSApplicationExtension 10.0, *) { + self.unfairLock = os_unfair_lock() + } else { + self.spinLock = OSSpinLock() + } + super.init() } // As we can have multiple inflight requests, we store them in a dictionary by product ids @@ -72,4 +82,22 @@ class ProductsInfoController: NSObject { inflightRequests[productIds]!.completionHandlers.append(completion) } } + + + + private func lock() { + if #available(iOSApplicationExtension 10.0, *) { + os_unfair_lock_lock(&self.unfairLock) + } else { + OSSpinLockLock(&self.spinLock) + } + } + + private func unlock() { + if #available(iOSApplicationExtension 10.0, *) { + os_unfair_lock_unlock(&self.unfairLock) + } else { + OSSpinLockUnlock(&self.spinLock) + } + } } From d3ed588d9858fa11c652597cb712242e671e52cb Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 11:54:49 +0200 Subject: [PATCH 6/9] =?UTF-8?q?fix:=20lock=20the=20retrieveProductsInfo=20?= =?UTF-8?q?so=20that=20it=E2=80=99s=20thread=20safe.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- SwiftyStoreKit/ProductsInfoController.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/SwiftyStoreKit/ProductsInfoController.swift b/SwiftyStoreKit/ProductsInfoController.swift index 33ebbfe9..28853feb 100644 --- a/SwiftyStoreKit/ProductsInfoController.swift +++ b/SwiftyStoreKit/ProductsInfoController.swift @@ -62,10 +62,17 @@ class ProductsInfoController: NSObject { private var inflightRequests: [Set: InAppProductQuery] = [:] func retrieveProductsInfo(_ productIds: Set, completion: @escaping (RetrieveResults) -> Void) { - + self.lock() + defer { + self.unlock() + } if inflightRequests[productIds] == nil { let request = inAppProductRequestBuilder.request(productIds: productIds) { results in - + self.lock() + defer { + self.unlock() + } + if let query = self.inflightRequests[productIds] { for completion in query.completionHandlers { completion(results) From 23f0f2d724a010de0145adaf5c54bb2b6de790ee Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 13:47:00 +0200 Subject: [PATCH 7/9] docs: add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30a58b7c..865eb6df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ All notable changes to this project will be documented in this file. +* Make `ProductsInfoController`'s `retrieveProductsInfo` thread safe ([#405]https://github.com/bizz84/SwiftyStoreKit/pull/495), related issues: [#344](https://github.com/bizz84/SwiftyStoreKit/issues/344) and [#468](https://github.com/bizz84/SwiftyStoreKit/issues/468) + ## [0.15.0](https://github.com/bizz84/SwiftyStoreKit/releases/tag/0.15.0) Update project to Swift 5, Xcode 10.2 * Update project to Swift 5 ([#457](https://github.com/bizz84/SwiftyStoreKit/pull/457)), related issue: [#456](https://github.com/bizz84/SwiftyStoreKit/issues/456) From f5d9858bc22a6d80f29abd4b022e00e7831603cb Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 14:19:16 +0200 Subject: [PATCH 8/9] fix: remove api available only on macos and only from version 10 --- SwiftyStoreKit/ProductsInfoController.swift | 38 ++++----------------- 1 file changed, 7 insertions(+), 31 deletions(-) diff --git a/SwiftyStoreKit/ProductsInfoController.swift b/SwiftyStoreKit/ProductsInfoController.swift index 28853feb..7cf492af 100644 --- a/SwiftyStoreKit/ProductsInfoController.swift +++ b/SwiftyStoreKit/ProductsInfoController.swift @@ -44,33 +44,27 @@ class ProductsInfoController: NSObject { } let inAppProductRequestBuilder: InAppProductRequestBuilder - - private var unfairLock: os_unfair_lock_s! - private var spinLock: OSSpinLock! + + private var spinLock: OSSpinLock init(inAppProductRequestBuilder: InAppProductRequestBuilder = InAppProductQueryRequestBuilder()) { self.inAppProductRequestBuilder = inAppProductRequestBuilder - if #available(iOSApplicationExtension 10.0, *) { - self.unfairLock = os_unfair_lock() - } else { - self.spinLock = OSSpinLock() - } - super.init() + self.spinLock = OSSpinLock() } // As we can have multiple inflight requests, we store them in a dictionary by product ids private var inflightRequests: [Set: InAppProductQuery] = [:] func retrieveProductsInfo(_ productIds: Set, completion: @escaping (RetrieveResults) -> Void) { - self.lock() + OSSpinLockLock(&self.spinLock) defer { - self.unlock() + OSSpinLockUnlock(&self.spinLock) } if inflightRequests[productIds] == nil { let request = inAppProductRequestBuilder.request(productIds: productIds) { results in - self.lock() + OSSpinLockLock(&self.spinLock) defer { - self.unlock() + OSSpinLockUnlock(&self.spinLock) } if let query = self.inflightRequests[productIds] { @@ -89,22 +83,4 @@ class ProductsInfoController: NSObject { inflightRequests[productIds]!.completionHandlers.append(completion) } } - - - - private func lock() { - if #available(iOSApplicationExtension 10.0, *) { - os_unfair_lock_lock(&self.unfairLock) - } else { - OSSpinLockLock(&self.spinLock) - } - } - - private func unlock() { - if #available(iOSApplicationExtension 10.0, *) { - os_unfair_lock_unlock(&self.unfairLock) - } else { - OSSpinLockUnlock(&self.spinLock) - } - } } From b8ee2d354e78bfd238379fb1695c72122a060ba4 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Mon, 30 Sep 2019 14:44:04 +0200 Subject: [PATCH 9/9] fix: reduce the wait time to fire the callbacks --- SwiftyStoreKitTests/ProductsInfoControllerTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift index 0d356a34..5b2e73a9 100644 --- a/SwiftyStoreKitTests/ProductsInfoControllerTests.swift +++ b/SwiftyStoreKitTests/ProductsInfoControllerTests.swift @@ -160,7 +160,7 @@ class ProductsInfoControllerTests: XCTestCase { } } } - DispatchQueue.global().asyncAfter(deadline: .now() + 1) { + DispatchQueue.global().asyncAfter(deadline: .now()+0.1) { requestBuilder.fireCallbacks() } // Fullfil the expectation when every thread finishes