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

Update handling of os_unfair_lock to manage the buffer. #2836

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@jshier
Copy link
Contributor

commented Jun 9, 2019

Goals ⚽️

In talking to Apple engineers at WWDC, I was told that passing a single lock instance by reference may lead to a crash rate of up to 1%, due to the compiler managing the lifetime of the instance in unexpected ways. This PR switches to keeping the memory buffer directly. We'll need to do the same thing if we add support for pthread_mutex back as well.

Implementation Details 🚧

Rather than passing a single os_unfair_lock by reference to lock and unlock, manage the os_unfair_lock_t instance directly by manually allocating and initializing the memory, as well as cleaning up afterward.

Testing Details 🔍

No tests were updated. I haven't seen this, so this is a preventative fix.

Update handling of os_unfair_lock to manage the buffer.
As recommended by Apple at WWDC, using os_unfair_lock by reference will lead to a crash rate of up to 1%.

@jshier jshier added the bug label Jun 9, 2019

@jshier jshier added this to the 5.0.0-beta.7 milestone Jun 9, 2019

@jshier jshier requested a review from cnoon Jun 9, 2019

@jshier jshier added this to Idea in Alamofire 5 via automation Jun 9, 2019

@cnoon

cnoon approved these changes Jun 10, 2019

Copy link
Member

left a comment

Looks good @jshier. Very non-obvious that this is something that a client would need to be aware of. I'm curious how you even got to the bottom of this with the WWDC engineers or knew to look for it.

@jshier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@cnoon In the midst of building a Combine operator for me (which is now on the initial-combine-support branch), he needed a locking mechanism. Once I told him about our os_unfair_lock wrapper, he immediately wanted to check it for this bug. Given that he's also the Operation maintainer, locking is kind of his thing. So it was a fortuitous but unrelated discovery.

@cnoon

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Wow, that's awesome! Super glad you happened to be chatting with that particular person! That would have been horrible for us to have to try and track down.

@jshier jshier merged commit 1b89a57 into master Jun 14, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

Alamofire 5 automation moved this from Idea to Done Jun 14, 2019

@jshier jshier deleted the bugfix/lock-usage branch Jun 14, 2019

toomasr added a commit to toomasr/Alamofire that referenced this pull request Aug 19, 2019

Update handling of os_unfair_lock to manage the buffer. (Alamofire#2836)
As recommended by Apple at WWDC, using os_unfair_lock by reference will lead to a crash rate of up to 1%.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.