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.

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: some crashes have come up because of logger injection. the reason seems to be the the binder is being updated every inject #387

Merged
merged 4 commits into from
Feb 23, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
12 changes: 12 additions & 0 deletions DemoSwiftApp/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,18 @@
import UIKit
import Optimizely

private final class CustomLogger: OPTLogger {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant with the separate CustomLogger file. I see you changed it to private. Is it required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just wanted to make sure it wasn't a problem if it was private.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's keep it internal and remove this extra copy (or the other one).

public static var logLevel: OptimizelyLogLevel = .info

required init() {
}

public func log(level: OptimizelyLogLevel, message: String) {
if level.rawValue <= CustomLogger.logLevel.rawValue {
print("🐱 - [\(level.name)] Kitty - \(message)")
}
}
}
@UIApplicationMain
class AppDelegate: UIResponder, UIApplicationDelegate {
let logLevel = OptimizelyLogLevel.debug
Expand Down
2 changes: 1 addition & 1 deletion DemoSwiftApp/Customization/CustomLogger.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import Foundation
import Optimizely

class CustomLogger: OPTLogger {
private final class CustomLogger: OPTLogger {
public static var logLevel: OptimizelyLogLevel = .info

required init() {
Expand Down
6 changes: 3 additions & 3 deletions Podfile.lock
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
PODS:
- OCMock (3.7.1)
- SwiftLint (0.40.3)
- SwiftLint (0.42.0)

DEPENDENCIES:
- OCMock (= 3.7.1)
Expand All @@ -13,8 +13,8 @@ SPEC REPOS:

SPEC CHECKSUMS:
OCMock: 75fbeaa46a9b11f8c182bbb1d1f7e9a35ccc9955
SwiftLint: dfd554ff0dff17288ee574814ccdd5cea85d76f7
SwiftLint: 4fa9579c63416865179bc416f0a92d55f009600d

PODFILE CHECKSUM: 0195918f4c7b47425ae284153b642617e2ef6781

COCOAPODS: 1.9.3
COCOAPODS: 1.9.2
12 changes: 6 additions & 6 deletions Sources/Extensions/OptimizelyClient+Extension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,24 @@ extension OptimizelyClient {
notificationCenter: OPTNotificationCenter) {
// bind it as a non-singleton. so, we will create an instance anytime injected.
// we don't associate the logger with a sdkKey at this time because not all components are sdkKey specific.
let binder: Binder = Binder<OPTLogger>(service: OPTLogger.self).to(factory: type(of: logger).init)
var binder: Binder = Binder<OPTLogger>(service: OPTLogger.self, factory: type(of: logger).init)

//Register my logger service.
HandlerRegistryService.shared.registerBinding(binder: binder)

// this is bound a reusable singleton. so, if we re-initalize, we will keep this.
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTNotificationCenter>(service: OPTNotificationCenter.self).singetlon().reInitializeStrategy(strategy: .reUse).using(instance: notificationCenter).sdkKey(key: sdkKey))

HandlerRegistryService.shared.registerBinding(binder: Binder<OPTNotificationCenter>(sdkKey: sdkKey, service: OPTNotificationCenter.self, strategy: .reUse, isSingleton: true, inst: notificationCenter))
// the decision service is also a singleton that will reCreate on re-initalize
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTDecisionService>(service: OPTDecisionService.self).singetlon().using(instance: decisionService).reInitializeStrategy(strategy: .reUse).sdkKey(key: sdkKey))
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTDecisionService>(sdkKey: sdkKey, service: OPTDecisionService.self, strategy: .reUse, isSingleton: true, inst: decisionService))

// An event dispatcher. We use a singleton and use the same Event dispatcher for all
// projects. If you change the event dispatcher, you can potentially lose data if you
// don't use the same backingstore.
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTEventDispatcher>(service: OPTEventDispatcher.self).singetlon().reInitializeStrategy(strategy: .reUse).using(instance: eventDispatcher).sdkKey(key: sdkKey))
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTEventDispatcher>(sdkKey: sdkKey, service: OPTEventDispatcher.self, strategy: .reUse, isSingleton: true, inst: eventDispatcher))

// This is a singleton and might be a good candidate for reuse. The handler supports mulitple
// sdk keys without having to be created for every key.
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTDatafileHandler>(service: OPTDatafileHandler.self).singetlon().reInitializeStrategy(strategy: .reUse).to(factory: type(of: datafileHandler).init).using(instance: datafileHandler).sdkKey(key: sdkKey))
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTDatafileHandler>(sdkKey: sdkKey, service: OPTDatafileHandler.self, strategy: .reUse, isSingleton: true, inst: datafileHandler))
}

/// OptimizelyClient init
Expand Down
44 changes: 18 additions & 26 deletions Sources/Utils/HandlerRegistryService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,29 @@ class HandlerRegistryService {

let binderToUse = binders.property?[skLocal] ?? binders.property?[skGlobal]

func updateBinder(b : BinderProtocol) {
if binders.property?[skLocal] != nil {
binders.property?[skLocal] = b
} else {
binders.property?[skGlobal] = b
}
Comment on lines +56 to +60
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need synchronization to update binders dictionary?
Let's fix this unsafe dictionaries/arrays in other places as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the binder set is an atomic property. the binder is a copy and so this is an atomic operation. the get and set are separate. but, the again, since this is only an issue with a singleton that usually provides a instance with creation. it should not be an issue. i don't want to slow everything down by constantly wrapping everything in a dispatch.

}

if var binder = binderToUse {
if isReintialize && binder.strategy == .reCreate {
binder.instance = binder.factory()
result = binder.instance
updateBinder(b: binder)
} else if let inst = binder.instance, binder.isSingleton {
result = inst
} else {
if !binder.isSingleton {
return binder.factory()
}
let inst = binder.factory()
binder.instance = inst
result = inst
updateBinder(b: binder)
}
}
return result
Expand Down Expand Up @@ -97,7 +110,7 @@ protocol BinderProtocol {
var instance: Any? { get set }

}
class Binder<T>: BinderProtocol {
struct Binder<T>: BinderProtocol {
var sdkKey: String?
var service: Any
var strategy: ReInitializeStrategy = .reCreate
Expand All @@ -117,34 +130,13 @@ class Binder<T>: BinderProtocol {
}
}

init(service: Any) {
init(sdkKey: String? = nil, service: Any, strategy: ReInitializeStrategy = .reCreate, factory: @escaping (() -> Any?) = { ()->Any? in { return nil as Any? }}, isSingleton: Bool = false, inst: T? = nil) {
self.sdkKey = sdkKey
self.service = service
}

func sdkKey(key: String) -> Binder {
self.sdkKey = key
return self
}

func singetlon() -> Binder {
isSingleton = true
return self
}

func reInitializeStrategy(strategy: ReInitializeStrategy) -> Binder {
self.strategy = strategy

return self
}

func using(instance: T) -> Binder {
self.inst = instance
return self
}

func to(factory:@escaping () -> T?) -> Binder {
self.factory = factory
return self
self.isSingleton = isSingleton
self.inst = inst
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class OptimizelyClientTests_DatafileHandler: XCTestCase {
// set the url to use as our datafile download url
handler.localFileUrl = fileUrl

HandlerRegistryService.shared.registerBinding(binder: Binder<OPTDatafileHandler>(service: OPTDatafileHandler.self).using(instance: handler).singetlon().sdkKey(key: sdkKey).reInitializeStrategy(strategy: .reUse))
HandlerRegistryService.shared.registerBinding(binder: Binder<OPTDatafileHandler>(sdkKey: sdkKey, service: OPTDatafileHandler.self, strategy: .reUse, isSingleton: true, inst: handler))

let client = OptimizelyClient(sdkKey: sdkKey)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ class OptimizelyClientTests_Init_Async: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())

HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))


let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -74,7 +76,7 @@ class OptimizelyClientTests_Init_Async: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .failure)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -95,8 +97,8 @@ class OptimizelyClientTests_Init_Async: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .failedToLoadFromCache)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

let exp = expectation(description: "x")
Expand All @@ -116,7 +118,7 @@ class OptimizelyClientTests_Init_Async: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey,
periodicDownloadInterval: 1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -85,7 +85,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithNil)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -100,7 +100,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .failure)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -115,7 +115,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -130,7 +130,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey)

Expand All @@ -147,7 +147,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey,
periodicDownloadInterval: 10)
Expand All @@ -163,7 +163,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey,
periodicDownloadInterval: 10)
Expand All @@ -185,7 +185,7 @@ class OptimizelyClientTests_Init_Sync: XCTestCase {
let testSdkKey = OTUtils.randomSdkKey // unique but consistent with registry + start

let handler = FakeDatafileHandler(mode: .successWithData)
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self).sdkKey(key: testSdkKey).using(instance: handler).to(factory: FakeDatafileHandler.init).reInitializeStrategy(strategy: .reUse).singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: testSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: testSdkKey,
periodicDownloadInterval: 10)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,7 @@ class OptimizelyClientTests_OptimizelyConfig: XCTestCase {

let badUniqueSdkKey = "badUniqueSdkKey"

HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self)
.sdkKey(key: badUniqueSdkKey)
.using(instance: FakeDatafileHandler())
.to(factory: FakeDatafileHandler.init)
.reInitializeStrategy(strategy: .reUse)
.singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: badUniqueSdkKey, service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: FakeDatafileHandler()))

var optimizelyConfig: OptimizelyConfig?
let optimizely = OptimizelyClient(sdkKey: badUniqueSdkKey, periodicDownloadInterval: 1)
Expand Down
15 changes: 2 additions & 13 deletions Tests/OptimizelyTests-APIs/OptimizelyClientTests_Others.swift
Original file line number Diff line number Diff line change
Expand Up @@ -342,12 +342,7 @@ class OptimizelyClientTests_Others: XCTestCase {
}

let handler = FakeDatafileHandler()
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self)
.sdkKey(key: "testFetchedDatafileInvalid")
.using(instance: handler)
.to(factory: FakeDatafileHandler.init)
.reInitializeStrategy(strategy: .reUse)
.singetlon())
HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: "testFetchedDatafileInvalid", service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: "testFetchedDatafileInvalid")

Expand Down Expand Up @@ -378,13 +373,7 @@ class OptimizelyClientTests_Others: XCTestCase {
}

let handler = FakeDatafileHandler()
HandlerRegistryService.shared.registerBinding(binder: Binder(service: OPTDatafileHandler.self)
.sdkKey(key: "testHandlerReinitializeOnBackgroundDatafileUpdate")
.using(instance: handler)
.to(factory: FakeDatafileHandler.init)
.reInitializeStrategy(strategy: .reUse)
.singetlon())

HandlerRegistryService.shared.registerBinding(binder: Binder(sdkKey: "testHandlerReinitializeOnBackgroundDatafileUpdate", service: OPTDatafileHandler.self, strategy: .reUse, factory: FakeDatafileHandler.init, isSingleton: true, inst: handler))

let optimizely = OptimizelyClient(sdkKey: "testHandlerReinitializeOnBackgroundDatafileUpdate")

Expand Down
4 changes: 2 additions & 2 deletions Tests/OptimizelyTests-Common/DataStoreTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,9 @@ class DataStoreTests: XCTestCase {
}
let logger = Logger()

let binder: Binder = Binder<OPTLogger>(service: OPTLogger.self).to { () -> OPTLogger? in
let binder: Binder = Binder<OPTLogger>(service: OPTLogger.self, factory: { () -> OPTLogger? in
return logger
}
})

HandlerRegistryService.shared.registerBinding(binder: binder)

Expand Down
Loading