Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,21 @@ final class AlamofireNetworkErrorHandler {
originalRequest: URLRequestConvertible,
failure: Error?
) {
let retriedRequestIndex = retriedJetpackRequests.firstIndex { retriedRequest in
let urlRequest = try? originalRequest.asURLRequest()
let retriedRequest = try? retriedRequest.request.asURLRequest()
return urlRequest == retriedRequest
}
let retriedRequest: RetriedJetpackRequest? = queue.sync(flags: .barrier) { [weak self] in
guard let self else { return nil }

let retriedRequestIndex = _retriedJetpackRequests.firstIndex { retriedRequest in
let urlRequest = try? originalRequest.asURLRequest()
let retriedRequest = try? retriedRequest.request.asURLRequest()
return urlRequest == retriedRequest
}

guard let index = retriedRequestIndex else { return }
guard let index = retriedRequestIndex else { return nil }

return _retriedJetpackRequests.remove(at: index)
}

let retriedRequest = retriedJetpackRequests.remove(at: index)
guard let retriedRequest else { return }

if failure == nil {
let siteID = retriedRequest.request.siteID
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,52 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase {
// Then
XCTAssertTrue(userDefaults.applicationPasswordUnsupportedList.keys.contains(String(siteID)))
}

func test_concurrent_flagSiteAsUnsupportedForAppPasswordIfNeeded_no_race_condition() {
// Given - test for the race condition fix where multiple threads
// try to remove the same item simultaneously
let expectation = XCTestExpectation(description: "All concurrent flag operations complete without crash")
let threadCount = 50
let siteID: Int64 = 999
let jetpackRequest = createJetpackRequest(siteID: siteID)
let restRequest = createRESTRequest()
let error = createNetworkError()

expectation.expectedFulfillmentCount = threadCount

// Add a single request to the retry list
_ = errorHandler.shouldRetryJetpackRequest(
originalRequest: jetpackRequest,
convertedRequest: restRequest,
failure: error
)

// When - multiple threads simultaneously try to flag and remove the SAME item
// This would cause a race condition in the old code where:
// 1. Thread A reads the array, finds index 0
// 2. Thread B reads the array, finds index 0
// 3. Thread A removes at index 0 (succeeds)
// 4. Thread B tries to remove at index 0 (crashes - array is now empty)
let group = DispatchGroup()
for _ in 0..<threadCount {
group.enter()
DispatchQueue.global().async {
// All threads try to remove the same request concurrently
self.errorHandler.flagSiteAsUnsupportedForAppPasswordIfNeeded(
originalRequest: jetpackRequest,
failure: nil
)
group.leave()
expectation.fulfill()
}
}

// Force threads to start as close together as possible
group.wait()

// Then - no crashes should occur (especially no EXC_BREAKPOINT from array index out of bounds)
wait(for: [expectation], timeout: 5.0)
}
}

// MARK: - Helper Methods
Expand Down
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
23.5
-----
- [internal] POS code was moved to its own module, including POS specific color and image assets. [https://github.com/woocommerce/woocommerce-ios/pull/16176]
- [*] Fix race condition when flagging sites as unsupported for application passwords [https://github.com/woocommerce/woocommerce-ios/pull/16223]
- [*] Fix Create Coupon screen scrolling issue. [https://github.com/woocommerce/woocommerce-ios/pull/16218]


Expand Down