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

Add thread safety to Clock #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sewerynplazuk
Copy link

@sewerynplazuk sewerynplazuk commented Feb 1, 2024

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

Solves #85.

@sewerynplazuk sewerynplazuk force-pushed the feature/add-thread-safety-to-clock branch 2 times, most recently from 2ed9bde to 256fbb6 Compare February 1, 2024 16:33
@acecilia
Copy link

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

Sources/Clock.swift Outdated Show resolved Hide resolved
Sources/Clock.swift Outdated Show resolved Hide resolved
Sources/UnfairLock.swift Outdated Show resolved Hide resolved
@sewerynplazuk sewerynplazuk force-pushed the feature/add-thread-safety-to-clock branch from 256fbb6 to 577b925 Compare May 21, 2024 09:57
@sberrevoets
Copy link
Member

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 sewerynplazuk force-pushed the feature/add-thread-safety-to-clock branch from 577b925 to 225a326 Compare May 22, 2024 14:27
@sewerynplazuk
Copy link
Author

sewerynplazuk commented May 22, 2024

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

@sewerynplazuk sewerynplazuk force-pushed the feature/add-thread-safety-to-clock branch 2 times, most recently from 22cb9c9 to 2bf08af Compare May 22, 2024 16:41
Signed-off-by: Seweryn Plażuk <[email protected]>
Copy link
Member

@sberrevoets sberrevoets left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

@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
Copy link
Author

@sberrevoets Could you please approve the PR checks?

@sberrevoets
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants