-
Notifications
You must be signed in to change notification settings - Fork 30
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
Changes from all 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 |
---|---|---|
|
@@ -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
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. Don't we need synchronization to update binders dictionary? 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. 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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
} | ||
} | ||
|
||
|
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.
This is redundant with the separate CustomLogger file. I see you changed it to private. Is it required?
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.
No, I just wanted to make sure it wasn't a problem if it was private.
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.
Then let's keep it internal and remove this extra copy (or the other one).