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

fix: Make Confidence.cache thread-safe #167

Merged
merged 3 commits into from
Nov 5, 2024
Merged

fix: Make Confidence.cache thread-safe #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c32f69c
fix: Make Confidence.cache thread-safe
fabriziodemaria Nov 5, 2024
06140a3
fix: different queues for different accesses
fabriziodemaria Nov 5, 2024
454a696
fix: Confidence.cache thread-safe reads
fabriziodemaria Nov 5, 2024
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
40 changes: 29 additions & 11 deletions Sources/Confidence/Confidence.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ public class Confidence: ConfidenceEventSender {
private let eventSenderEngine: EventSenderEngine
private let contextSubject = CurrentValueSubject<ConfidenceStruct, Never>([:])
private var removedContextKeys: Set<String> = Set()
private let confidenceQueue = DispatchQueue(label: "com.confidence.queue")
private let contextSubjectQueue = DispatchQueue(label: "com.confidence.queue.contextsubject")
private let cacheQueue = DispatchQueue(label: "com.confidence.queue.cache")
private let flagApplier: FlagApplier
private var cache = FlagResolution.EMPTY
private var storage: Storage
Expand Down Expand Up @@ -70,14 +71,20 @@ public class Confidence: ConfidenceEventSender {
}
.store(in: &cancellables)
}

/**
Activating the cache means that the flag data on disk is loaded into memory, so consumers can access flag values.
Errors can be thrown if something goes wrong access data on disk.
*/
public func activate() throws {
let savedFlags = try storage.load(defaultValue: FlagResolution.EMPTY)
self.cache = savedFlags
debugLogger?.logFlags(action: "Activate", flag: "")
try cacheQueue.sync { [weak self] in
guard let self = self else {
return
}
let savedFlags = try storage.load(defaultValue: FlagResolution.EMPTY)
cache = savedFlags
debugLogger?.logFlags(action: "Activate", flag: "")
}
}

/**
Expand Down Expand Up @@ -133,12 +140,23 @@ public class Confidence: ConfidenceEventSender {
- Parameter defaultValue: returned in case of errors or in case of the variant's rule indicating to use the default value.
*/
public func getEvaluation<T>(key: String, defaultValue: T) -> Evaluation<T> {
self.cache.evaluate(
flagName: key,
defaultValue: defaultValue,
context: getContext(),
flagApplier: flagApplier
)
cacheQueue.sync { [weak self] in
guard let self = self else {
return Evaluation(
nicklasl marked this conversation as resolved.
Show resolved Hide resolved
value: defaultValue,
variant: nil,
reason: .error,
errorCode: .providerNotReady,
errorMessage: "Confidence instance deallocated before end of evaluation"
)
}
return self.cache.evaluate(
flagName: key,
defaultValue: defaultValue,
context: getContext(),
flagApplier: flagApplier
)
}
}

/**
Expand Down Expand Up @@ -278,7 +296,7 @@ public class Confidence: ConfidenceEventSender {
}

private func withLock(callback: @escaping (Confidence) -> Void) {
confidenceQueue.sync { [weak self] in
contextSubjectQueue.sync { [weak self] in
guard let self = self else {
return
}
Expand Down
23 changes: 23 additions & 0 deletions Tests/ConfidenceTests/ConfidenceTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,29 @@ class ConfidenceTest: XCTestCase {
XCTAssertEqual(error as? ConfidenceError, ConfidenceError.invalidContextInMessage)
}
}

func testConcurrentActivate() async {
for _ in 1...100 {
Task {
await concurrentActivate()
}
}
}

private func concurrentActivate() async {
let confidence = Confidence.Builder(clientSecret: "test")
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to create a single instance and pass that to this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

concurrentActivate is meant to be the test we want to perform (it already has 10000 concurrent calls on the same instance). The wrapping testConcurrentActivate is just to be able to run the same test 100 times, since I noticed the exception is not always happening

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... and to be picky: running it more times is no guarantee for it to happen.
Of course it would be amazing to come up with a guaranteed way to make it crash but if that's not possible I would be fine to merge this without the added test. Up to you 👍

Copy link
Member Author

@fabriziodemaria fabriziodemaria Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree: even with multiple runs, sometimes the test suite succeeds. Although it's worth mentioning that in the majority of cases the repeated test does fail, at least on my machine.

Despite the imperfect testing, I think we are confident in the fix and we should get it out asap. I am going to merge and we improve the tests later

.build()

await withTaskGroup(of: Void.self) { group in
for _ in 0..<10000 {
group.addTask {
// no need to handle errors
// race condition crashes will surface regardless
try? confidence.activate()
}
}
}
}
}

final class DispatchQueueFake: DispatchQueueType {
Expand Down
Loading