Skip to content

Commit ab596ba

Browse files
jaclyncjaclync
authored
Merge pull request #2135 from woocommerce/issue/1713-thread-safe-product-image-statuses
Thread safety for image statuses in `ProductImageActionHandler`
2 parents 2a87d08 + 141af43 commit ab596ba

File tree

2 files changed

+138
-56
lines changed

2 files changed

+138
-56
lines changed

‎WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift‎

Lines changed: 111 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,22 @@ final class ProductImageActionHandler {
1010

1111
private let siteID: Int64
1212

13+
/// The queue where internal states like `allStatuses` and `observations` are updated on to maintain thread safety.
14+
private let queue: DispatchQueue
15+
1316
var productImageStatuses: [ProductImageStatus] {
1417
return allStatuses.productImageStatuses
1518
}
1619

1720
private var allStatuses: AllStatuses {
1821
didSet {
19-
observations.allStatusesUpdated.values.forEach { closure in
20-
closure(allStatuses)
22+
queue.async { [weak self] in
23+
guard let self = self else {
24+
return
25+
}
26+
self.observations.allStatusesUpdated.values.forEach { closure in
27+
closure(self.allStatuses)
28+
}
2129
}
2230
}
2331
}
@@ -27,37 +35,55 @@ final class ProductImageActionHandler {
2735
assetUploaded: [UUID: OnAssetUpload]()
2836
)
2937

30-
init(siteID: Int64, product: Product) {
38+
/// - Parameters:
39+
/// - siteID: the ID of a site/store where the product belongs to.
40+
/// - product: the product whose image statuses and actions are of concern.
41+
/// - queue: the queue where the update callbacks are called on. Default to be the main queue.
42+
init(siteID: Int64, product: Product, queue: DispatchQueue = .main) {
3143
self.siteID = siteID
44+
self.queue = queue
3245
self.allStatuses = (productImageStatuses: product.imageStatuses, error: nil)
3346
}
3447

3548
/// Observes when the image statuses have been updated.
3649
///
3750
/// - Parameters:
3851
/// - observer: the observer that `onUpdate` is associated with.
39-
/// - onUpdate: called when the image statuses have been updated, if `observer` is not nil.
52+
/// - onUpdate: called when the image statuses have been updated on the thread passed in the initializer (default to the main thread),
53+
/// if `observer` is not nil.
4054
@discardableResult
4155
func addUpdateObserver<T: AnyObject>(_ observer: T,
4256
onUpdate: @escaping OnAllStatusesUpdate) -> ObservationToken {
4357
let id = UUID()
4458

45-
observations.allStatusesUpdated[id] = { [weak self, weak observer] allStatuses in
46-
// If the observer has been deallocated, we can
47-
// automatically remove the observation closure.
48-
guard observer != nil else {
49-
self?.observations.allStatusesUpdated.removeValue(forKey: id)
59+
queue.async { [weak self] in
60+
guard let self = self else {
5061
return
5162
}
5263

53-
onUpdate(allStatuses)
54-
}
64+
self.observations.allStatusesUpdated[id] = { [weak self, weak observer] allStatuses in
65+
guard let self = self else {
66+
return
67+
}
68+
69+
// If the observer has been deallocated, we can
70+
// automatically remove the observation closure.
71+
guard observer != nil else {
72+
self.observations.allStatusesUpdated.removeValue(forKey: id)
73+
return
74+
}
5575

56-
// Sends the initial value.
57-
onUpdate(allStatuses)
76+
onUpdate(self.allStatuses)
77+
}
78+
79+
// Sends the initial value.
80+
onUpdate(self.allStatuses)
81+
}
5882

5983
return ObservationToken { [weak self] in
60-
self?.observations.allStatusesUpdated.removeValue(forKey: id)
84+
self?.queue.async { [weak self] in
85+
self?.observations.allStatusesUpdated.removeValue(forKey: id)
86+
}
6187
}
6288
}
6389

@@ -71,70 +97,103 @@ final class ProductImageActionHandler {
7197
onAssetUpload: @escaping OnAssetUpload) -> ObservationToken {
7298
let id = UUID()
7399

74-
observations.assetUploaded[id] = { [weak self, weak observer] asset, productImage in
75-
// If the observer has been deallocated, we can
76-
// automatically remove the observation closure.
77-
guard observer != nil else {
78-
self?.observations.assetUploaded.removeValue(forKey: id)
100+
queue.async { [weak self] in
101+
guard let self = self else {
79102
return
80103
}
81104

82-
onAssetUpload(asset, productImage)
105+
self.observations.assetUploaded[id] = { [weak self, weak observer] asset, productImage in
106+
// If the observer has been deallocated, we can
107+
// automatically remove the observation closure.
108+
guard observer != nil else {
109+
self?.observations.assetUploaded.removeValue(forKey: id)
110+
return
111+
}
112+
113+
onAssetUpload(asset, productImage)
114+
}
83115
}
84116

85117
return ObservationToken { [weak self] in
86-
self?.observations.assetUploaded.removeValue(forKey: id)
118+
self?.queue.async { [weak self] in
119+
self?.observations.assetUploaded.removeValue(forKey: id)
120+
}
87121
}
88122
}
89123

90124
func addSiteMediaLibraryImagesToProduct(mediaItems: [Media]) {
91-
let newProductImageStatuses = mediaItems.map { ProductImageStatus.remote(image: $0.toProductImage) }
92-
let imageStatuses = newProductImageStatuses + productImageStatuses
93-
allStatuses = (productImageStatuses: imageStatuses, error: nil)
125+
queue.async { [weak self] in
126+
guard let self = self else {
127+
return
128+
}
129+
130+
let newProductImageStatuses = mediaItems.map { ProductImageStatus.remote(image: $0.toProductImage) }
131+
let imageStatuses = newProductImageStatuses + self.productImageStatuses
132+
self.allStatuses = (productImageStatuses: imageStatuses, error: nil)
133+
}
94134
}
95135

96136
func uploadMediaAssetToSiteMediaLibrary(asset: PHAsset) {
97-
let imageStatuses = [.uploading(asset: asset)] + allStatuses.productImageStatuses
98-
allStatuses = (productImageStatuses: imageStatuses, error: nil)
137+
queue.async { [weak self] in
138+
guard let self = self else {
139+
return
140+
}
99141

100-
let action = MediaAction.uploadMedia(siteID: siteID,
101-
mediaAsset: asset) { [weak self] (media, error) in
102-
guard let self = self else {
103-
return
104-
}
142+
let imageStatuses = [.uploading(asset: asset)] + self.allStatuses.productImageStatuses
143+
self.allStatuses = (productImageStatuses: imageStatuses, error: nil)
105144

106-
guard let index = self.index(of: asset) else {
107-
return
108-
}
145+
self.uploadMediaAssetToSiteMediaLibrary(asset: asset) { [weak self] (media, error) in
146+
self?.queue.async { [weak self] in
147+
guard let self = self else {
148+
return
149+
}
109150

110-
guard let media = media else {
111-
DispatchQueue.main.async {
151+
guard let index = self.index(of: asset) else {
152+
return
153+
}
154+
155+
guard let media = media else {
112156
self.updateProductImageStatus(at: index, error: error)
157+
return
113158
}
114-
return
115-
}
116-
let productImage = ProductImage(imageID: media.mediaID,
117-
dateCreated: media.date,
118-
dateModified: media.date,
119-
src: media.src,
120-
name: media.name,
121-
alt: media.alt)
122-
DispatchQueue.main.async {
159+
let productImage = ProductImage(imageID: media.mediaID,
160+
dateCreated: media.date,
161+
dateModified: media.date,
162+
src: media.src,
163+
name: media.name,
164+
alt: media.alt)
123165
self.updateProductImageStatus(at: index, productImage: productImage)
124166
}
167+
}
168+
}
169+
}
170+
171+
private func uploadMediaAssetToSiteMediaLibrary(asset: PHAsset, onCompletion: @escaping (_ uploadedMedia: Media?, _ error: Error?) -> Void) {
172+
DispatchQueue.main.async { [weak self] in
173+
guard let siteID = self?.siteID else {
174+
return
175+
}
176+
177+
let action = MediaAction.uploadMedia(siteID: siteID, mediaAsset: asset, onCompletion: onCompletion)
178+
ServiceLocator.stores.dispatch(action)
125179
}
126-
ServiceLocator.stores.dispatch(action)
127180
}
128181

129182
func deleteProductImage(_ productImage: ProductImage) {
130-
var imageStatuses = allStatuses.productImageStatuses
131-
imageStatuses.removeAll { status -> Bool in
132-
guard case .remote(let image) = status else {
133-
return false
183+
queue.async { [weak self] in
184+
guard let self = self else {
185+
return
134186
}
135-
return image.imageID == productImage.imageID
187+
188+
var imageStatuses = self.allStatuses.productImageStatuses
189+
imageStatuses.removeAll { status -> Bool in
190+
guard case .remote(let image) = status else {
191+
return false
192+
}
193+
return image.imageID == productImage.imageID
194+
}
195+
self.allStatuses = (productImageStatuses: imageStatuses, error: nil)
136196
}
137-
allStatuses = (productImageStatuses: imageStatuses, error: nil)
138197
}
139198

140199
/// Resets the product images to the ones from the given Product.

‎WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ final class ProductImageActionHandlerTests: XCTestCase {
5050

5151
var observedProductImageStatusChanges: [[ProductImageStatus]] = []
5252
productImageActionHandler.addUpdateObserver(self) { (productImageStatuses, error) in
53+
XCTAssertTrue(Thread.current.isMainThread)
5354
observedProductImageStatusChanges.append(productImageStatuses)
5455
if observedProductImageStatusChanges.count >= expectedStatusUpdates.count {
5556
waitForStatusUpdates.fulfill()
@@ -58,6 +59,7 @@ final class ProductImageActionHandlerTests: XCTestCase {
5859

5960
let waitForAssetUpload = self.expectation(description: "Wait for asset upload callback from image upload")
6061
productImageActionHandler.addAssetUploadObserver(self) { (asset, productImage) in
62+
XCTAssertTrue(Thread.current.isMainThread)
6163
XCTAssertEqual(asset, mockAsset)
6264
XCTAssertEqual(productImage, mockUploadedProductImage)
6365
waitForAssetUpload.fulfill()
@@ -98,6 +100,7 @@ final class ProductImageActionHandlerTests: XCTestCase {
98100

99101
var observedProductImageStatusChanges: [[ProductImageStatus]] = []
100102
productImageActionHandler.addUpdateObserver(self) { (productImageStatuses, error) in
103+
XCTAssertTrue(Thread.current.isMainThread)
101104
observedProductImageStatusChanges.append(productImageStatuses)
102105
if observedProductImageStatusChanges.count >= expectedStatusUpdates.count {
103106
expectation.fulfill()
@@ -134,6 +137,7 @@ final class ProductImageActionHandlerTests: XCTestCase {
134137

135138
var observedProductImageStatusChanges: [[ProductImageStatus]] = []
136139
productImageActionHandler.addUpdateObserver(self) { (productImageStatuses, error) in
140+
XCTAssertTrue(Thread.current.isMainThread)
137141
observedProductImageStatusChanges.append(productImageStatuses)
138142
if observedProductImageStatusChanges.count >= expectedStatusUpdates.count {
139143
expectation.fulfill()
@@ -163,7 +167,7 @@ final class ProductImageActionHandlerTests: XCTestCase {
163167
let productImageActionHandler = ProductImageActionHandler(siteID: 123,
164168
product: mockProduct)
165169

166-
// Act
170+
// Media items to upload to site media library.
167171
let mockMedia1 = Media(mediaID: 134, date: Date(),
168172
fileExtension: "jpg", mimeType: "image/jpeg",
169173
src: "pic", thumbnailURL: "https://test.com/pic1",
@@ -175,12 +179,31 @@ final class ProductImageActionHandlerTests: XCTestCase {
175179
name: "woo", alt: "the second image",
176180
height: 320, width: 776)
177181
let mockMediaItems = [mockMedia1, mockMedia2]
182+
183+
let expectedImageStatusesFromSiteMediaLibrary = mockMediaItems.map { ProductImageStatus.remote(image: $0.toProductImage) }
184+
let expectedStatusUpdates: [[ProductImageStatus]] = [
185+
mockRemoteProductImageStatuses,
186+
expectedImageStatusesFromSiteMediaLibrary + mockRemoteProductImageStatuses
187+
]
188+
189+
let expectation = self.expectation(description: "Wait for image upload")
190+
expectation.expectedFulfillmentCount = 1
191+
192+
var observedProductImageStatusChanges: [[ProductImageStatus]] = []
193+
productImageActionHandler.addUpdateObserver(self) { (productImageStatuses, error) in
194+
XCTAssertTrue(Thread.current.isMainThread)
195+
observedProductImageStatusChanges.append(productImageStatuses)
196+
if observedProductImageStatusChanges.count >= expectedStatusUpdates.count {
197+
expectation.fulfill()
198+
}
199+
}
200+
201+
// Act
178202
productImageActionHandler.addSiteMediaLibraryImagesToProduct(mediaItems: mockMediaItems)
179203

180204
// Assert
181-
let expectedImageStatusesFromSiteMediaLibrary = mockMediaItems.map { ProductImageStatus.remote(image: $0.toProductImage) }
182-
let expectedImageStatuses = expectedImageStatusesFromSiteMediaLibrary + mockRemoteProductImageStatuses
183-
XCTAssertEqual(productImageActionHandler.productImageStatuses, expectedImageStatuses)
205+
waitForExpectations(timeout: Constants.expectationTimeout, handler: nil)
206+
XCTAssertEqual(observedProductImageStatusChanges, expectedStatusUpdates)
184207
}
185208

186209
// MARK: - `resetProductImages(to:)`

0 commit comments

Comments
 (0)