v4.0.4 - Using threadsafe dictionaries to avoid race condition accessing udpat…#101
v4.0.4 - Using threadsafe dictionaries to avoid race condition accessing udpat…#101PravinPK merged 5 commits intoadobe:dev-v4.0.4from
Conversation
…eRequestEventIdsInProgress
spoorthipujariadobe
left a comment
There was a problem hiding this comment.
LGTM. Since we're doing remove all in a bunch of places, might be easy to make a method witch accepts the dictionary and loops through the keys removing each, but it's just a NIT
| private func updateCachedPropositions(for requestedScopes: [DecisionScope]) { | ||
| // update cache with accumulated propositions | ||
| cachedPropositions.merge(propositionsInProgress) { _, new in new } | ||
| cachedPropositions.merge(propositionsInProgress.shallowCopy) { _, new in new } |
There was a problem hiding this comment.
Can you check if cachedPropositions dict is accessed from different threads? If so, can you make it thread safe?
Sources/AEPOptimize/Optimize.swift
Outdated
| event.isPersonalizationDecisionResponse, | ||
| let requestEventId = event.requestEventId, | ||
| updateRequestEventIdsInProgress.contains(where: { $0.key == requestEventId }) | ||
| updateRequestEventIdsInProgress.shallowCopy.contains(where: { $0.key == requestEventId }) |
There was a problem hiding this comment.
Nit: Instead of shallowCopy, use the ThreadSafeDictionary.first(...) method
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-v4.0.4 #101 +/- ##
=============================================
Coverage ? 97.21%
=============================================
Files ? 13
Lines ? 861
Branches ? 0
=============================================
Hits ? 837
Misses ? 24
Partials ? 0
|
| // Removing all the values from thread-safe dictionary | ||
| // TODO: Introduce removeAll method to ThreadSafeDictionary in AEPCore | ||
| for key in propositionsInProgress.keys { | ||
| _ = propositionsInProgress.removeValue(forKey: key) |
There was a problem hiding this comment.
Is there an edge case where another thread is adding another entry while we are removing the all in here?
Description
Fix for crash could occurs when updateProposition is called recursively
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: