Add thread-safe access to custom headers#1235
Conversation
| /// - Returns: A dictionary containing all custom headers. | ||
| public func getCustomHeaders() -> [String : String] { | ||
| customHeaderQueue.sync { | ||
| return customHeaders |
There was a problem hiding this comment.
The customHeaders property is public and still accessible out of the queue to be thread-safe. Could you change access modifier or declare some private property _customHeaders and make customHeader as computed one ?
There was a problem hiding this comment.
@mdanylov-sigma , I've updated the PR with the latter suggestion as that'll not break anyone directly referencing the customHeaders
Implement thread-safe custom headers to prevent race conditions when accessing headers from multiple threads. Uses a serial dispatch queue to synchronize all custom header operations. Changes: - Add customHeaderQueue dispatch queue for synchronization - Add private _customHeaders backing storage - Convert customHeaders to computed property with thread-safe getter/setter - Update addCustomHeader() to use sync for thread-safe writes - Update clearCustomHeaders() to use sync for thread-safe clearing - Add getCustomHeaders() method for safe external access - Update PrebidServerConnection to use getCustomHeaders() getter The computed property approach maintains backward compatibility while ensuring all access (direct or via methods) goes through the synchronization queue. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ankit-thanekar007
force-pushed
the
feature/thread-safe-custom-headers
branch
from
88350d1 to
b6c086a
Compare
YuriyVelichkoPI
left a comment
YuriyVelichkoPI
left a comment
There was a problem hiding this comment.
Please describe the problem you’re trying to solve with these changes.
With the current design, this property doesn’t need to be thread-safe. The expectation is that custom headers are set during SDK initialization, and then the SDK engine only reads them. Read operations are inherently thread-safe unless you’re mutating the data during the app lifecycle.
If you do need to update headers dynamically, it might indicate that we need a different API - for example, setting custom headers at the ad unit level rather than globally at the SDK level.
That said, you might be addressing some other edge cases I’m not aware of, so please describe the underlying problem first.
Hey @YuriyVelichkoPI , I've updated the PR description with the Problem we faced. Please let me know if you need more details. |
|
|
||
|
|
||
| /// Dispatch queue for thread-safe access to custom headers | ||
| let customHeaderQueue : DispatchQueue = DispatchQueue(label: "com.prebid.customHeaderQ") |
There was a problem hiding this comment.
Please don't mix private and public properties. Adhere the following sections in the class
- Public/Open properties and methods (the type's primary interface).
- Internal properties and methods (the default access level, used within the module).
- Fileprivate properties and methods (used within the same file).
- Private properties and methods (implementation details hidden from the rest of the code).
| /// Private backing storage for custom HTTP headers | ||
| private var _customHeaders: [String: String] = [:] | ||
|
|
||
| /// Custom HTTP headers to be sent with requests. |
There was a problem hiding this comment.
Please add a note in the comment that this property is thread-safe and implements sync access.
|
|
||
| /// Returns a copy of the current custom HTTP headers. | ||
| /// - Returns: A dictionary containing all custom headers. | ||
| public func getCustomHeaders() -> [String : String] { |
There was a problem hiding this comment.
What is the purpose of this method? Can you leave only the property?
|
|
||
| /// Returns a copy of the current custom HTTP headers. | ||
| /// - Returns: A dictionary containing all custom headers. | ||
| public func getCustomHeaders() -> [String : String] { |
There was a problem hiding this comment.
@ankit-thanekar007 do we really need the second getter for customHeaders? I guess we can use only customHeaders property, looks like the new method is redundant, what do you think ?
3 participants
Problem :
This PR fixes a thread safety issue that caused production crashes in some of our Apps. The crash occurred when
PrebidServerConnection.createRequest()accessedcustomHeadersfrom multiple dispatch queues concurrently.Currently the headers on our side can update dynamically. This can be triggered because of some privacy flags being updated by the user on desktop, or some other use cases.
Implement thread-safe custom headers to prevent race conditions when accessing headers from multiple threads. Uses a serial dispatch queue to synchronize all custom header operations.
Changes: