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

Potential atomic property/init deadlock fix #285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ERobsham
Copy link
Contributor

@ERobsham ERobsham commented Mar 8, 2024

Initial attempt at resolving issue #284 (Rare atomic property during initialization deadlock)

@davidchisnall
Copy link
Member

I think this code looks correct, it would be nice to have a test. I’m still not quite sure how code can get to this path though.

@ERobsham
Copy link
Contributor Author

ERobsham commented Mar 9, 2024

That is a tough one. I'm not really sure how to force this behavior, as its been very intermittent in our systems. There are definitely a couple of exacerbating factors I know of:

  • initializing objects with a lot of atomic properties, then calling an instance methods on those atomic properties (or any access I guess, just needs to trigger the objc_getProperty() call that starts this chain of events).
  • running on aarch64 (seems like the pointer hashes are much more likely to collide).

The process that is deadlocking for us has the 'main application' class with a ton of atomic properties, and most of those classes are also host to a bunch of atomic properties, in some cases this continues a number of levels deep...

As far as I can tell the only way to hit this is more or less a numbers game. You need to have an instance of an object where it has an atomic property, and both theAtomicProp and theAtomicProp.class->isa produce the same hash when run through the lock_for_pointer() hashing -- And the class for the atomic property needs to currently have an uninstalled dtable when you try to 'get' the property for the first time.

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

Successfully merging this pull request may close these issues.

2 participants