Skip to content

Commit cbfd82c

Browse files
loganseaseclaude
andcommitted
Make NetworkingClient.headers thread-safe
headers was a plain stored dictionary read on every request build (and on retries from URLSession/Combine threads) while being rewritten on auth token refresh from arbitrary threads. Swift Dictionary is not thread-safe, so this raced and corrupted the CoW buffer, producing the Crashlytics crashes in NetworkingClient.addHeaders/removeHeaders and NetworkingRequest.__ivar_destroyer (EXC_BAD_ACCESS / EXC_BREAKPOINT / SIGABRT). - back headers with an NSLock-protected private store; the public property keeps the exact same get/set surface, so existing consumers compile and behave unchanged - add withHeaders(_:) for callers that need an atomic read-modify-write (e.g. merging auth headers) instead of racy get-modify-set - add regression tests; the concurrent hammer test segfaults (signal 11) against the previous implementation and passes with the lock Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent bcebda8 commit cbfd82c

2 files changed

Lines changed: 128 additions & 1 deletion

File tree

Sources/Networking/NetworkingClient.swift

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,40 @@ public class NetworkingClient {
1010
*/
1111
public var defaultCollectionParsingKeyPath: String?
1212
let baseURL: String
13-
public var headers = [String: String]()
13+
14+
// Headers are read on every request build (including retries on URLSession/Combine
15+
// threads) and mutated on auth token refresh from arbitrary threads. Dictionary is
16+
// not thread-safe, so all access goes through a lock. The public property keeps the
17+
// same get/set surface as the previous stored `var`.
18+
private let headersLock = NSLock()
19+
private var _headers = [String: String]()
20+
public var headers: [String: String] {
21+
get {
22+
headersLock.lock()
23+
defer { headersLock.unlock() }
24+
return _headers
25+
}
26+
set {
27+
headersLock.lock()
28+
defer { headersLock.unlock() }
29+
_headers = newValue
30+
}
31+
}
32+
33+
/**
34+
Atomically reads and mutates the headers dictionary under the same lock that
35+
protects the `headers` property. Use this instead of get-modify-set on `headers`
36+
when the modification must not race with other writers (e.g. merging auth headers).
37+
Do not access `headers` or call `withHeaders` again from inside `body` — the lock
38+
is not reentrant.
39+
*/
40+
@discardableResult
41+
public func withHeaders<T>(_ body: (inout [String: String]) -> T) -> T {
42+
headersLock.lock()
43+
defer { headersLock.unlock() }
44+
return body(&_headers)
45+
}
46+
1447
public var parameterEncoding = ParameterEncoding.urlEncoded
1548
public var timeout: TimeInterval?
1649
public var sessionConfiguration = URLSessionConfiguration.default
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
//
2+
// HeadersThreadSafetyTests.swift
3+
//
4+
// Regression tests for the data race on NetworkingClient.headers that caused
5+
// EXC_BAD_ACCESS / EXC_BREAKPOINT / SIGABRT crashes in production when auth
6+
// header updates raced with request building on other threads.
7+
//
8+
9+
import Foundation
10+
import XCTest
11+
12+
@testable
13+
import Networking
14+
15+
final class HeadersThreadSafetyTests: XCTestCase {
16+
17+
// Hammers the same access pattern that crashed in production:
18+
// writers doing get-modify-set add/remove cycles (auth token refresh) while
19+
// readers copy the dictionary (request building). Before headers was
20+
// lock-protected this reliably corrupted the dictionary's CoW buffer.
21+
func testConcurrentReadsAndWritesDoNotCrash() {
22+
let network = NetworkingClient(baseURL: "https://mocked.com")
23+
network.headers = ["Client-Version": "1.0.0", "Client-OS": "iOS"]
24+
25+
let group = DispatchGroup()
26+
let queue = DispatchQueue.global(qos: .userInitiated)
27+
let iterations = 2_000
28+
29+
// writers: replace auth headers, like setAuthHeaders on token refresh
30+
for writer in 0..<4 {
31+
queue.async(group: group) {
32+
for i in 0..<iterations {
33+
var copy = network.headers
34+
copy.removeValue(forKey: "Authorization")
35+
network.headers = copy
36+
network.headers = network.headers.merging(
37+
["Authorization": "Bearer token-\(writer)-\(i)"]) { _, new in new }
38+
}
39+
}
40+
}
41+
42+
// readers: copy headers into a request, like NetworkingClient+Requests
43+
for _ in 0..<4 {
44+
queue.async(group: group) {
45+
for _ in 0..<iterations {
46+
let snapshot = network.headers
47+
_ = snapshot.count
48+
}
49+
}
50+
}
51+
52+
XCTAssertEqual(group.wait(timeout: .now() + 60), .success)
53+
XCTAssertEqual(network.headers["Client-OS"], "iOS")
54+
}
55+
56+
// withHeaders must be atomic: concurrent read-modify-writes through it must
57+
// never lose updates, which get-modify-set on the plain property can.
58+
func testWithHeadersIsAtomic() {
59+
let network = NetworkingClient(baseURL: "https://mocked.com")
60+
network.headers = ["counter": "0"]
61+
62+
let totalIncrements = 8_000
63+
DispatchQueue.concurrentPerform(iterations: totalIncrements) { _ in
64+
network.withHeaders { headers in
65+
let current = Int(headers["counter"] ?? "0") ?? 0
66+
headers["counter"] = String(current + 1)
67+
}
68+
}
69+
70+
XCTAssertEqual(network.headers["counter"], String(totalIncrements))
71+
}
72+
73+
// The property must keep behaving like the stored `var` it replaced.
74+
func testHeadersPropertyKeepsValueSemantics() {
75+
let network = NetworkingClient(baseURL: "https://mocked.com")
76+
77+
network.headers["a"] = "1"
78+
XCTAssertEqual(network.headers["a"], "1")
79+
80+
var snapshot = network.headers
81+
snapshot["b"] = "2"
82+
XCTAssertNil(network.headers["b"], "mutating a copy must not affect the client")
83+
84+
network.headers = [:]
85+
XCTAssertTrue(network.headers.isEmpty)
86+
87+
let returned = network.withHeaders { headers -> Int in
88+
headers["c"] = "3"
89+
return headers.count
90+
}
91+
XCTAssertEqual(returned, 1)
92+
XCTAssertEqual(network.headers["c"], "3")
93+
}
94+
}

0 commit comments

Comments
 (0)