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

[SR-9077] NSKeyValueObservingCustomization is broken with multiple threads #3606

Closed
lilyball mannequin opened this issue Oct 25, 2018 · 11 comments
Closed

[SR-9077] NSKeyValueObservingCustomization is broken with multiple threads #3606

lilyball mannequin opened this issue Oct 25, 2018 · 11 comments
Assignees

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Oct 25, 2018

Previous ID SR-9077
Radar rdar://problem/45567020
Original Reporter @lilyball
Type Bug
Status Resolved
Resolution Done
Environment

Latest Swift master (14d4235b571d7a4e2ae51854f0d665f96959d182)

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee @lilyball
Priority Medium

md5: 2b32c8ca7953815296bfa7e8eac77b80

relates to:

  • SR-9081 NSKeyValueObservingCustomization fundamentally broken, should be removed

Issue Description:

NSKeyValueObservingCustomization relies on a global table to map String key paths back into AnyKeyPath values. However, as the string keypath does not include the root type, this means that observing properties with the same name on separate objects will overwrite each other in the global table. In a single-threaded scenario this is acceptable as the NSKeyValueObservingCustomization methods are invoked synchronously when the observation is created, and the global table is populated immediately prior to creating the observation. However, in a multithreaded scenario, the global keypath table could be overwritten with a different keypath prior to invoking the NSKeyValueObservingCustomization method.

Assuming we can rely on the fact that these methods will be invoked synchronously during the observation and never at any other time, we could fix this by using a per-thread table instead, which we populate prior to the observation and then clear afterwards.

@belkadan
Copy link

cc @millenomi

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

I've got a proof-of-concept patch done already, but I haven't attempt to test it yet. I'm not sure how to even try and write tests for race conditions involving code that is executed once per process.

Also, while writing this, I determined that even the per-thread approach isn't a complete solution. If the implementation of automaticallyNotifiesObservers(for:) itself creates any observations (or NSSortDescriptor or NSExpression using KeyPath) then this can overwrite the per-thread value before keyPathsAffectingValue(for:) is invoked. This is of course a pretty esoteric edge case, but I don't see any way to solve it.

That said, I think the whole protocol is fundamentally flawed and should be removed. I will be filing a separate ticket about that.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

I filed the issue SR-9081 about how we should just remove the protocol entirely.

@millenomi
Copy link
Contributor

@swift-ci create

@millenomi
Copy link
Contributor

API can't be removed; we have to fix this.

@millenomi
Copy link
Contributor

It looks to me like the solution here is either a per-thread table; or just add type information to the global table (e.g.: )

String(describing: type(of: keyPath).rootType)

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

Type information doesn't work in the face of subclasses. See SR-9081 for more details. The protocol is fundamentally broken.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

I've submitted PR #20103 for this issue.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

This PR implements the per-thread table approach. It occurs to me just now that the above esoteric edge case could be solved by having the stubs that invoke the NSKeyValueObservingCustomization methods reinstall the key path after invoking the method. I'll probably update the PR to do that.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

Upon reflection, reinstalling the key path won’t work if a subclass (or the class itself) overrides the Obj-C method and uses key paths there. This will overwrite the key path table before our stub is even invoked.

I’ve also just realized that the protocol itself is broken in the case where someone uses the Obj-C observation method because we won’t have an opportunity to populate the key path table to begin with.

@millenomi
Copy link
Contributor

Merged: swiftlang/swift@7c51419

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from swiftlang/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants