Skip to content

Update ProductsRemote.loadAllProducts to call async/await enqueue method for response parsing to be in a background thread #15743

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 17, 2025
Merged

Update ProductsRemote.loadAllProducts to call async/await enqueue method for response parsing to be in a background thread #15743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63ae531
Update `ProductsRemote.loadAllProducts` to call async/await enqueue m…
jaclync Jun 13, 2025
914957d
Remove trailing whitespace.
jaclync Jun 13, 2025
e9bd05f
Use generic list mapper for future enhancements to limit response size.
jaclync Jun 13, 2025
40268f3
Fix test failure that is caused by remote call moved to an async Task.
jaclync Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Networking/NetworkingTests/Extensions/URLRequestConvertible+PathTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ final class URLRequestConvertible_PathTests: XCTestCase {
// MARK: - `pathForAnalytics`

// Example from `ProductsRemote.loadAllProducts`.
func test_pathForAnalytics_returns_path_of_JetpackRequest() throws {
func test_pathForAnalytics_returns_path_of_JetpackRequest() async throws {
// Given
let productsRemote = ProductsRemote(network: network)
productsRemote.loadAllProducts(for: 134, completion: { _ in })
_ = try? await productsRemote.loadAllProducts(for: 134)

// When
let request = try XCTUnwrap(network.requestsForResponseData.first)
Expand Down
64 changes: 20 additions & 44 deletions Networking/NetworkingTests/Remote/ProductsRemoteTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,39 +256,28 @@ final class ProductsRemoteTests: XCTestCase {

/// Verifies that loadAllProducts properly parses the `products-load-all` sample response.
///
func testLoadAllProductsProperlyReturnsParsedProducts() {
func test_loadAllProducts_properly_returns_parsed_products() async throws {
// Given
let remote = ProductsRemote(network: network)
let expectation = self.expectation(description: "Load All Products")

network.simulateResponse(requestUrlSuffix: "products", filename: "products-load-all")

remote.loadAllProducts(for: sampleSiteID) { result in
switch result {
case .success(let products):
XCTAssertEqual(products.count, 10)
default:
XCTFail("Unexpected result: \(result)")
}
expectation.fulfill()
}
// When
let products = try await remote.loadAllProducts(for: sampleSiteID)

wait(for: [expectation], timeout: Constants.expectationTimeout)
// Then
XCTAssertEqual(products.count, 10)
}

/// Verifies that loadAllProducts with `excludedProductIDs` makes a network request with the corresponding parameter.
///
func testLoadAllProductsWithExcludedIDsIncludesAnExcludeParamInNetworkRequest() throws {
func test_loadAllProducts_with_excluded_ids_includes_an_exclude_param_in_network_request() async throws {
// Arrange
let remote = ProductsRemote(network: network)
network.simulateResponse(requestUrlSuffix: "products", filename: "products-load-all")
let excludedProductIDs: [Int64] = [17, 671]

// Action
waitForExpectation { expectation in
remote.loadAllProducts(for: sampleSiteID, excludedProductIDs: excludedProductIDs) { result in
expectation.fulfill()
}
}
_ = try await remote.loadAllProducts(for: sampleSiteID, excludedProductIDs: excludedProductIDs)

// Assert
let queryParameters = try XCTUnwrap(network.queryParameters)
Expand All @@ -298,18 +287,14 @@ final class ProductsRemoteTests: XCTestCase {

/// Verifies that loadAllProducts with `productIDs` makes a network request with the `include` parameter.
///
func test_loadAllProducts_with_productIDs_adds_a_include_param_in_network_request() throws {
func test_loadAllProducts_with_productIDs_adds_a_include_param_in_network_request() async throws {
// Given
let remote = ProductsRemote(network: network)
network.simulateResponse(requestUrlSuffix: "products", filename: "products-load-all")
let productIDs: [Int64] = [13, 61]

// When
waitForExpectation { expectation in
remote.loadAllProducts(for: sampleSiteID, productIDs: productIDs) { result in
expectation.fulfill()
}
}
_ = try await remote.loadAllProducts(for: sampleSiteID, productIDs: productIDs)

// Then
let queryParameters = try XCTUnwrap(network.queryParameters)
Expand All @@ -319,17 +304,13 @@ final class ProductsRemoteTests: XCTestCase {

/// Verifies that loadAllProducts with empty `productIDs` makes a network request without the `include` parameter.
///
func test_loadAllProducts_with_empty_productIDs_does_not_add_include_param_in_network_request() throws {
func test_loadAllProducts_with_empty_productIDs_does_not_add_include_param_in_network_request() async throws {
// Given
let remote = ProductsRemote(network: network)
network.simulateResponse(requestUrlSuffix: "products", filename: "products-load-all")

// When
waitForExpectation { expectation in
remote.loadAllProducts(for: sampleSiteID, productIDs: []) { result in
expectation.fulfill()
}
}
_ = try await remote.loadAllProducts(for: sampleSiteID, productIDs: [])

// Then
let queryParameters = try XCTUnwrap(network.queryParameters)
Expand All @@ -339,21 +320,16 @@ final class ProductsRemoteTests: XCTestCase {

/// Verifies that loadAllProducts properly relays Networking Layer errors.
///
func test_loadAllProducts_properly_relays_netwoking_errors() {
func test_loadAllProducts_properly_relays_netwoking_errors() async {
// Given
let remote = ProductsRemote(network: network)
let expectation = self.expectation(description: "Load all products returns error")

remote.loadAllProducts(for: sampleSiteID) { result in
switch result {
case .failure:
break
default:
XCTFail("Unexpected result: \(result)")
}
expectation.fulfill()
}

wait(for: [expectation], timeout: Constants.expectationTimeout)
do {
_ = try await remote.loadAllProducts(for: sampleSiteID)
XCTFail("Expected error to be thrown")
} catch {
XCTAssertEqual(error as? NetworkError, .notFound())
}
}


Expand Down
10 changes: 4 additions & 6 deletions Networking/Sources/Extended/Remote/ProductsRemote.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ public protocol ProductsRemoteProtocol {
orderBy: ProductsRemote.OrderKey,
order: ProductsRemote.Order,
productIDs: [Int64],
excludedProductIDs: [Int64],
completion: @escaping (Result<[Product], Error>) -> Void)
excludedProductIDs: [Int64]) async throws -> [Product]
func searchProducts(for siteID: Int64,
keyword: String,
pageNumber: Int,
Expand Down Expand Up @@ -179,8 +178,7 @@ public final class ProductsRemote: Remote, ProductsRemoteProtocol {
orderBy: OrderKey = .name,
order: Order = .ascending,
productIDs: [Int64] = [],
excludedProductIDs: [Int64] = [],
completion: @escaping (Result<[Product], Error>) -> Void) {
excludedProductIDs: [Int64] = []) async throws -> [Product] {
let stringOfExcludedProductIDs = excludedProductIDs.map { String($0) }
.joined(separator: ",")
let stringOfProductIDs: String = {
Expand Down Expand Up @@ -211,9 +209,9 @@ public final class ProductsRemote: Remote, ProductsRemoteProtocol {

let path = Path.products
let request = JetpackRequest(wooApiVersion: .mark3, method: .get, siteID: siteID, path: path, parameters: parameters, availableAsRESTRequest: true)
let mapper = ProductListMapper(siteID: siteID)
let mapper = ListMapper<Product>(siteID: siteID)

enqueue(request, mapper: mapper, completion: completion)
return try await enqueue(request, mapper: mapper)
}

/// Retrieves products for the Point of Sale. Simple and variable products are loaded for WC version 9.6+, otherwise only simple products are loaded.
Expand Down
117 changes: 67 additions & 50 deletions Yosemite/Yosemite/Stores/ProductStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,24 @@ public class ProductStore: Store {
let excludedProductIDs,
let shouldDeleteStoredProductsOnFirstPage,
let onCompletion):
synchronizeProducts(siteID: siteID,
pageNumber: pageNumber,
pageSize: pageSize,
stockStatus: stockStatus,
productStatus: productStatus,
productType: productType,
productCategory: productCategory,
sortOrder: sortOrder,
productIDs: productIDs,
excludedProductIDs: excludedProductIDs,
shouldDeleteStoredProductsOnFirstPage: shouldDeleteStoredProductsOnFirstPage,
onCompletion: onCompletion)
Task { @MainActor in
Copy link
Contributor

@iamgabrielma iamgabrielma Jun 16, 2025

Choose a reason for hiding this comment

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

In one of my test runs I did encounter a concurrency warning on ProductAttribute+ReadOnlyConvertible but I have not been able to reproduce it again. From my initial check, it seemed to come from here so I've tried to play around with removing the call to @MainActor in this task, but I saw no difference

[...]
/woocommerce-ios/Yosemite/Yosemite/Model/Storage/ProductAttribute+ReadOnlyConvertible.swift:24 Performing I/O on the main thread can cause hangs.
This is known to cause hangs for your users.

Since this sync calls both the mapper via loadAllProducts and later upsertStoredProductsInBackground in background threads, perhaps we are able to remove the MainActor tag if not needed 🤔

Copy link
Contributor Author

@jaclync jaclync Jun 17, 2025

Choose a reason for hiding this comment

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

In one of my test runs I did encounter a concurrency warning on ProductAttribute+ReadOnlyConvertible but I have not been able to reproduce it again.

Did the stack trace start from accessing ResultsController.fetchedObjects? I've been seeing ProductAttribute+ReadOnlyConvertible warnings in trunk and the PR branch for a while. I think it's from Product's relationships being loaded on the main thread (they were probably not loaded in storage before that) when converting the storage object to readonly struct. And there are several relationships of the Product model, all of them can have any number of related objects. I don't see an issue yet, will create one.

Screenshot 2025-06-16 at 9 53 26 PM Screenshot 2025-06-16 at 9 54 07 PM Screenshot 2025-06-16 at 9 56 54 PM

Since this sync calls both the mapper via loadAllProducts and later upsertStoredProductsInBackground in background threads, perhaps we are able to remove the MainActor tag if not needed 🤔

The @MainActor only puts the non-awaiting code on the main thread, in this case the completion callbacks. I think this is expected and how it works before, this PR just ensures the networking code, particularly the response decoding part, is not on the main thread.

Copy link
Contributor Author

@jaclync jaclync Jun 17, 2025

Choose a reason for hiding this comment

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

Created an issue about the performance warning: WOOMOB-619

Copy link
Contributor

@iamgabrielma iamgabrielma Jun 17, 2025

Choose a reason for hiding this comment

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

Did the stack trace start from accessing ResultsController.fetchedObjects? I've been seeing ProductAttribute+ReadOnlyConvertible warnings in trunk and the PR branch for a while.

I'm not sure, but now that you mention it I might have seen the same warning outside this PR as well before 🤔

Thanks for logging it in!

The @mainactor only puts the non-awaiting code on the main thread, in this case the completion callbacks. I think this is expected and how it works before, this PR just ensures the networking code, particularly the response decoding part, is not on the main thread.

👍

do {
let hasNextPage = try await synchronizeProducts(siteID: siteID,
pageNumber: pageNumber,
pageSize: pageSize,
stockStatus: stockStatus,
productStatus: productStatus,
productType: productType,
productCategory: productCategory,
sortOrder: sortOrder,
productIDs: productIDs,
excludedProductIDs: excludedProductIDs,
shouldDeleteStoredProductsOnFirstPage: shouldDeleteStoredProductsOnFirstPage)
onCompletion(.success(hasNextPage))
} catch {
onCompletion(.failure(error))
}
}
case .requestMissingProducts(let order, let onCompletion):
requestMissingProducts(for: order, onCompletion: onCompletion)
case .updateProduct(let product, let onCompletion):
Expand Down Expand Up @@ -287,43 +293,35 @@ private extension ProductStore {
sortOrder: ProductsSortOrder,
productIDs: [Int64],
excludedProductIDs: [Int64],
shouldDeleteStoredProductsOnFirstPage: Bool,
onCompletion: @escaping (Result<Bool, Error>) -> Void) {
remote.loadAllProducts(for: siteID,
context: nil,
pageNumber: pageNumber,
pageSize: pageSize,
stockStatus: stockStatus,
productStatus: productStatus,
productType: productType,
productCategory: productCategory,
orderBy: sortOrder.remoteOrderKey,
order: sortOrder.remoteOrder,
productIDs: productIDs,
excludedProductIDs: excludedProductIDs) { [weak self] result in
switch result {
case .failure(let error):
if let productType,
let error = error as? DotcomError,
case let .unknown(code, message) = error,
code == "rest_invalid_param",
message == "Invalid parameter(s): type",
ProductType.coreTypes.contains(productType) == false {
return onCompletion(.success(false))
}
onCompletion(.failure(error))
case .success(let products):
guard let self = self else {
return
}
let shouldDeleteExistingProducts = pageNumber == Default.firstPageNumber && shouldDeleteStoredProductsOnFirstPage
self.upsertStoredProductsInBackground(readOnlyProducts: products,
siteID: siteID,
shouldDeleteExistingProducts: shouldDeleteExistingProducts) {
let hasNextPage = products.count == pageSize
onCompletion(.success(hasNextPage))
}
}
shouldDeleteStoredProductsOnFirstPage: Bool) async throws -> Bool {
do {
let products = try await remote.loadAllProducts(for: siteID,
context: nil,
pageNumber: pageNumber,
pageSize: pageSize,
stockStatus: stockStatus,
productStatus: productStatus,
productType: productType,
productCategory: productCategory,
orderBy: sortOrder.remoteOrderKey,
order: sortOrder.remoteOrder,
productIDs: productIDs,
excludedProductIDs: excludedProductIDs)

let shouldDeleteExistingProducts = pageNumber == Default.firstPageNumber && shouldDeleteStoredProductsOnFirstPage
await upsertStoredProductsInBackground(readOnlyProducts: products,
siteID: siteID,
shouldDeleteExistingProducts: shouldDeleteExistingProducts)
let hasNextPage = products.count == pageSize
return hasNextPage
} catch let error as DotcomError where error == .unknown(code: "rest_invalid_param", message: "Invalid parameter(s): type") {
if let productType,
ProductType.coreTypes.contains(productType) == false {
return false
}
throw error
} catch {
throw error
}
}

Expand Down Expand Up @@ -852,6 +850,23 @@ extension ProductStore {
}, completion: onCompletion, on: .main)
}

/// Updates (OR Inserts) the specified ReadOnly Product Entities *in a background thread* async.
/// Also deletes existing products if requested.
func upsertStoredProductsInBackground(readOnlyProducts: [Networking.Product],
siteID: Int64,
shouldDeleteExistingProducts: Bool = false) async {
await withCheckedContinuation { [weak self] continuation in
guard let self else {
return continuation.resume()
}
upsertStoredProductsInBackground(readOnlyProducts: readOnlyProducts,
siteID: siteID,
shouldDeleteExistingProducts: shouldDeleteExistingProducts) {
continuation.resume()
}
}
}

/// Updates (OR Inserts) the specified ReadOnly Product Entities *in a background thread*.
/// Also deletes existing products if requested.
/// `onCompletion` will be called on the main thread!
Expand All @@ -861,7 +876,9 @@ extension ProductStore {
shouldDeleteExistingProducts: Bool = false,
onCompletion: @escaping () -> Void) {
storageManager.performAndSave({ [weak self] storage in
guard let self else { return }
guard let self else {
return onCompletion()
}
if shouldDeleteExistingProducts {
storage.deleteProducts(siteID: siteID)
}
Expand Down
15 changes: 10 additions & 5 deletions Yosemite/YosemiteTests/Mocks/Networking/Remote/MockProductsRemote.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,7 @@ extension MockProductsRemote: ProductsRemoteProtocol {
orderBy: ProductsRemote.OrderKey,
order: ProductsRemote.Order,
productIDs: [Int64],
excludedProductIDs: [Int64],
completion: @escaping (Result<[Product], Error>) -> Void) {
excludedProductIDs: [Int64]) async throws -> [Product] {
synchronizeProductsTriggered = true
synchronizeProductsWithStockStatus = stockStatus
synchronizeProductsWithProductStatus = productStatus
Expand All @@ -271,10 +270,16 @@ extension MockProductsRemote: ProductsRemoteProtocol {
synchronizeProductsOrder = order
synchronizeProductsProductIDs = productIDs
synchronizeProductsExcludedProductIDs = excludedProductIDs
if let result = loadAllProductsResultsBySiteID[siteID] {
completion(result)
} else {
guard let result = loadAllProductsResultsBySiteID[siteID] else {
XCTFail("\(String(describing: self)) Could not find Result for \(siteID)")
throw NetworkError.notFound()
}

switch result {
case let .success(products):
return products
case let .failure(error):
throw error
}
}

Expand Down
Loading