Skip to content

Commit

Permalink
Move delegate management into shared MTRDevice super-class. (#35084)
Browse files Browse the repository at this point in the history 
* Move delegate management into shared MTRDevice super-class.

MTRDevice_XPC and MTRDevice_Concrete can then share that code.

* Address review comments, fix TAPI build.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Aug 20, 2024
1 parent b367512 commit 1866812
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 351 deletions.
130 changes: 20 additions & 110 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,43 +64,6 @@
// Disabling pending crashes
#define ENABLE_CONNECTIVITY_MONITORING 0

// Consider moving utility classes to their own file
#pragma mark - Utility Classes

// container of MTRDevice delegate weak reference, its queue, and its interested paths for attribute reports
MTR_DIRECT_MEMBERS
@interface MTRDeviceDelegateInfo : NSObject {
@private
void * _delegatePointerValue;
__weak id _delegate;
dispatch_queue_t _queue;
NSArray * _Nullable _interestedPathsForAttributes;
NSArray * _Nullable _interestedPathsForEvents;
}

// Array of interested cluster paths, attribute paths, or endpointID, for attribute report filtering.
@property (readonly, nullable) NSArray * interestedPathsForAttributes;

// Array of interested cluster paths, attribute paths, or endpointID, for event report filtering.
@property (readonly, nullable) NSArray * interestedPathsForEvents;

// Expose delegate
@property (readonly) id delegate;

// Pointer value for logging purpose only
@property (readonly) void * delegatePointerValue;

- (instancetype)initWithDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queue interestedPathsForAttributes:(NSArray * _Nullable)interestedPathsForAttributes interestedPathsForEvents:(NSArray * _Nullable)interestedPathsForEvents;

// Returns YES if delegate and queue are both non-null, and the block is scheduled to run.
- (BOOL)callDelegateWithBlock:(void (^)(id<MTRDeviceDelegate>))block;

#ifdef DEBUG
// Only used for unit test purposes - normal delegate should not expect or handle being called back synchronously.
- (BOOL)callDelegateSynchronouslyWithBlock:(void (^)(id<MTRDeviceDelegate>))block;
#endif
@end

@implementation MTRDeviceDelegateInfo
- (instancetype)initWithDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queue interestedPathsForAttributes:(NSArray * _Nullable)interestedPathsForAttributes interestedPathsForEvents:(NSArray * _Nullable)interestedPathsForEvents
{
Expand Down Expand Up @@ -371,7 +334,6 @@ - (BOOL)isEqual:(id)object
#define MTRDEVICE_SUBSCRIPTION_LATENCY_NEW_VALUE_WEIGHT (1.0 / 3.0)

@interface MTRDevice ()
@property (nonatomic, readonly) os_unfair_lock lock; // protects the caches and device state
// protects against concurrent time updates by guarding timeUpdateScheduled flag which manages time updates scheduling,
// and protects device calls to setUTCTime and setDSTOffset. This can't just be replaced with "lock", because the time
// update code calls public APIs like readAttributeWithEndpointID:.. (which attempt to take "lock") while holding
Expand Down Expand Up @@ -502,8 +464,6 @@ @implementation MTRDevice {
// System time change observer reference
id _systemTimeChangeObserverToken;

NSMutableSet<MTRDeviceDelegateInfo *> * _delegates;

// Protects mutable state used by our description getter. This is a separate lock from "lock"
// so that we don't need to worry about getting our description while holding "lock" (e.g due to
// logging self). This lock _must_ be held narrowly, with no other lock acquisitions allowed
Expand All @@ -526,10 +486,11 @@ @implementation MTRDevice {
NSDate * _Nullable _lastSubscriptionFailureTimeForDescription;
}

- (instancetype)initForSubclasses
- (instancetype)initForSubclassesWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
{
if (self = [super init]) {
// nothing, as superclass of MTRDevice is NSObject
_lock = OS_UNFAIR_LOCK_INIT;
_delegates = [NSMutableSet set];
}

return self;
Expand Down Expand Up @@ -875,6 +836,8 @@ - (BOOL)_subscriptionsAllowed
{
os_unfair_lock_assert_owner(&self->_lock);

// TODO: XPC: This function and all its callsites should go away from this class.

// We should not allow a subscription for device controllers over XPC.
return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
}
Expand Down Expand Up @@ -923,34 +886,14 @@ - (void)_addDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)que
[_delegates addObject:newDelegateInfo];
MTR_LOG("%@ added delegate info %@", self, newDelegateInfo);

__block BOOL shouldSetUpSubscription = [self _subscriptionsAllowed];

// For unit testing only. If this ever changes to not being for unit testing purposes,
// we would need to move the code outside of where we acquire the lock above.
#ifdef DEBUG
[self _callFirstDelegateSynchronouslyWithBlock:^(id testDelegate) {
if ([testDelegate respondsToSelector:@selector(unitTestShouldSetUpSubscriptionForDevice:)]) {
shouldSetUpSubscription = [testDelegate unitTestShouldSetUpSubscriptionForDevice:self];
}
}];
#endif
// Call hook to allow subclasses to act on delegate addition.
[self _delegateAdded];
}

if (shouldSetUpSubscription) {
MTR_LOG("%@ - starting subscription setup", self);
// Record the time of first addDelegate call that triggers initial subscribe, and do not reset this value on subsequent addDelegate calls
if (!_initialSubscribeStart) {
_initialSubscribeStart = [NSDate now];
}
if ([self _deviceUsesThread]) {
MTR_LOG(" => %@ - device is a thread device, scheduling in pool", self);
[self _scheduleSubscriptionPoolWork:^{
std::lock_guard lock(self->_lock);
[self _setupSubscriptionWithReason:@"delegate is set and scheduled subscription is happening"];
} inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"];
} else {
[self _setupSubscriptionWithReason:@"delegate is set and subscription is needed"];
}
}
- (void)_delegateAdded
{
// Nothing to do; this is a hook for subclasses. If that ever changes for
// some reason, subclasses need to start calling this hook on their super.
}

- (void)removeDelegate:(id<MTRDeviceDelegate>)delegate
Expand All @@ -962,7 +905,10 @@ - (void)removeDelegate:(id<MTRDeviceDelegate>)delegate
NSMutableSet<MTRDeviceDelegateInfo *> * delegatesToRemove = [NSMutableSet set];
[self _iterateDelegatesWithBlock:^(MTRDeviceDelegateInfo * delegateInfo) {
id<MTRDeviceDelegate> strongDelegate = delegateInfo.delegate;
if (strongDelegate == delegate) {
if (!strongDelegate) {
[delegatesToRemove addObject:delegateInfo];
MTR_LOG("%@ removing delegate info for nil delegate %p", self, delegateInfo.delegatePointerValue);
} else if (strongDelegate == delegate) {
[delegatesToRemove addObject:delegateInfo];
MTR_LOG("%@ removing delegate info %@ for %p", self, delegateInfo, delegate);
}
Expand All @@ -976,45 +922,9 @@ - (void)removeDelegate:(id<MTRDeviceDelegate>)delegate

- (void)invalidate
{
MTR_LOG("%@ invalidate", self);

[_asyncWorkQueue invalidate];

os_unfair_lock_lock(&self->_timeSyncLock);
_timeUpdateScheduled = NO;
os_unfair_lock_unlock(&self->_timeSyncLock);

os_unfair_lock_lock(&self->_lock);

_state = MTRDeviceStateUnknown;
std::lock_guard lock(_lock);

[_delegates removeAllObjects];

// Make sure we don't try to resubscribe if we have a pending resubscribe
// attempt, since we now have no delegate.
_reattemptingSubscription = NO;

[_deviceController asyncDispatchToMatterQueue:^{
MTR_LOG("%@ invalidate disconnecting ReadClient and SubscriptionCallback", self);

// Destroy the read client and callback (has to happen on the Matter
// queue, to avoid deleting objects that are being referenced), to
// tear down the subscription. We will get no more callbacks from
// the subscription after this point.
std::lock_guard lock(self->_lock);
self->_currentReadClient = nullptr;
if (self->_currentSubscriptionCallback) {
delete self->_currentSubscriptionCallback;
}
self->_currentSubscriptionCallback = nullptr;

[self _changeInternalState:MTRInternalDeviceStateUnsubscribed];
}
errorHandler:nil];

[self _stopConnectivityMonitoring];

os_unfair_lock_unlock(&self->_lock);
}

- (void)nodeMayBeAdvertisingOperational
Expand Down Expand Up @@ -1145,8 +1055,7 @@ - (BOOL)_delegateExists
return [self _iterateDelegatesWithBlock:nil];
}

// Returns YES if any non-null delegates were found
- (BOOL)_iterateDelegatesWithBlock:(void(NS_NOESCAPE ^)(MTRDeviceDelegateInfo * delegateInfo)_Nullable)block
- (BOOL)_iterateDelegatesWithBlock:(void(NS_NOESCAPE ^ _Nullable)(MTRDeviceDelegateInfo * delegateInfo))block
{
os_unfair_lock_assert_owner(&self->_lock);

Expand Down Expand Up @@ -1198,7 +1107,6 @@ - (BOOL)_callDelegatesWithBlock:(void (^)(id<MTRDeviceDelegate> delegate))block

#ifdef DEBUG
// Only used for unit test purposes - normal delegate should not expect or handle being called back synchronously
// Returns YES if a delegate is called
- (void)_callFirstDelegateSynchronouslyWithBlock:(void (^)(id<MTRDeviceDelegate> delegate))block
{
os_unfair_lock_assert_owner(&self->_lock);
Expand Down Expand Up @@ -2496,6 +2404,8 @@ - (void)unitTestResetSubscription
// assume lock is held
- (void)_setupSubscriptionWithReason:(NSString *)reason
{
// TODO: XPC: This is not really called anymore in this class. Should
// remove this function and anything only reachable from it.
os_unfair_lock_assert_owner(&self->_lock);

if (![self _subscriptionsAllowed]) {
Expand Down
Loading

0 comments on commit 1866812

Please sign in to comment.