Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Small notice, might be negligible as _retriedJetpackRequests could be a few items, but we could move parsing originalRequest outside firstIndex so it only runs once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I moved the parsing of originalRequest to outside of the firstIndex block in b63ea52.

let retriedRequest = try? retriedRequest.request.asURLRequest()
return urlRequest == retriedRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Relying on == for URLRequest may compare unstable fields, like header order. This can cause misses where the request is present but not found. However, I'm not sure what the intentions are here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, just want to point out, that if both try? are nil, we match the request to the original. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with both of your points - updated the implementation in b63ea52.

}

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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Small note — the group.wait() here doesn’t actually make the threads start together; it just waits for them to finish. In this case, that’s probably fine since the goal is to ensure no crash under load, but I just wanted to flag it in case a true concurrent start was intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comments in ed4f8fa, sorry for the misleading notes.


// 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
Loading