Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/cache logic #78

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Bugfix/cache logic #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf56936
Only call completion handler on not-modifier response, if not a cron …
maerki Dec 11, 2023
3703aea
Overwrite cache rule for all requests
maerki Dec 11, 2023
fab1e31
send both cached and new response headers to cache logic
maerki Dec 11, 2023
636babb
use now as reference date for all refresh tasks
maerki Dec 11, 2023
583a006
Log refresh scheduling
maerki Dec 11, 2023
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
37 changes: 31 additions & 6 deletions Sources/UBFoundation/Networking/AutoRefreshCacheLogic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
//

import Foundation
import OSLog

@available(iOS 14.0, watchOS 7.0, *)
fileprivate struct Log {
static let logger = Logger(subsystem: "UBKit", category: "AutoRefreshCacheLogic")
}

/// A caching logic that will launch and refresh the data automatically when the data expires
open class UBAutoRefreshCacheLogic: UBBaseCachingLogic {
Expand All @@ -26,10 +32,26 @@ open class UBAutoRefreshCacheLogic: UBBaseCachingLogic {
cancelRefreshCronJob(for: task)

guard let nextRefreshDate = cachedResponseNextRefreshDate(headers, metrics: metrics, referenceDate: referenceDate) else {
if #available(iOS 14.0, watchOS 7.0, *) {
Log.logger.trace("No refresh date for task \(task)")
}
return
}

if #available(iOS 14.0, watchOS 7.0, *) {
Log.logger.trace("Schedule refresh for \(task) at \(nextRefreshDate) (\(round(nextRefreshDate.timeIntervalSinceNow))s)")
}

// Schedule a new job
let job = UBCronJob(fireAt: nextRefreshDate, qos: qos) { [weak task] in
if #available(iOS 14.0, watchOS 7.0, *) {
if let task {
Log.logger.trace("Start cron refresh for task \(task)")
}
else {
Log.logger.trace("Not start cron refresh, task doesn't exist anymore.")
}
}
task?.start(flags: [.systemTriggered, .refresh])
}
refreshJobsAccess.sync {
Expand All @@ -42,7 +64,7 @@ open class UBAutoRefreshCacheLogic: UBBaseCachingLogic {
/// - Parameter allHeaderFields: The header fiealds.
/// - Returns: The next refresh date. `nil` if no next refresh date is available
open func cachedResponseNextRefreshDate(_ allHeaderFields: [AnyHashable: Any], metrics: URLSessionTaskMetrics?, referenceDate: Date?) -> Date? {
guard let responseDateHeader = allHeaderFields.getCaseInsensitiveValue(key: dateHeaderFieldName) as? String, let responseDate = referenceDate ?? dateFormatter.date(from: responseDateHeader) else {
guard let responseDateHeader = allHeaderFields.getCaseInsensitiveValue(key: dateHeaderFieldName) as? String, var responseDate = dateFormatter.date(from: responseDateHeader) else {
// If we cannot find a date in the response header then we cannot comput the next refresh date
return nil
}
Expand All @@ -65,13 +87,14 @@ open class UBAutoRefreshCacheLogic: UBBaseCachingLogic {
} else {
age = 0
}
responseDate = referenceDate ?? responseDate + age

// The backoff date is the response date added to the backoff interval
let backoffDate: Date
if let metrics = metrics, let date = metrics.transactionMetrics.last?.connectEndDate {
backoffDate = max(responseDate + age + backoffInterval, date + backoffInterval)
backoffDate = max(responseDate + backoffInterval, date + backoffInterval)
} else {
backoffDate = responseDate + age + backoffInterval
backoffDate = responseDate + backoffInterval
}

// Return the date that is the most in the future.
Expand All @@ -93,7 +116,8 @@ open class UBAutoRefreshCacheLogic: UBBaseCachingLogic {
if cachedURLResponse != nil ||
response == UBStandardHTTPCode.notModified {
// If there is a response or the response is not modified, reschedule the cron job
scheduleRefreshCronJob(for: ubDataTask, headers: response.allHeaderFields, metrics: metrics, referenceDate: Date())
let referenceDate = ubDataTask.flags.contains(.refresh) ? Date() : nil
scheduleRefreshCronJob(for: ubDataTask, headers: response.allHeaderFields, metrics: metrics, referenceDate: referenceDate)
} else {
// Otherwise cancel any current cron jobs
cancelRefreshCronJob(for: ubDataTask)
Expand All @@ -109,7 +133,8 @@ open class UBAutoRefreshCacheLogic: UBBaseCachingLogic {

/// :nodoc:

override public func hasUsed(response: HTTPURLResponse, metrics: URLSessionTaskMetrics?, request _: URLRequest, dataTask: UBURLDataTask) {
scheduleRefreshCronJob(for: dataTask, headers: response.allHeaderFields, metrics: metrics, referenceDate: nil)
override public func hasUsed(cachedResponse: HTTPURLResponse, nonModifiedResponse: HTTPURLResponse?, metrics: URLSessionTaskMetrics?, request _: URLRequest, dataTask: UBURLDataTask) {
let referenceDate = dataTask.flags.contains(.refresh) ? Date() : nil
scheduleRefreshCronJob(for: dataTask, headers: (nonModifiedResponse ?? cachedResponse).allHeaderFields, metrics: metrics, referenceDate: referenceDate)
}
}
6 changes: 5 additions & 1 deletion Sources/UBFoundation/Networking/BaseCachingLogic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ open class UBBaseCachingLogic: UBCachingLogic {
dateFormatter = df
}

public func prepareRequest(_ request: inout URLRequest) {
request.cachePolicy = .reloadIgnoringLocalCacheData
}
Comment on lines +43 to +45
Copy link

Choose a reason for hiding this comment

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

The new prepareRequest method correctly sets the cachePolicy to .reloadIgnoringLocalCacheData. Ensure that the implications of this change in caching behavior are well-documented and communicated to consumers of the UBBaseCachingLogic class.

maerki marked this conversation as resolved.
Show resolved Hide resolved

/// Gets a cached url response from a url session.
///
/// - Parameters:
Expand Down Expand Up @@ -269,7 +273,7 @@ open class UBBaseCachingLogic: UBCachingLogic {
}

/// :nodoc:
public func hasUsed(response _: HTTPURLResponse, metrics _: URLSessionTaskMetrics?, request _: URLRequest, dataTask _: UBURLDataTask) {
public func hasUsed(cachedResponse _: HTTPURLResponse, nonModifiedResponse _: HTTPURLResponse?, metrics _: URLSessionTaskMetrics?, request _: URLRequest, dataTask _: UBURLDataTask) {
// don't care, subclasses might
}

Expand Down
7 changes: 6 additions & 1 deletion Sources/UBFoundation/Networking/CachingLogic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ public enum UBCacheResult {

/// A caching logic object can provide decision when comes to requests and response that needs caching
public protocol UBCachingLogic {

/// Modify the request before starting
/// Allows to change the cache policy
func prepareRequest(_ request: inout URLRequest)

/// Asks the caching logic to provide a cached proposition.
///
/// Returning `nil` will indicate that the response should not be cached
Expand Down Expand Up @@ -75,7 +80,7 @@ public protocol UBCachingLogic {
func hasMissedCache(dataTask: UBURLDataTask)

/// Tell the caching logic that the result was used
func hasUsed(response: HTTPURLResponse, metrics: URLSessionTaskMetrics?, request _: URLRequest, dataTask: UBURLDataTask)
func hasUsed(cachedResponse: HTTPURLResponse, nonModifiedResponse: HTTPURLResponse?, metrics: URLSessionTaskMetrics?, request _: URLRequest, dataTask: UBURLDataTask)

/// Tell the caching logic that a new result was cached
func hasProposedCachedResponse(cachedURLResponse: CachedURLResponse?, response: HTTPURLResponse, session: URLSession, request: URLRequest, ubDataTask: UBURLDataTask, metrics: URLSessionTaskMetrics?)
Expand Down
2 changes: 1 addition & 1 deletion Sources/UBFoundation/Networking/UBURLRequest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public struct UBURLRequest: Equatable, Hashable, CustomReflectable, CustomString
/// - method: The HTTP Method to use. Default to GET.
/// - timeoutInterval: The timeout interval for the request. The default is 60.0.
public init(url: URL, method: UBHTTPMethod = .get, timeoutInterval: TimeInterval = 60.0) {
self.init(request: URLRequest(url: url, cachePolicy: .reloadIgnoringCacheData, timeoutInterval: timeoutInterval))
self.init(request: URLRequest(url: url, cachePolicy: .useProtocolCachePolicy, timeoutInterval: timeoutInterval))
httpMethod = method
}
Comment on lines 93 to 98
Copy link

Choose a reason for hiding this comment

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

💡 NOTE
Codebase verification is a beta feature.

The verification script has identified multiple instances in the codebase where UBURLRequest is used. Given the change to the default cache policy, it is recommended to audit these specific instances to ensure that the new default behavior does not introduce regressions or unexpected behavior. The instances found are in various source and test files, and each should be reviewed to confirm that the new cache policy is suitable or to make necessary adjustments.

  • Audit all instances of UBURLRequest found in the output to ensure compatibility with the new default cache policy .useProtocolCachePolicy.
Analysis chain

The change to the default cache policy in the UBURLRequest initializer from .reloadIgnoringCacheData to .useProtocolCachePolicy is a significant alteration to the caching behavior. Ensure that all usages of UBURLRequest throughout the codebase have been audited to confirm that this new default behavior does not introduce any regressions or unexpected behavior, especially in cases where the previous default cache policy was implicitly relied upon.

Given the absence of results from the script, it's not possible to confirm that the change to the default cache policy in UBURLRequest will not affect existing code. A broader search is recommended to ensure that all instances of UBURLRequest initialization are accounted for and reviewed for potential impacts due to the change in cache policy.

The absence of results from the broader search pattern still does not provide confirmation that the change to the default cache policy in UBURLRequest is safe. A different approach is needed to verify the usage of UBURLRequest throughout the codebase.


Expand Down
6 changes: 4 additions & 2 deletions Sources/UBFoundation/Networking/UBURLSession+Delegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,11 @@ class UBURLSessionDelegate: NSObject, URLSessionTaskDelegate, URLSessionDataDele
let request = task.currentRequest {
session.configuration.urlCache?.storeCachedResponse(newCachedResponse, for: request)
}
ubDataTask.dataTaskCompleted(data: cached.data, response: cached.response as? HTTPURLResponse, error: collectedData.error ?? error, info: info)
if !ubDataTask.flags.contains(.refresh) {
ubDataTask.dataTaskCompleted(data: cached.data, response: cached.response as? HTTPURLResponse, error: collectedData.error ?? error, info: info)
}
Comment on lines +115 to +117
Copy link

Choose a reason for hiding this comment

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

The new condition checks if the .refresh flag is not contained in ubDataTask.flags before completing the task with cached data. This change appears to be part of the caching logic bugfix. Ensure that this new behavior is documented and tested to confirm that it works as intended and does not introduce regressions.

The conditional check for the .refresh flag before completing the task with cached data is aligned with the PR's objective to fix caching logic. However, due to a script error, I couldn't verify if this behavior is covered by tests. Please ensure that this logic is well-documented and tested to prevent regressions.

The conditional check for the .refresh flag before completing the task with cached data is consistent with the PR's caching logic bugfix. However, the absence of script output prevents verification of test coverage for this new behavior. It is important to ensure that this logic is thoroughly tested to avoid regressions.

  • Please confirm that tests covering the new .refresh flag condition are present and adequately address the changes made.

if let response = cached.response as? HTTPURLResponse {
cachingLogic?.hasUsed(response: response, metrics: collectedData.metrics, request: collectedData.request, dataTask: ubDataTask)
cachingLogic?.hasUsed(cachedResponse: response, nonModifiedResponse: response, metrics: collectedData.metrics, request: collectedData.request, dataTask: ubDataTask)
}
return
}
Expand Down
6 changes: 4 additions & 2 deletions Sources/UBFoundation/Networking/UBURLSession.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public class UBURLSession: UBDataTaskURLSession {
public func dataTask(with request: UBURLRequest, owner: UBURLDataTask) -> URLSessionDataTask? {
// Creats and adds a data task to the delegate
func createTask(_ request: URLRequest, cachedResponse: CachedURLResponse? = nil) -> URLSessionDataTask? {
var request = request
sessionDelegate.cachingLogic?.prepareRequest(&request)
let sessionDataTask = urlSession.dataTask(with: request)
sessionDelegate.addTaskPair(key: sessionDataTask, value: owner, cachedResponse: cachedResponse)
return sessionDataTask
Expand Down Expand Up @@ -87,8 +89,8 @@ public class UBURLSession: UBDataTaskURLSession {

owner.dataTaskCompleted(data: cachedResponse.data, response: cachedResponse.response as? HTTPURLResponse, error: nil, info: info)
owner.completionHandlersDispatchQueue.sync {
if let response = cachedResponse.response as? HTTPURLResponse {
sessionDelegate.cachingLogic?.hasUsed(response: response, metrics: metrics, request: request.getRequest(), dataTask: owner)
if let cachedResponse = cachedResponse.response as? HTTPURLResponse {
sessionDelegate.cachingLogic?.hasUsed(cachedResponse: cachedResponse, nonModifiedResponse: nil, metrics: metrics, request: request.getRequest(), dataTask: owner)
}
}
return nil
Expand Down
Loading