And then I use Logger.custom.log() all around from different isolation contexts.
It is important to note that Logger.custom is set only once.
I understand why Swift 6 complains about this pattern as not safe, but my question is, is this a data race in practice? Is it ok to use nonisolated(unsafe) in this case?
Thread sanitizer doesn't complain about this, but I understand that there might be implicit synchronization operations that make this code pattern safe at the time being.
If this is genuine data race on a high level, would adding OSMemoryBarrier or atomic_thread_fence after setting Logger.custom resolve it?
It seems to me that if you attempt to read Logger.custom (in the Logger.custom.log() call) while another thread is calling Logger.custom = FirebaseCustomLogger(), it'd be a data race, yes.
If I had no other way around this problem, I'd create a concurrent queue to access the value (to be able to read it from multiple threads at once), and dispatch the write operation using dispatchQueue.async(flags: .barrier) { ... }.
But honestly, if possible I'd try to find an alternative pattern. You don't really have any compiler guarantees that Logger.custom.log() will use FirebaseCustomLogger instead of EmptyLogger, which could lead to unexpectedly missing logs and is IMHO more likely to result in a bug in practice than the data race.
Perhaps a solution would be to do make Logger.custom a let:
public extension Logger {
static let custom = FirebaseCustomLogger()
}
And make any additional setup of the logger in the applicationDidFinishLanching (assuming you can't do it earlier):
I am just wondering if this is possible in practice as didFinishLaunchingWithOptions is entry point to the app and no other code can execute before it finishes?
Yeah, this makes sense, but I wanted to avoid this if I can.
I think this is the main thing I am after. I am wondering if and how can this situation happen?
The problem here is that this is in another module (Core) that doesn't have access to FirebaseCustomLogger.
But this is still running on a single (main) thread I believe?
Which means that even if some piece of code uses Logger.custom before didFinishLaunchingWithOptions, there is no data race there - they will only get a default empty logger?
There are a few things that can run before didFinishLaunchingWithOptions. For example, Objective-C's load method for NSObject classes, UIApplicationDelegate's willFinishLaunchingWithOptions...
Even an innocent code addition before Logger.custom = FirebaseCustomLogger() in didFinishLaunchingWithOptions could end up calling the logger before it was properly set.
If you're confident that this won't happen in the future, adding nonisolated(unsafe) may be fine.
Code that runs before didFinishLaunchingWithOptions can trigger work that is executed in a different thread. For example, something as innocent as this:
func willFinishLaunchingWithOptions() {
Task {
// ...
Logger.custom.log("...") // <-- Which logger is this?
}
}
Written in the willFinishLaunchingWithOptions, would race against the Logger.custom = ... in didFinishLaunchingWithOptions.
You may not have any code running before didFinishLaunchingWithOptions now, but maybe you will in the future. This is very project dependent, so you'd know best.
Hmm. Dependency issues are tricky and very project dependent too. One idea that comes to mind is creating some basic object (for example BaseLogger) somewhere accessible by both Core and FirebaseCustomLogger, with an API that allows configuring it (BaseLogger) from the outside to reroute logs to Firebase. Something like this:
func applicationDidFinishLanching() {
Logger.custom.setupFirebaseRelay { log in
Analytics.logEvent(log)
}
}
With BaseLogger handling the logic of relaying the messages received by Logger.custom.log() to Firebase once that becomes available. Again, this is highly dependent on the kind of project you're writing, but I've noticed many projects end up adding multiple services to the logger instead of just one (for example: logging both to console and Firebase, or Firebase and Amplitude...). So you may find the need to create a similar logger object that is not Firebase-specific in the future.
But if all this sounds overkill to you, maybe a simple lock will do:
public extension Logger {
private static let _custom = OSAllocatedUnfairLock<any CustomLogger>(
initialState: EmptyCustomLogger()
)
static var custom: CustomLogger {
get {
_custom.withLock { logger in
return logger
}
}
set {
_custom.withLock { logger in
logger = newValue
}
}
}
}
This prevents Logger.custom from being read from multiple threads at once, but it's a really fast operation so I don't think this could conceivably have any performance implications (unless you're calling Logger.custom from multiple threads at once thousands of times per second, which sounds unlikely for a logger!).
A plain ol' Mutex will almost certainly be much faster than this, fwiw. The pattern you describe is widespread, but I've rarely, if ever, seen it used in a situation where it was actually an improvement.
This case is almost one of those cases, except that for reasons, even DispatchQueue.sync has to call Block_copy, which will take several orders of magnitude longer than the work being done.
You can verify this in a little test program that just calls .sync {} in a loop by setting a breakpoint on _Block_copy.
I’ve the years I’ve learnt that it’s better not to ‘bend’ the rules as far as compiler correctness and concurrency are concerned. Eventually something will change and you’ll encounter a very weird problem.
Given how fast Mutex is, I would just put this value behind a Mutex. Or, if that’s too new, an OSAllocatedUnfairLock, as shown by Andropov’s post. It’s simple, fast, and future proof.