feat: urlsessionconfiguration change requestTimeout#6
Conversation
WalkthroughAdds a new public NetworkConfig API exposed on Flagsmith.shared, updates APIManager and SSEManager to recreate URLSession instances from NetworkConfig, and includes example usage, documentation, and unit/integration tests validating propagation and timeout behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App
participant FS as Flagsmith.shared
participant API as APIManager
participant SSE as SSEManager
participant URLS as URLSession
rect rgba(235,245,255,0.6)
App->>FS: set networkConfig (e.g., requestTimeout)
Note right of FS: thread-safe store of NetworkConfig
end
par REST request
App->>API: performRequest(...)
API->>API: createURLSessionConfiguration(from: FS.networkConfig)
API->>URLS: create URLSession(configuration)
API->>URLS: dataTask(with: request)
URLS-->>API: response / error
API-->>App: result
and SSE stream
App->>SSE: start(...)
SSE->>SSE: createURLSessionConfiguration(from: FS.networkConfig)
SSE->>URLS: create URLSession(configuration)
SSE->>URLS: dataTask(with: stream URL)
URLS-->>SSE: events / error
SSE-->>App: callbacks
end
Note over API,SSE: Sessions are recreated to reflect current NetworkConfig before use
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
FlagsmithClient/Tests/NetworkConfigTests.swift (3)
45-49: Fix lint: short identifier 'i'Rename loop variable to satisfy swiftlint.
- for i in 0..<10 { + for idx in 0..<10 { DispatchQueue.global().async { // Test that we can safely set values concurrently - flagsmith.networkConfig.requestTimeout = Double(i) + flagsmith.networkConfig.requestTimeout = Double(idx) expectation.fulfill() } }
1-1: Remove leading blank line (swiftlint: leading_whitespace)Top-of-file leading whitespace violates lint rules.
- +// (no leading blank line)
80-94: Equality test is too narrow if more fields are addedThis compares only requestTimeout. If NetworkConfig gains more properties (as docs/example show), consider Equatable on NetworkConfig to compare all fields, or adjust the test name to reflect single-field comparison.
FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift (5)
1-1: Remove leading blank line (swiftlint: leading_whitespace)- +// (no leading blank line)
45-50: Replace unused closure parameter with underscore (swiftlint)- apiManager.request(.getFlags) { result in + apiManager.request(.getFlags) { _ in // Check the session configuration inside the completion handler let sessionConfig = apiManager.session.configuration XCTAssertEqual(sessionConfig.timeoutIntervalForRequest, 1.0, "Request timeout should be applied") expectation.fulfill() }
68-70: Replace unused closure parameter with underscore (swiftlint)- sseManager.start { result in + sseManager.start { _ in // This will be called immediately when start() is called }
89-91: Replace unused closure parameters with underscore (swiftlint)- apiManager.request(.getFlags) { result in + apiManager.request(.getFlags) { _ in initialExpectation.fulfill() } ... - apiManager.request(.getFlags) { result in + apiManager.request(.getFlags) { _ in expectation.fulfill() }Also applies to: 103-105
120-122: Fix lint: short identifier 'i' in performance loop- for i in 0..<100 { - flagsmith.networkConfig.requestTimeout = Double(i) + for iteration in 0..<100 { + flagsmith.networkConfig.requestTimeout = Double(iteration) }Example/NetworkConfigExample.swift (2)
45-55: Replace unused closure parameter with underscore (swiftlint)- flagsmith.getFeatureFlags { result in + flagsmith.getFeatureFlags { _ in // Handle result... }(Apply in all indicated closures)
Also applies to: 95-98, 110-113, 125-128
1-1: Remove leading blank line (swiftlint: leading_whitespace)- +// (no leading blank line)FlagsmithClient/Classes/Internal/SSEManager.swift (2)
189-196: Consider invalidating the previous session before swapRepeatedly recreating sessions without invalidating the old one can accumulate resources during reconnects.
- // Update session using the property setter to ensure thread-safe access - self.session = newSession + // Invalidate previous session before swapping to avoid resource buildup + let oldSession = self.session + self.session = newSession + oldSession.invalidateAndCancel()Make sure no tasks are still needed on the old session when invalidating.
202-205: Nil out dataTask on stopMinor cleanup to release references.
func stop() { - dataTask?.cancel() - completionHandler = nil + dataTask?.cancel() + dataTask = nil + completionHandler = nil }FlagsmithClient/Classes/Internal/APIManager.swift (2)
188-200: Recreating a URLSession per request is costlyCreating a new session on every request can add overhead and increase connection churn. Prefer reusing a session and only recreating when a config “generation” changes.
Consider tracking a configVersion on Flagsmith/NetworkConfig and only rebuilding when it changes, or cache a session keyed by effective NetworkConfig+CacheConfig.
I can propose a lightweight “generation” mechanism if desired.
188-200: Optionally invalidate previous session before swapTo avoid resource buildup during rapid requests:
- let newSession = URLSession(configuration: newConfig, delegate: self, delegateQueue: OperationQueue.main) - - // Update session using the property setter to ensure thread-safe access - self.session = newSession + let newSession = URLSession(configuration: newConfig, delegate: self, delegateQueue: OperationQueue.main) + let oldSession = self.session + self.session = newSession + // Invalidate the old session if no tasks are expected to keep running + oldSession.invalidateAndCancel()Ensure no active tasks depend on the old session when invalidating.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Example/NetworkConfigExample.swift(1 hunks)FlagsmithClient/Classes/Flagsmith.swift(2 hunks)FlagsmithClient/Classes/Internal/APIManager.swift(3 hunks)FlagsmithClient/Classes/Internal/SSEManager.swift(3 hunks)FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift(1 hunks)FlagsmithClient/Tests/NetworkConfigTests.swift(1 hunks)NETWORK_CONFIG_README.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
FlagsmithClient/Classes/Internal/SSEManager.swift (1)
FlagsmithClient/Classes/Internal/APIManager.swift (5)
createURLSessionConfiguration(78-95)createURLSessionConfiguration(98-108)request(163-203)request(210-219)request(227-249)
Example/NetworkConfigExample.swift (1)
FlagsmithClient/Classes/Flagsmith.swift (1)
getFeatureFlags(139-184)
FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift (2)
FlagsmithClient/Classes/Internal/APIManager.swift (3)
request(163-203)request(210-219)request(227-249)FlagsmithClient/Classes/Internal/SSEManager.swift (2)
start(173-200)stop(202-205)
FlagsmithClient/Classes/Internal/APIManager.swift (1)
FlagsmithClient/Classes/Internal/SSEManager.swift (2)
createURLSessionConfiguration(83-97)createURLSessionConfiguration(100-107)
🪛 GitHub Check: swift-lint
Example/NetworkConfigExample.swift
[warning] 125-125:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 110-110:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 95-95:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 1-1:
Leading Whitespace Violation: Files should not contain leading whitespace (leading_whitespace)
FlagsmithClient/Tests/NetworkConfigTests.swift
[failure] 45-45:
Identifier Name Violation: Variable name 'i' should be between 3 and 40 characters long (identifier_name)
FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift
[warning] 103-103:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 89-89:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 68-68:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 45-45:
Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)
[warning] 1-1:
Leading Whitespace Violation: Files should not contain leading whitespace (leading_whitespace)
[failure] 120-120:
Identifier Name Violation: Variable name 'i' should be between 3 and 40 characters long (identifier_name)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
| /// Configuration class for network settings | ||
| private var _networkConfig: NetworkConfig = .init() | ||
| public var networkConfig: NetworkConfig { | ||
| get { | ||
| apiManager.propertiesSerialAccessQueue.sync { _networkConfig } | ||
| } | ||
| set { | ||
| apiManager.propertiesSerialAccessQueue.sync { | ||
| _networkConfig = newValue | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Exposing a mutable reference is not thread-safe
networkConfig returns a shared reference to a class with unsynchronized vars. Concurrent mutations (as tested) create data races. Make NetworkConfig internally synchronized or expose a value-type snapshot API.
Follow the NetworkConfig refactor below to make properties thread-safe. This keeps the external API intact while preventing races.
🤖 Prompt for AI Agents
In FlagsmithClient/Classes/Flagsmith.swift around lines 114–125, returning a
shared mutable NetworkConfig instance causes data races because its properties
are unsynchronized; replace this by exposing a thread-safe snapshot or by making
NetworkConfig itself thread-safe: either (A) change networkConfig to return a
value-type snapshot (e.g. a struct copy or a deep-cloned immutable copy) from
inside apiManager.propertiesSerialAccessQueue.sync, or (B) make NetworkConfig
encapsulate its mutable state with its own serial queue and provide thread-safe
getters/setters for each property, then keep the getter/setter on Flagsmith
simply delegating through the queue. Ensure all external mutations occur via the
synchronized API (or by setting a whole new immutable value) so no shared,
unsynchronized class instance is exposed.
| /// Configuration class for network settings | ||
| public final class NetworkConfig { | ||
| /// The timeout interval for URL requests, in seconds. Default is 60.0 seconds. | ||
| public var requestTimeout: TimeInterval = 60.0 | ||
| } |
There was a problem hiding this comment.
Implement full NetworkConfig (thread-safe) to match docs/example
Add missing properties and make them thread-safe. Also mark as Sendable if accessed across concurrency domains.
-public final class NetworkConfig {
- /// The timeout interval for URL requests, in seconds. Default is 60.0 seconds.
- public var requestTimeout: TimeInterval = 60.0
-}
+public final class NetworkConfig: @unchecked Sendable {
+ private let queue = DispatchQueue(label: "flagsmith.networkConfig.queue", attributes: .concurrent)
+
+ // Backing storage
+ private var _requestTimeout: TimeInterval = 60.0
+ private var _resourceTimeout: TimeInterval = 604_800.0 // 7 days
+ private var _waitsForConnectivity: Bool = true
+ private var _allowsCellularAccess: Bool = true
+ private var _httpMaximumConnectionsPerHost: Int = 6
+ private var _httpAdditionalHeaders: [String: String] = [:]
+ private var _httpShouldUsePipelining: Bool = true
+ private var _httpShouldSetCookies: Bool = true
+
+ public init() {}
+
+ /// Timeout for individual requests (seconds). Default 60.0
+ public var requestTimeout: TimeInterval {
+ get { queue.sync { _requestTimeout } }
+ set { queue.async(flags: .barrier) { self._requestTimeout = newValue } }
+ }
+ /// Total timeout for a resource request (seconds). Default 7 days
+ public var resourceTimeout: TimeInterval {
+ get { queue.sync { _resourceTimeout } }
+ set { queue.async(flags: .barrier) { self._resourceTimeout = newValue } }
+ }
+ public var waitsForConnectivity: Bool {
+ get { queue.sync { _waitsForConnectivity } }
+ set { queue.async(flags: .barrier) { self._waitsForConnectivity = newValue } }
+ }
+ public var allowsCellularAccess: Bool {
+ get { queue.sync { _allowsCellularAccess } }
+ set { queue.async(flags: .barrier) { self._allowsCellularAccess = newValue } }
+ }
+ public var httpMaximumConnectionsPerHost: Int {
+ get { queue.sync { _httpMaximumConnectionsPerHost } }
+ set { queue.async(flags: .barrier) { self._httpMaximumConnectionsPerHost = newValue } }
+ }
+ public var httpAdditionalHeaders: [String: String] {
+ get { queue.sync { _httpAdditionalHeaders } }
+ set { queue.async(flags: .barrier) { self._httpAdditionalHeaders = newValue } }
+ }
+ public var httpShouldUsePipelining: Bool {
+ get { queue.sync { _httpShouldUsePipelining } }
+ set { queue.async(flags: .barrier) { self._httpShouldUsePipelining = newValue } }
+ }
+ public var httpShouldSetCookies: Bool {
+ get { queue.sync { _httpShouldSetCookies } }
+ set { queue.async(flags: .barrier) { self._httpShouldSetCookies = newValue } }
+ }
+}Additionally, ensure APIManager/SSEManager apply all of these properties when creating configurations (see file-specific suggestions).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Configuration class for network settings | |
| public final class NetworkConfig { | |
| /// The timeout interval for URL requests, in seconds. Default is 60.0 seconds. | |
| public var requestTimeout: TimeInterval = 60.0 | |
| } | |
| /// Configuration class for network settings | |
| public final class NetworkConfig: @unchecked Sendable { | |
| private let queue = DispatchQueue(label: "flagsmith.networkConfig.queue", attributes: .concurrent) | |
| // Backing storage | |
| private var _requestTimeout: TimeInterval = 60.0 | |
| private var _resourceTimeout: TimeInterval = 604_800.0 // 7 days | |
| private var _waitsForConnectivity: Bool = true | |
| private var _allowsCellularAccess: Bool = true | |
| private var _httpMaximumConnectionsPerHost: Int = 6 | |
| private var _httpAdditionalHeaders: [String: String] = [:] | |
| private var _httpShouldUsePipelining: Bool = true | |
| private var _httpShouldSetCookies: Bool = true | |
| public init() {} | |
| /// Timeout for individual requests (seconds). Default 60.0 | |
| public var requestTimeout: TimeInterval { | |
| get { queue.sync { _requestTimeout } } | |
| set { queue.async(flags: .barrier) { self._requestTimeout = newValue } } | |
| } | |
| /// Total timeout for a resource request (seconds). Default 7 days | |
| public var resourceTimeout: TimeInterval { | |
| get { queue.sync { _resourceTimeout } } | |
| set { queue.async(flags: .barrier) { self._resourceTimeout = newValue } } | |
| } | |
| public var waitsForConnectivity: Bool { | |
| get { queue.sync { _waitsForConnectivity } } | |
| set { queue.async(flags: .barrier) { self._waitsForConnectivity = newValue } } | |
| } | |
| public var allowsCellularAccess: Bool { | |
| get { queue.sync { _allowsCellularAccess } } | |
| set { queue.async(flags: .barrier) { self._allowsCellularAccess = newValue } } | |
| } | |
| public var httpMaximumConnectionsPerHost: Int { | |
| get { queue.sync { _httpMaximumConnectionsPerHost } } | |
| set { queue.async(flags: .barrier) { self._httpMaximumConnectionsPerHost = newValue } } | |
| } | |
| public var httpAdditionalHeaders: [String: String] { | |
| get { queue.sync { _httpAdditionalHeaders } } | |
| set { queue.async(flags: .barrier) { self._httpAdditionalHeaders = newValue } } | |
| } | |
| public var httpShouldUsePipelining: Bool { | |
| get { queue.sync { _httpShouldUsePipelining } } | |
| set { queue.async(flags: .barrier) { self._httpShouldUsePipelining = newValue } } | |
| } | |
| public var httpShouldSetCookies: Bool { | |
| get { queue.sync { _httpShouldSetCookies } } | |
| set { queue.async(flags: .barrier) { self._httpShouldSetCookies = newValue } } | |
| } | |
| } |
| /// Creates a URLSessionConfiguration with specific network and cache settings | ||
| private func createURLSessionConfiguration(networkConfig: NetworkConfig, cacheConfig: CacheConfig) -> URLSessionConfiguration { | ||
| let configuration = URLSessionConfiguration.default | ||
|
|
||
| // Apply network configuration | ||
| configuration.timeoutIntervalForRequest = networkConfig.requestTimeout | ||
|
|
||
| // Apply cache configuration | ||
| configuration.urlCache = cacheConfig.cache | ||
|
|
||
| return configuration | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply full NetworkConfig and CacheConfig when creating session config
Only requestTimeout/urlCache are applied. Propagate all documented settings for consistency.
- private func createURLSessionConfiguration(networkConfig: NetworkConfig, cacheConfig: CacheConfig) -> URLSessionConfiguration {
+ private func createURLSessionConfiguration(networkConfig: NetworkConfig, cacheConfig: CacheConfig) -> URLSessionConfiguration {
let configuration = URLSessionConfiguration.default
// Apply network configuration
- configuration.timeoutIntervalForRequest = networkConfig.requestTimeout
+ configuration.timeoutIntervalForRequest = networkConfig.requestTimeout
+ configuration.timeoutIntervalForResource = networkConfig.resourceTimeout
+ configuration.waitsForConnectivity = networkConfig.waitsForConnectivity
+ configuration.allowsCellularAccess = networkConfig.allowsCellularAccess
+ configuration.httpMaximumConnectionsPerHost = networkConfig.httpMaximumConnectionsPerHost
+ configuration.httpAdditionalHeaders = networkConfig.httpAdditionalHeaders as [AnyHashable: Any]
+ configuration.httpShouldUsePipelining = networkConfig.httpShouldUsePipelining
+ configuration.httpShouldSetCookies = networkConfig.httpShouldSetCookies
// Apply cache configuration
configuration.urlCache = cacheConfig.cache
return configuration
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Creates a URLSessionConfiguration with specific network and cache settings | |
| private func createURLSessionConfiguration(networkConfig: NetworkConfig, cacheConfig: CacheConfig) -> URLSessionConfiguration { | |
| let configuration = URLSessionConfiguration.default | |
| // Apply network configuration | |
| configuration.timeoutIntervalForRequest = networkConfig.requestTimeout | |
| // Apply cache configuration | |
| configuration.urlCache = cacheConfig.cache | |
| return configuration | |
| } | |
| /// Creates a URLSessionConfiguration with specific network and cache settings | |
| private func createURLSessionConfiguration(networkConfig: NetworkConfig, cacheConfig: CacheConfig) -> URLSessionConfiguration { | |
| let configuration = URLSessionConfiguration.default | |
| // Apply network configuration | |
| configuration.timeoutIntervalForRequest = networkConfig.requestTimeout | |
| configuration.timeoutIntervalForResource = networkConfig.resourceTimeout | |
| configuration.waitsForConnectivity = networkConfig.waitsForConnectivity | |
| configuration.allowsCellularAccess = networkConfig.allowsCellularAccess | |
| configuration.httpMaximumConnectionsPerHost = networkConfig.httpMaximumConnectionsPerHost | |
| configuration.httpAdditionalHeaders = networkConfig.httpAdditionalHeaders as [AnyHashable: Any] | |
| configuration.httpShouldUsePipelining = networkConfig.httpShouldUsePipelining | |
| configuration.httpShouldSetCookies = networkConfig.httpShouldSetCookies | |
| // Apply cache configuration | |
| configuration.urlCache = cacheConfig.cache | |
| return configuration | |
| } |
🤖 Prompt for AI Agents
In FlagsmithClient/Classes/Internal/APIManager.swift around lines 97 to 108, the
URLSessionConfiguration only applies requestTimeout and urlCache; update the
method to propagate all relevant NetworkConfig and CacheConfig fields: set
timeoutIntervalForRequest and timeoutIntervalForResource, waitsForConnectivity,
allowsCellularAccess, httpMaximumConnectionsPerHost, networkServiceType,
requestCachePolicy and httpAdditionalHeaders from NetworkConfig as available,
and set configuration.urlCache plus configuration.requestCachePolicy (and any
cache-specific timeouts/limits) from CacheConfig; ensure any optional values are
conditionally applied and preserve existing defaults when fields are nil.
| /// Creates a URLSessionConfiguration with specific network settings | ||
| private func createURLSessionConfiguration(networkConfig: NetworkConfig) -> URLSessionConfiguration { | ||
| let configuration = URLSessionConfiguration.default | ||
|
|
||
| // Apply network configuration | ||
| configuration.timeoutIntervalForRequest = networkConfig.requestTimeout | ||
|
|
||
| return configuration | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply full NetworkConfig when creating session config
Currently only requestTimeout is propagated. Apply the rest for consistency with docs/example.
- private func createURLSessionConfiguration(networkConfig: NetworkConfig) -> URLSessionConfiguration {
+ private func createURLSessionConfiguration(networkConfig: NetworkConfig) -> URLSessionConfiguration {
let configuration = URLSessionConfiguration.default
// Apply network configuration
- configuration.timeoutIntervalForRequest = networkConfig.requestTimeout
+ configuration.timeoutIntervalForRequest = networkConfig.requestTimeout
+ configuration.timeoutIntervalForResource = networkConfig.resourceTimeout
+ configuration.waitsForConnectivity = networkConfig.waitsForConnectivity
+ configuration.allowsCellularAccess = networkConfig.allowsCellularAccess
+ configuration.httpMaximumConnectionsPerHost = networkConfig.httpMaximumConnectionsPerHost
+ configuration.httpAdditionalHeaders = networkConfig.httpAdditionalHeaders as [AnyHashable: Any]
+ configuration.httpShouldUsePipelining = networkConfig.httpShouldUsePipelining
+ configuration.httpShouldSetCookies = networkConfig.httpShouldSetCookies
return configuration
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Creates a URLSessionConfiguration with specific network settings | |
| private func createURLSessionConfiguration(networkConfig: NetworkConfig) -> URLSessionConfiguration { | |
| let configuration = URLSessionConfiguration.default | |
| // Apply network configuration | |
| configuration.timeoutIntervalForRequest = networkConfig.requestTimeout | |
| return configuration | |
| } | |
| /// Creates a URLSessionConfiguration with specific network settings | |
| private func createURLSessionConfiguration(networkConfig: NetworkConfig) -> URLSessionConfiguration { | |
| let configuration = URLSessionConfiguration.default | |
| // Apply network configuration | |
| configuration.timeoutIntervalForRequest = networkConfig.requestTimeout | |
| configuration.timeoutIntervalForResource = networkConfig.resourceTimeout | |
| configuration.waitsForConnectivity = networkConfig.waitsForConnectivity | |
| configuration.allowsCellularAccess = networkConfig.allowsCellularAccess | |
| configuration.httpMaximumConnectionsPerHost = networkConfig.httpMaximumConnectionsPerHost | |
| configuration.httpAdditionalHeaders = networkConfig.httpAdditionalHeaders as [AnyHashable: Any] | |
| configuration.httpShouldUsePipelining = networkConfig.httpShouldUsePipelining | |
| configuration.httpShouldSetCookies = networkConfig.httpShouldSetCookies | |
| return configuration | |
| } |
🤖 Prompt for AI Agents
In FlagsmithClient/Classes/Internal/SSEManager.swift around lines 99 to 107, the
URLSessionConfiguration only sets requestTimeout; update the function to
propagate the rest of NetworkConfig onto the configuration object. Map
NetworkConfig fields such as timeoutIntervalForResource (resource timeout),
waitsForConnectivity, allowsCellularAccess, allowsExpensiveNetworkAccess,
allowsConstrainedNetworkAccess, httpAdditionalHeaders (or headers) and any other
relevant network flags onto the URLSessionConfiguration so the session honors
the full NetworkConfig used elsewhere in docs/examples.
| for i in 0..<10 { | ||
| DispatchQueue.global().async { | ||
| // Test that we can safely set values concurrently | ||
| flagsmith.networkConfig.requestTimeout = Double(i) | ||
| expectation.fulfill() | ||
| } | ||
| } |
There was a problem hiding this comment.
Test asserts thread-safety that code does not guarantee
flagsmith.networkConfig.requestTimeout = Double(i) mutates a shared reference without synchronization; NetworkConfig is not thread-safe. This test may pass nondeterministically, masking a data race. Either make NetworkConfig thread-safe (preferred) or drop this test’s concurrency claim.
Apply thread-safety as proposed in the NetworkConfig class comment; this test will then legitimately exercise concurrent writes.
🧰 Tools
🪛 GitHub Check: swift-lint
[failure] 45-45:
Identifier Name Violation: Variable name 'i' should be between 3 and 40 characters long (identifier_name)
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/NetworkConfigTests.swift around lines 45 to 51, the
test concurrently mutates flagsmith.networkConfig.requestTimeout but
NetworkConfig is not thread-safe; either remove the concurrent writes or
(preferred) make NetworkConfig thread-safe as the class comment suggests. Fix by
adding synchronization to NetworkConfig (e.g., a private serial DispatchQueue or
an NSLock) and wrap all mutable property getters/setters (including
requestTimeout) in that lock/queue so concurrent reads/writes are safe, update
any internal state accesses similarly, and then keep the concurrent test as-is.
| // Test negative timeout values (should be allowed as URLSessionConfiguration accepts them) | ||
| networkConfig.requestTimeout = -1.0 | ||
| XCTAssertEqual(networkConfig.requestTimeout, -1.0) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify negative timeout semantics
Docs often treat 0 as “no timeout”; negative intervals may be undefined. Please confirm before codifying acceptance of negatives.
🌐 Web query:
Does URLSessionConfiguration.timeoutIntervalForRequest support negative values, or should 0 be used to indicate no timeout?
💡 Result:
Short answer: Don’t use negative values — they’re not documented or supported. Apple documents timeoutIntervalForRequest as a TimeInterval (seconds) with a default of 60 and does not define negative semantics. If you need “no timeout”, avoid relying on undocumented negative behavior; instead use a very large timeout (or manage the lifetime yourself). See Apple’s URLSessionConfiguration docs and the Developer Forums thread explaining timeout semantics. [1][2]
Sources
- URLSessionConfiguration.timeoutIntervalForRequest (Apple docs). [1]
- Apple Developer Forums: explanation of timeoutInterval / timeoutIntervalForRequest semantics. [2]
Disallow negative timeout values—clamp to ≥0
URLSessionConfiguration.timeoutIntervalForRequest doesn’t support negative intervals. Update your setter to clamp negative inputs to 0 (or throw) and adjust/remove the negative-timeout test in FlagsmithClient/Tests/NetworkConfigTests.swift (lines 67–70).
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/NetworkConfigTests.swift around lines 67 to 70, the
test currently allows a negative requestTimeout value but
URLSessionConfiguration does not support negative intervals; update the
NetworkConfig.requestTimeout setter to clamp negative values to 0 (or
alternatively throw), and then change the test to assert the clamped value
(XCTAssertEqual(networkConfig.requestTimeout, 0.0)) or remove the
negative-timeout test entirely to reflect the new behavior.
| The `NetworkConfig` class exposes the following commonly used `URLSessionConfiguration` parameters: | ||
|
|
||
| - **Request Timeout**: `requestTimeout` - Timeout for individual requests (default: 60.0 seconds) | ||
| - **Resource Timeout**: `resourceTimeout` - Total timeout for the entire resource request (default: 604800.0 seconds / 7 days) | ||
| - **Connectivity**: `waitsForConnectivity` - Whether to wait for connectivity (default: true) | ||
| - **Cellular Access**: `allowsCellularAccess` - Whether to allow cellular access (default: true) | ||
| - **Connection Limits**: `httpMaximumConnectionsPerHost` - Max concurrent connections per host (default: 6) | ||
| - **Custom Headers**: `httpAdditionalHeaders` - Additional HTTP headers to send with requests (default: empty) | ||
| - **HTTP Pipelining**: `httpShouldUsePipelining` - Whether to use HTTP pipelining (default: true) | ||
| - **HTTP Cookies**: `httpShouldSetCookies` - Whether to automatically set cookies (default: true) | ||
|
|
There was a problem hiding this comment.
Docs list properties not implemented in NetworkConfig
resourceTimeout, waitsForConnectivity, allowsCellularAccess, httpMaximumConnectionsPerHost, httpAdditionalHeaders, httpShouldUsePipelining, httpShouldSetCookies are documented but not present in the code. Example code won’t compile as-is.
Either implement these properties (recommended; see proposed NetworkConfig diff) and apply them in APIManager/SSEManager, or update docs/example to only reflect supported fields.
🤖 Prompt for AI Agents
In NETWORK_CONFIG_README.md around lines 7–17, the docs list NetworkConfig
properties (resourceTimeout, waitsForConnectivity, allowsCellularAccess,
httpMaximumConnectionsPerHost, httpAdditionalHeaders, httpShouldUsePipelining,
httpShouldSetCookies) that are not implemented causing example code to not
compile; fix this by adding those properties to the NetworkConfig type with the
documented defaults and updating the places that create URLSessionConfiguration
(APIManager and SSEManager) to apply each value to the session configuration
(set timeoutIntervalForResource, waitsForConnectivity, allowsCellularAccess,
httpMaximumConnectionsPerHost, httpAdditionalHeaders, httpShouldUsePipelining,
httpShouldSetCookies) or, if you prefer not to implement, update the README and
example to remove these fields and document only the supported properties so
docs and code match.
| ## Thread Safety | ||
|
|
||
| The `NetworkConfig` is thread-safe and can be modified from any thread. Changes to the configuration will automatically trigger the recreation of the underlying `URLSession` with the new settings. | ||
|
|
There was a problem hiding this comment.
Thread-safety claim is inaccurate
NetworkConfig is currently not thread-safe. Update the implementation to synchronize access (recommended), or remove this guarantee from the docs.
🤖 Prompt for AI Agents
In NETWORK_CONFIG_README.md around lines 113 to 116, the doc incorrectly states
NetworkConfig is thread-safe; update the implementation to actually synchronize
access or remove the claim. To fix, add a concurrency primitive (e.g., a private
serial DispatchQueue or an NSLock/RWLock) and wrap all mutations and reads of
the config properties and the URLSession recreation logic with that lock/queue
so state and session lifecycle are atomic; ensure session recreation uses a
snapshot of the config inside the lock and publish any external notifications on
the appropriate queue (main if UI); alternatively, if you prefer not to change
code, remove the thread-safety sentence from the README and clearly state the
object is not thread-safe and must be accessed from a single thread.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
FlagsmithClient/Tests/NetworkConfigTests.swift (1)
66-76: Do not accept negative request timeouts.
URLSessionConfiguration.timeoutIntervalForRequesttreats negative values as invalid. AllowingnetworkConfig.requestTimeout = -1.0will hand a negative interval to the session, which is undefined behaviour and can result in the request timing out immediately or the configuration being rejected. Clamp to>= 0(or throw) inNetworkConfigand adjust this test to assert the clamped value instead of expecting-1.0.- networkConfig.requestTimeout = -1.0 - XCTAssertEqual(networkConfig.requestTimeout, -1.0) + networkConfig.requestTimeout = -1.0 + XCTAssertEqual(networkConfig.requestTimeout, 0.0)FlagsmithClient/Classes/Internal/APIManager.swift (1)
97-106: Preserve the default network configuration when recreating sessions.
createURLSessionConfiguration()initialises the session with waitsForConnectivity, timeoutIntervalForResource, httpShouldUsePipelining, etc. As soon as the first request runs, you rebuild the session viacreateURLSessionConfiguration(networkConfig:cacheConfig:)which only copiesrequestTimeout. All of the other defaults silently revert to Apple’s defaults (e.g. waitsForConnectivity becomesfalse), changing behaviour at runtime. Mirror the same defaults in this overload (or move the shared configuration into a helper) so subsequent sessions remain configured identically apart from the overridden timeout.private func createURLSessionConfiguration(networkConfig: NetworkConfig, cacheConfig: CacheConfig) -> URLSessionConfiguration { let configuration = URLSessionConfiguration.default // Apply network configuration configuration.timeoutIntervalForRequest = networkConfig.requestTimeout + configuration.timeoutIntervalForResource = 604800.0 + configuration.waitsForConnectivity = true + configuration.allowsCellularAccess = true + configuration.httpMaximumConnectionsPerHost = 6 + configuration.httpAdditionalHeaders = [:] + configuration.httpShouldUsePipelining = true + configuration.httpShouldSetCookies = true // Apply cache configuration configuration.urlCache = cacheConfig.cache
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Example/NetworkConfigExample.swift(1 hunks)FlagsmithClient/Classes/Internal/APIManager.swift(3 hunks)FlagsmithClient/Classes/Internal/SSEManager.swift(3 hunks)FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift(1 hunks)FlagsmithClient/Tests/NetworkConfigTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
FlagsmithClient/Classes/Internal/SSEManager.swift (1)
FlagsmithClient/Classes/Internal/APIManager.swift (5)
createURLSessionConfiguration(78-95)createURLSessionConfiguration(98-108)request(163-205)request(212-221)request(229-251)
FlagsmithClient/Classes/Internal/APIManager.swift (1)
FlagsmithClient/Classes/Internal/SSEManager.swift (2)
createURLSessionConfiguration(83-97)createURLSessionConfiguration(100-107)
Example/NetworkConfigExample.swift (1)
FlagsmithClient/Classes/Flagsmith.swift (1)
getFeatureFlags(139-184)
FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift (2)
FlagsmithClient/Classes/Internal/APIManager.swift (3)
request(163-205)request(212-221)request(229-251)FlagsmithClient/Classes/Internal/SSEManager.swift (2)
start(173-202)stop(204-208)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/NetworkConfigTests.swift
[warning] 94-94:
Trailing Newline Violation: Files should have a single trailing newline (trailing_newline)
Example/NetworkConfigExample.swift
[warning] 76-76:
Trailing Newline Violation: Files should have a single trailing newline (trailing_newline)
FlagsmithClient/Tests/NetworkConfigIntegrationTests.swift
[warning] 121-121:
Trailing Newline Violation: Files should have a single trailing newline (trailing_newline)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
| // we must use the delegate form here, not the completion handler, to be able to modify the cache | ||
| serialAccessQueue.sync { | ||
| // Update session cache configuration if it has changed (must be done inside the serial queue) | ||
| if session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { | ||
| let configuration = URLSessionConfiguration.default | ||
| configuration.urlCache = Flagsmith.shared.cacheConfig.cache | ||
| session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main) | ||
| } | ||
| // Always recreate session with current network and cache configuration | ||
| // This ensures that any changes to network config are applied immediately | ||
| let networkConfig = Flagsmith.shared.networkConfig | ||
| let cacheConfig = Flagsmith.shared.cacheConfig | ||
|
|
||
| let newConfig = createURLSessionConfiguration(networkConfig: networkConfig, cacheConfig: cacheConfig) | ||
| let newSession = URLSession(configuration: newConfig, delegate: self, delegateQueue: OperationQueue.main) | ||
|
|
||
| // Invalidate previous session before swapping to avoid resource buildup | ||
| let oldSession = self.session | ||
| self.session = newSession | ||
| oldSession.invalidateAndCancel() | ||
|
|
||
| let task = session.dataTask(with: request) | ||
| let task = newSession.dataTask(with: request) | ||
| tasksToCompletionHandlers[task.taskIdentifier] = completion | ||
| task.resume() |
There was a problem hiding this comment.
Do not cancel in-flight requests when rebuilding the session.
Calling oldSession.invalidateAndCancel() here tears down any tasks still running on the previous session. If a second request arrives before the first completes, you cancel the earlier request mid-flight and return an error to its caller. Swap to finishTasksAndInvalidate() (or delay the swap until no tasks are active) so existing tasks finish before recreating the session.
- let oldSession = self.session
- self.session = newSession
- oldSession.invalidateAndCancel()
+ let oldSession = self.session
+ self.session = newSession
+ oldSession.finishTasksAndInvalidate()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // we must use the delegate form here, not the completion handler, to be able to modify the cache | |
| serialAccessQueue.sync { | |
| // Update session cache configuration if it has changed (must be done inside the serial queue) | |
| if session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { | |
| let configuration = URLSessionConfiguration.default | |
| configuration.urlCache = Flagsmith.shared.cacheConfig.cache | |
| session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main) | |
| } | |
| // Always recreate session with current network and cache configuration | |
| // This ensures that any changes to network config are applied immediately | |
| let networkConfig = Flagsmith.shared.networkConfig | |
| let cacheConfig = Flagsmith.shared.cacheConfig | |
| let newConfig = createURLSessionConfiguration(networkConfig: networkConfig, cacheConfig: cacheConfig) | |
| let newSession = URLSession(configuration: newConfig, delegate: self, delegateQueue: OperationQueue.main) | |
| // Invalidate previous session before swapping to avoid resource buildup | |
| let oldSession = self.session | |
| self.session = newSession | |
| oldSession.invalidateAndCancel() | |
| let task = session.dataTask(with: request) | |
| let task = newSession.dataTask(with: request) | |
| tasksToCompletionHandlers[task.taskIdentifier] = completion | |
| task.resume() | |
| // we must use the delegate form here, not the completion handler, to be able to modify the cache | |
| serialAccessQueue.sync { | |
| // Always recreate session with current network and cache configuration | |
| // This ensures that any changes to network config are applied immediately | |
| let networkConfig = Flagsmith.shared.networkConfig | |
| let cacheConfig = Flagsmith.shared.cacheConfig | |
| let newConfig = createURLSessionConfiguration(networkConfig: networkConfig, cacheConfig: cacheConfig) | |
| let newSession = URLSession(configuration: newConfig, delegate: self, delegateQueue: OperationQueue.main) | |
| // Invalidate previous session before swapping to avoid resource buildup | |
| let oldSession = self.session | |
| self.session = newSession | |
| oldSession.finishTasksAndInvalidate() | |
| let task = newSession.dataTask(with: request) | |
| tasksToCompletionHandlers[task.taskIdentifier] = completion | |
| task.resume() |
🤖 Prompt for AI Agents
In FlagsmithClient/Classes/Internal/APIManager.swift around lines 186-203, the
code currently calls oldSession.invalidateAndCancel(), which cancels any
in-flight tasks; change this to call oldSession.finishTasksAndInvalidate() so
existing tasks are allowed to complete before the session is torn down, or
alternatively delay swapping sessions until oldSession has no active tasks
(e.g., check oldSession.getAllTasks and wait/completion handler); ensure you
still assign self.session to the new session and resume the new task without
cancelling existing requests.
gazreese
changed the title
|
Hi @polatolu let's pause on this one for now, it looks like it's getting complicated |
2 participants
Description
Addresses a requirement for changing the requestTimeout parameter in URLSessionConfiguration
Regression Test Recommendations
Type of Change
🗑️ ChoreUnit Test Results
Summary by CodeRabbit
New Features
Documentation
Tests