Add thread safety to Clock

[sewerynplazuk]

This PR appends thread safety to Clock stableTime property, using single unfair lock.

Solves #85.

[acecilia]

@keith @sberrevoets Any chance to get feedback on this? 🙏

[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.

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.?

[sewerynplazuk]

@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]

I was going to suggest changing to a modern API anyway, but realized it probably doesn't matter since a lot of the callsites use GCD so that wouldn't really solve the problem here. It's probably worth adding that in the meantime, but no need to hold up this PR for that.

I assume you've used this in prod for a while? No problems that came up?

[sewerynplazuk]

@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?

[sewerynplazuk]

@sberrevoets Could you please approve the PR checks?

[sberrevoets]

I just did but they'll likely fail because there is a problem with the tests and how they run on GitHub in general