Kronos
Kronos copied to clipboard
MobileNativeFoundation
Add thread safety to Clock
This PR appends thread safety to Clock stableTime property, using single unfair lock.
Solves https://github.com/MobileNativeFoundation/Kronos/issues/85.
Can you add some tests here that demonstrate (how) this works? CI doesn't really work here but as long as they pass locally that's probably alright for now.
We don't need thread safety at Lyft, so we can't really support it in the sense that if this breaks we'd be able to fix it, but if there's some reasonable evidence this works as expected I don't really see a reason why we couldn't add it.
As an aside, have you looked into using a more modern Swift concurrency approach to this? Using async/await, making Clock an actor, annotating it with @MainActor, etc.?
@sberrevoets
Can you add some tests here that demonstrate (how) this works? CI doesn't really work here but as long as they pass locally that's probably alright for now.
We don't need thread safety at Lyft, so we can't really support it in the sense that if this breaks we'd be able to fix it, but if there's some reasonable evidence this works as expected I don't really see a reason why we couldn't add it.
I am not sure if it is possible to reliably test thread safety in unit tests (see this and this). Running the code in the application and checking the thread sanitizer is probably the best we can do.
Calling the sync on the main thread and accessing annotatedNow on the other thread will quickly reproduce the issue.
As an aside, have you looked into using a more modern Swift concurrency approach to this? Using async/await, making Clock an actor, annotating it with
@MainActor, etc.?
That should be relatively easy but I didn't want to break the existing API.
@sberrevoets Hey, thanks for the approval and sorry for the slight delay in responding. Fix has been in production for over a week now and we haven't noticed any worrying symptoms.
In any case, the linter failed on the code I haven't touched. How would you advise to proceed? Can this be merged regardless?
I just did but they'll likely fail because there is a problem with the tests and how they run on GitHub in general