From e2c2f37e6052ff4e5d976009d481faff12ae08f8 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 11 Aug 2020 17:25:47 -0300 Subject: [PATCH] Respond to CR. --- .../src/Contacts/SignalServiceAddress.swift | 166 +++++++++--------- .../Messages/SignalServiceAddressTest.swift | 61 ++++--- 2 files changed, 122 insertions(+), 105 deletions(-) diff --git a/SignalServiceKit/src/Contacts/SignalServiceAddress.swift b/SignalServiceKit/src/Contacts/SignalServiceAddress.swift index 64268f5ecf..7fa0b01079 100644 --- a/SignalServiceKit/src/Contacts/SignalServiceAddress.swift +++ b/SignalServiceKit/src/Contacts/SignalServiceAddress.swift @@ -129,80 +129,6 @@ public class SignalServiceAddress: NSObject, NSCopying, NSSecureCoding, Codable observeMappingChanges() } - @objc - private class AddressMappingObserver: NSObject { - private let uuid: UUID - private let unfairLock = UnfairLock() - - // There might be multiple instances in this aray that represent the same address. - // - // weakAddresses should only be accessed within unfairLock. - private var weakAddresses = NSHashTable(options: [ - .weakMemory, - .objectPointerPersonality - ]) - - init(uuid: UUID) { - self.uuid = uuid - super.init() - registerForMappingChangeNotification() - } - - func add(address: SignalServiceAddress) { - unfairLock.withLock { - weakAddresses.add(address) - } - } - - private func registerForMappingChangeNotification() { - NotificationCenter.default.addObserver( - self, selector: - #selector(mappingDidChange), - name: SignalServiceAddressCache.mappingDidChangeNotification, - object: nil - ) - } - - @objc - func mappingDidChange(notification: Notification) { - guard let updatedUuid = notification.userInfo?[SignalServiceAddressCache.mappingDidChangeNotificationUUIDKey] as? UUID, - uuid == updatedUuid - else { return } - - let addresses = unfairLock.withLock { - weakAddresses.allObjects - } - for address in addresses { - address.backingPhoneNumber.set(SignalServiceAddress.cache.phoneNumber(forUuid: uuid)) - } - } - } - - private var mappingObserver = AtomicOptional(nil) - private static let mappingObserverCache = NSCache() - - private func observeMappingChanges() { - guard let uuid = backingUuid.get() else { - return - } - guard mappingObserver.get() == nil else { - owsFailDebug("There's shouldn't be an existing observer.") - return - } - let observer: AddressMappingObserver - if let cachedObserver = Self.mappingObserverCache.object(forKey: uuid as NSUUID) { - observer = cachedObserver - } else { - observer = AddressMappingObserver(uuid: uuid) - Self.mappingObserverCache.setObject(observer, forKey: uuid as NSUUID) - } - observer.add(address: self) - // We could race in this method, but in practice it should never happen. - // If it did, it wouldn't have any adverse side effects. - owsAssertDebug(mappingObserver.get() == nil) - mappingObserver.set(observer) - } - // MARK: - Codable private enum CodingKeys: String, CodingKey { @@ -371,8 +297,91 @@ public class SignalServiceAddress: NSObject, NSCopying, NSSecureCoding, Codable override public var description: String { return "" } + + // MARK: - Mapping Changes + + // The "observer" is a weak array of addresses with the + // same UUID observing changes to that UUID. + private typealias AddressMappingObserver = NSHashTable + + // Every address retains a strong reference to its observer. + private var mappingObserver = AtomicOptional(nil) + + private static let unfairLock = UnfairLock() + private static let mappingObserverCache = NSMapTable(keyOptions: .strongMemory, + valueOptions: .weakMemory) + + private func observeMappingChanges() { + guard let uuid = backingUuid.get() else { + return + } + guard mappingObserver.get() == nil else { + owsFailDebug("There's shouldn't be an existing observer.") + return + } + let observer = Self.unfairLock.withLock { () -> AddressMappingObserver in + let observer = { () -> AddressMappingObserver in + if let observer = Self.mappingObserverCache.object(forKey: uuid as NSUUID) { + return observer + } else { + // * Use weak references to addresses. + // * Use .objectPointerPersonality; this NSHashTable will contain + // a list of addresses that are all "equal". + let observer = NSHashTable(options: [ + .weakMemory, + .objectPointerPersonality + ]) + Self.mappingObserverCache.setObject(observer, forKey: uuid as NSUUID) + return observer + } + }() + observer.add(self) + return observer + } + // We could race in this method, but in practice it should never happen. + // If it did, it wouldn't have any adverse side effects. + owsAssertDebug(mappingObserver.get() == nil) + mappingObserver.set(observer) + } + + fileprivate static func notifyMappingDidChange(forUuid uuid: UUID) { + guard let addresses = (Self.unfairLock.withLock { () -> [SignalServiceAddress]? in + guard let observer = Self.mappingObserverCache.object(forKey: uuid as NSUUID) else { + return nil + } + return observer.allObjects + }) else { + return + } + for address in addresses { + address.mappingDidChange(uuid: uuid) + } + } + + fileprivate func mappingDidChange(uuid: UUID) { + owsAssertDebug(uuid == self.uuid) + backingPhoneNumber.set(SignalServiceAddress.cache.phoneNumber(forUuid: uuid)) + } } +// MARK: - + +#if TESTABLE_BUILD + +extension SignalServiceAddress { + var unresolvedUuid: UUID? { + backingUuid.get() + } + + var unresolvedPhoneNumber: String? { + backingPhoneNumber.get() + } +} + +#endif + +// MARK: - + @objc public class SignalServiceAddressCache: NSObject { private let serialQueue = DispatchQueue(label: "SignalServiceAddressCache") @@ -462,9 +471,6 @@ public class SignalServiceAddressCache: NSObject { return serialQueue.sync { uuidToPhoneNumberCache[uuid] } } - static let mappingDidChangeNotification = Notification.Name("SignalServiceAddressCacheMappingDidChange") - static let mappingDidChangeNotificationUUIDKey = "mappingDidChangeNotificationUUIDKey" - @objc func updateMapping(uuid: UUID, phoneNumber: String?) { serialQueue.sync { @@ -505,10 +511,6 @@ public class SignalServiceAddressCache: NSObject { } // Notify any existing address objects to update their backing phone number - NotificationCenter.default.post( - name: SignalServiceAddressCache.mappingDidChangeNotification, - object: nil, - userInfo: [SignalServiceAddressCache.mappingDidChangeNotificationUUIDKey: uuid] - ) + SignalServiceAddress.notifyMappingDidChange(forUuid: uuid) } } diff --git a/SignalServiceKit/tests/Messages/SignalServiceAddressTest.swift b/SignalServiceKit/tests/Messages/SignalServiceAddressTest.swift index c8032ef9e5..bb5308b46d 100644 --- a/SignalServiceKit/tests/Messages/SignalServiceAddressTest.swift +++ b/SignalServiceKit/tests/Messages/SignalServiceAddressTest.swift @@ -85,37 +85,52 @@ class SignalServiceAddressTest: SSKBaseTestSwift { autoreleasepool { let address1a = SignalServiceAddress(uuid: uuid1, phoneNumber: nil) let address1b = SignalServiceAddress(uuid: uuid1, phoneNumber: nil) - let address1c = SignalServiceAddress(uuid: uuid1, phoneNumber: nil) + let address1c = SignalServiceAddress(uuid: uuid1, phoneNumber: phoneNumber1) let address2a = SignalServiceAddress(uuid: uuid2, phoneNumber: nil) let address2b = SignalServiceAddress(uuid: uuid2, phoneNumber: phoneNumber2) - XCTAssertEqual(address1a.uuid, uuid1) - XCTAssertNil(address1a.phoneNumber) - XCTAssertEqual(address1b.uuid, uuid1) - XCTAssertNil(address1b.phoneNumber) - XCTAssertEqual(address1c.uuid, uuid1) - XCTAssertNil(address1c.phoneNumber) - XCTAssertEqual(address2a.uuid, uuid2) - XCTAssertNil(address2a.phoneNumber) - XCTAssertEqual(address2b.uuid, uuid2) - XCTAssertEqual(address2b.uuid, uuid2) + // We use the "unresolved" accessors unresolvedUuid(), unresolvedPhoneNumber() + // to avoid filling in the backing values. + + XCTAssertEqual(address1a.unresolvedUuid, uuid1) + XCTAssertNil(address1a.unresolvedPhoneNumber) + XCTAssertEqual(address1b.unresolvedUuid, uuid1) + XCTAssertNil(address1b.unresolvedPhoneNumber) + XCTAssertEqual(address1c.unresolvedUuid, uuid1) + XCTAssertEqual(address1c.unresolvedPhoneNumber, phoneNumber1) + XCTAssertEqual(address2a.unresolvedUuid, uuid2) + XCTAssertNil(address2a.unresolvedPhoneNumber) + XCTAssertEqual(address2b.unresolvedUuid, uuid2) + XCTAssertEqual(address2b.unresolvedPhoneNumber, phoneNumber2) SSKEnvironment.shared.signalServiceAddressCache.updateMapping(uuid: uuid1, phoneNumber: phoneNumber1) - XCTAssertEqual(address1a.uuid, uuid1) - XCTAssertEqual(address1a.phoneNumber, phoneNumber1) - XCTAssertEqual(address1b.uuid, uuid1) - // Consulting address1b.phoneNumber will fill in the missing value. - XCTAssertEqual(address1b.phoneNumber, phoneNumber1) - XCTAssertEqual(address1c.uuid, uuid1) - // Do not consult address1c.phoneNumber. - XCTAssertEqual(address2a.uuid, uuid2) - XCTAssertNil(address2a.phoneNumber) - XCTAssertEqual(address2b.uuid, uuid2) - XCTAssertEqual(address2b.uuid, uuid2) + XCTAssertEqual(address1a.unresolvedUuid, uuid1) + XCTAssertEqual(address1a.unresolvedPhoneNumber, phoneNumber1) + XCTAssertEqual(address1b.unresolvedUuid, uuid1) + XCTAssertEqual(address1b.unresolvedPhoneNumber, phoneNumber1) + XCTAssertEqual(address1c.unresolvedUuid, uuid1) + XCTAssertEqual(address1c.unresolvedPhoneNumber, phoneNumber1) + XCTAssertEqual(address2a.unresolvedUuid, uuid2) + XCTAssertNil(address2a.unresolvedPhoneNumber) + XCTAssertEqual(address2b.unresolvedUuid, uuid2) + XCTAssertEqual(address2b.unresolvedPhoneNumber, phoneNumber2) SSKEnvironment.shared.signalServiceAddressCache.updateMapping(uuid: uuid1, phoneNumber: phoneNumber3) + XCTAssertEqual(address1a.unresolvedUuid, uuid1) + XCTAssertEqual(address1a.unresolvedPhoneNumber, phoneNumber3) + XCTAssertEqual(address1b.unresolvedUuid, uuid1) + XCTAssertEqual(address1b.unresolvedPhoneNumber, phoneNumber3) + XCTAssertEqual(address1c.unresolvedUuid, uuid1) + XCTAssertEqual(address1c.unresolvedPhoneNumber, phoneNumber3) + XCTAssertEqual(address2a.unresolvedUuid, uuid2) + XCTAssertNil(address2a.unresolvedPhoneNumber) + XCTAssertEqual(address2b.unresolvedUuid, uuid2) + XCTAssertEqual(address2b.unresolvedPhoneNumber, phoneNumber2) + + // MARK: - Resolved values + XCTAssertEqual(address1a.uuid, uuid1) XCTAssertEqual(address1a.phoneNumber, phoneNumber3) XCTAssertEqual(address1b.uuid, uuid1) @@ -125,7 +140,7 @@ class SignalServiceAddressTest: SSKBaseTestSwift { XCTAssertEqual(address2a.uuid, uuid2) XCTAssertNil(address2a.phoneNumber) XCTAssertEqual(address2b.uuid, uuid2) - XCTAssertEqual(address2b.uuid, uuid2) + XCTAssertEqual(address2b.phoneNumber, phoneNumber2) } } }