-
Notifications
You must be signed in to change notification settings - Fork 84
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.
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
feat: Honor polling interval between restarts #355
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ jobs: | |
runs-on: ${{ matrix.os }} | ||
|
||
strategy: | ||
fail-fast: false | ||
matrix: | ||
include: | ||
- xcode-version: 15.0.1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,18 @@ protocol FeatureFlagCaching { | |
// sourcery: defaultMockValue = KeyedValueCachingMock() | ||
var keyedValueCache: KeyedValueCaching { get } | ||
|
||
func getCachedData(cacheKey: String) -> (items: StoredItems?, etag: String?) | ||
/// Retrieve all cached data for the given cache key. | ||
/// | ||
/// - parameter cacheKey: The unique key into the local cache store. | ||
/// - returns: Returns a tuple of cached value information. | ||
/// items: This is the associated flag evaluation results associated with this context. | ||
/// etag: The last known e-tag value from a polling request (see saveCachedData | ||
/// comments) for more informmation. | ||
/// lastUpdated: The date the cache was last considered up-to-date. If there are no cached | ||
/// values, this should return nil. | ||
/// | ||
/// | ||
func getCachedData(cacheKey: String) -> (items: StoredItems?, etag: String?, lastUpdated: Date?) | ||
|
||
// When we update the cache, we save the flag data and if we have it, an | ||
// etag. For polling, we should always have the flag data and an etag | ||
|
@@ -42,16 +53,24 @@ final class FeatureFlagCache: FeatureFlagCaching { | |
self.maxCachedContexts = maxCachedContexts | ||
} | ||
|
||
func getCachedData(cacheKey: String) -> (items: StoredItems?, etag: String?) { | ||
func getCachedData(cacheKey: String) -> (items: StoredItems?, etag: String?, lastUpdated: Date?) { | ||
guard let cachedFlagsData = keyedValueCache.data(forKey: "flags-\(cacheKey)"), | ||
let cachedFlags = try? JSONDecoder().decode(StoredItemCollection.self, from: cachedFlagsData) | ||
else { return (items: nil, etag: nil) } | ||
else { return (items: nil, etag: nil, lastUpdated: nil) } | ||
|
||
guard let cachedETagData = keyedValueCache.data(forKey: "etag-\(cacheKey)"), | ||
let etag = try? JSONDecoder().decode(String.self, from: cachedETagData) | ||
else { return (items: cachedFlags.flags, etag: nil) } | ||
else { return (items: cachedFlags.flags, etag: nil, lastUpdated: nil) } | ||
|
||
return (items: cachedFlags.flags, etag: etag) | ||
var cachedContexts: [String: Int64] = [:] | ||
if let cacheMetadata = keyedValueCache.data(forKey: "cached-contexts") { | ||
cachedContexts = (try? JSONDecoder().decode([String: Int64].self, from: cacheMetadata)) ?? [:] | ||
} | ||
|
||
guard let lastUpdated = cachedContexts[cacheKey] | ||
else { return (items: cachedFlags.flags, etag: etag, lastUpdated: nil) } | ||
|
||
return (items: cachedFlags.flags, etag: etag, lastUpdated: Date(timeIntervalSince1970: TimeInterval(lastUpdated / 1_000))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm here, the lastUpdate from the cache is millis, but the TimeInterval takes in seconds as a float? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. A TimeInterval is always in seconds. You can see here that the timestamp in the cache is millis. |
||
} | ||
|
||
func saveCachedData(_ storedItems: StoredItems, cacheKey: String, lastUpdated: Date, etag: String?) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,18 +79,21 @@ class FlagSynchronizer: LDFlagSynchronizing, EventHandler { | |
private var isOnlineQueue = DispatchQueue(label: "com.launchdarkly.FlagSynchronizer.isOnlineQueue") | ||
let pollingInterval: TimeInterval | ||
let useReport: Bool | ||
private var lastCachedRequestedTime: Date? | ||
|
||
private var syncQueue = DispatchQueue(label: Constants.queueName, qos: .utility) | ||
private var eventSourceStarted: Date? | ||
|
||
init(streamingMode: LDStreamingMode, | ||
pollingInterval: TimeInterval, | ||
useReport: Bool, | ||
lastUpdated: Date?, | ||
service: DarklyServiceProvider, | ||
onSyncComplete: FlagSyncCompleteClosure?) { | ||
self.streamingMode = streamingMode | ||
self.pollingInterval = pollingInterval | ||
self.useReport = useReport | ||
self.lastCachedRequestedTime = lastUpdated | ||
self.service = service | ||
self.onSyncComplete = onSyncComplete | ||
os_log("%s streamingMode: %s pollingInterval: %s useReport: %s", log: service.config.logger, type: .debug, typeName(and: #function), String(describing: streamingMode), String(describing: pollingInterval), useReport.description) | ||
|
@@ -151,9 +154,17 @@ class FlagSynchronizer: LDFlagSynchronizing, EventHandler { | |
return | ||
} | ||
|
||
// We should fire right away, unless we know how fresh the cache is and can | ||
// adjust accordingly. | ||
var fireAt = Date.distantPast | ||
if let lastTime = self.lastCachedRequestedTime { | ||
fireAt = lastTime.addingTimeInterval(pollingInterval) | ||
// If we do consider the cached values already fresh enough, we should | ||
// signal completion immediately | ||
syncQueue.async { [self] in reportSyncComplete(.upToDate) } | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like it should be possible to make these two code paths converge. If the fireAt is in the past, does the LDTimer fire immediately? If so, seems like defaulting last cached requested time to epoch when it is missing should work. Perhaps I'm missing why makeFlagRequest(...) is in one path but not the other and that reason I'm missing is why the paths can't converge? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was affecting unit test execution order. I've updated the unit tests and combined these two code paths. |
||
flagRequestTimer = LDTimer(withTimeInterval: pollingInterval, fireQueue: syncQueue, fireAt: fireAt, execute: processTimer) | ||
os_log("%s", log: service.config.logger, type: .debug, typeName(and: #function)) | ||
flagRequestTimer = LDTimer(withTimeInterval: pollingInterval, fireQueue: syncQueue, execute: processTimer) | ||
makeFlagRequest(isOnline: true) | ||
} | ||
|
||
private func stopPolling() { | ||
|
@@ -184,6 +195,9 @@ class FlagSynchronizer: LDFlagSynchronizing, EventHandler { | |
os_log("%s starting", log: service.config.logger, type: .debug, typeName(and: #function)) | ||
let context = (useReport: useReport, | ||
logPrefix: typeName(and: #function)) | ||
// We blank this value here so that future `startPolling` requests do | ||
// not prematurely trigger the sync completion. | ||
self.lastCachedRequestedTime = nil | ||
service.getFeatureFlags(useReport: useReport) { [weak self] serviceResponse in | ||
if FlagSynchronizer.shouldRetryFlagRequest(useReport: context.useReport, statusCode: (serviceResponse.urlResponse as? HTTPURLResponse)?.statusCode) { | ||
if let myself = self { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation. The other method also is missing documentation around lastUpdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods are internal and so won't be reflected in the public documentation. I have still added some additional documentation.