Make MTRServerCluster threadsafe. (#32245)

* Make MTRServerCluster threadsafe.

If two API clients are both touching the same instance of MTRServerCluster on
different threads, we should handle that correctly.

* Address review comments.

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h

@@ -24,6 +24,8 @@ NS_ASSUME_NONNULL_BEGIN
  * A representation of an attribute implemented on a server cluster by an
  * MTRDeviceController.  An attribute has an identifier and a value, and may or
  * may not be writable.
+ *
+ * MTRServerAttribute's API can be accessed from any thread.
  */
 NS_SWIFT_SENDABLE
 MTR_NEWLY_AVAILABLE
@@ -51,13 +53,13 @@ MTR_NEWLY_AVAILABLE
  */
 - (BOOL)setValue:(NSDictionary<NSString *, id> *)value;
 
-@property (nonatomic, copy, readonly) NSNumber * attributeID;
-@property (nonatomic, copy, readonly) NSDictionary<NSString *, id> * value;
+@property (atomic, copy, readonly) NSNumber * attributeID;
+@property (atomic, copy, readonly) NSDictionary<NSString *, id> * value;
 /**
  * The privilege level necessary to read this attribute.
  */
-@property (nonatomic, assign, readonly) MTRAccessControlEntryPrivilege requiredReadPrivilege;
-@property (nonatomic, assign, readonly, getter=isWritable) BOOL writable;
+@property (atomic, assign, readonly) MTRAccessControlEntryPrivilege requiredReadPrivilege;
+@property (atomic, assign, readonly, getter=isWritable) BOOL writable;
 
 @end
 

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h

@@ -23,6 +23,8 @@ NS_ASSUME_NONNULL_BEGIN
 
 /**
  * A representation of a server cluster implemented by an MTRDeviceController.
+ *
+ * MTRServerCluster's API can be accessed from any thread.
  */
 NS_SWIFT_SENDABLE
 MTR_NEWLY_AVAILABLE
@@ -90,22 +92,22 @@ MTR_NEWLY_AVAILABLE
  */
 + (MTRServerCluster *)newDescriptorCluster;
 
-@property (nonatomic, copy, readonly) NSNumber * clusterID;
+@property (atomic, copy, readonly) NSNumber * clusterID;
 
-@property (nonatomic, copy, readonly) NSNumber * clusterRevision;
+@property (atomic, copy, readonly) NSNumber * clusterRevision;
 
 /**
  * The list of entities that are allowed to access this cluster instance.  This
  * list is in addition to any endpoint-wide access grants that exist.
  *
  * Defaults to empty list, which means no additional access grants.
  */
-@property (nonatomic, copy, readonly) NSArray<MTRAccessGrant *> * accessGrants;
+@property (atomic, copy, readonly) NSArray<MTRAccessGrant *> * accessGrants;
 
 /**
  * The list of attributes supported by the cluster.
  */
-@property (nonatomic, copy, readonly) NSArray<MTRServerAttribute *> * attributes;
+@property (atomic, copy, readonly) NSArray<MTRServerAttribute *> * attributes;
 
 @end
 

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm

@@ -20,11 +20,12 @@
 #import "MTRServerAttribute_Internal.h"
 #import "MTRServerCluster_Internal.h"
 #import "MTRServerEndpoint_Internal.h"
+#import "MTRUnfairLock.h"
+#import "NSDataSpanConversion.h"
+
 #import <Matter/MTRClusterConstants.h>
 #import <Matter/MTRServerCluster.h>
 
-#import "NSDataSpanConversion.h"
-
 #include <app/AttributeAccessInterface.h>
 #include <app/clusters/descriptor/descriptor.h>
 #include <app/data-model/PreEncodedValue.h>
@@ -71,11 +72,26 @@ @implementation MTRServerCluster {
     std::unique_ptr<MTRServerAttributeAccessInterface> _attributeAccessInterface;
     // We can't use something like std::unique_ptr<EmberAfAttributeMetadata[]>
     // because EmberAfAttributeMetadata does not have a default constructor, so
-    // we can't alloc and then initializer later.
+    // we can't alloc and then initialize later.
     std::vector<EmberAfAttributeMetadata> _matterAttributeMetadata;
 
     std::unique_ptr<CommandId[]> _matterAcceptedCommandList;
     std::unique_ptr<CommandId[]> _matterGeneratedCommandList;
+
+    NSSet<MTRAccessGrant *> * _matterAccessGrants;
+
+    chip::EndpointId _parentEndpoint;
+
+    // _acceptedCommands and _generatedCommands are touched directly by our API
+    // consumer.
+    NSArray<NSNumber *> * _acceptedCommands;
+    NSArray<NSNumber *> * _generatedCommands;
+
+    /**
+     * _lock always protects access to all our mutable ivars (the ones that are
+     * modified after init).
+     */
+    os_unfair_lock _lock;
 }
 
 - (nullable instancetype)initWithClusterID:(NSNumber *)clusterID revision:(NSNumber *)revision
@@ -117,6 +133,7 @@ - (instancetype)initInternalWithClusterID:(NSNumber *)clusterID revision:(NSNumb
         return nil;
     }
 
+    _lock = OS_UNFAIR_LOCK_INIT;
     _clusterID = [clusterID copy];
     _clusterRevision = [revision copy];
     _accessGrants = [[NSMutableSet alloc] init];
@@ -141,6 +158,8 @@ - (instancetype)initInternalWithClusterID:(NSNumber *)clusterID revision:(NSNumb
 
 - (void)updateMatterAccessGrants
 {
+    os_unfair_lock_assert_owner(&_lock);
+
     MTRDeviceController * deviceController = _deviceController;
     if (deviceController == nil) {
         // _matterAccessGrants will be updated when we get bound to a controller.
@@ -149,27 +168,41 @@ - (void)updateMatterAccessGrants
 
     NSSet * grants = [_accessGrants copy];
     [deviceController asyncDispatchToMatterQueue:^{
+        std::lock_guard lock(self->_lock);
         self->_matterAccessGrants = grants;
     }
                                     errorHandler:nil];
 }
 
 - (void)addAccessGrant:(MTRAccessGrant *)accessGrant
 {
+    std::lock_guard lock(self->_lock);
+
     [_accessGrants addObject:accessGrant];
 
     [self updateMatterAccessGrants];
 }
 
 - (void)removeAccessGrant:(MTRAccessGrant *)accessGrant;
 {
+    std::lock_guard lock(self->_lock);
+
     [_accessGrants removeObject:accessGrant];
 
     [self updateMatterAccessGrants];
 }
 
+- (NSArray<MTRAccessGrant *> *)matterAccessGrants
+{
+    std::lock_guard lock(self->_lock);
+
+    return [_matterAccessGrants allObjects];
+}
+
 - (BOOL)addAttribute:(MTRServerAttribute *)attribute
 {
+    std::lock_guard lock(self->_lock);
+
     MTRDeviceController * deviceController = _deviceController;
     if (deviceController != nil) {
         MTR_LOG_ERROR("Cannot add attribute on cluster %llx which is already in use", _clusterID.unsignedLongLongValue);
@@ -213,6 +246,8 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
 
 - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 {
+    std::lock_guard lock(self->_lock);
+
     MTRDeviceController * existingController = _deviceController;
     if (existingController != nil) {
 #if MTR_PER_CONTROLLER_STORAGE_ENABLED
@@ -313,14 +348,16 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 
     _deviceController = controller;
 
-    MTR_LOG_DEFAULT("Associated %@, attribute count %llu, with controller", self,
+    MTR_LOG_DEFAULT("Associated %@, attribute count %llu, with controller", [self _descriptionWhileLocked],
         static_cast<unsigned long long>(attributeCount));
 
     return YES;
 }
 
 - (void)invalidate
 {
+    std::lock_guard lock(_lock);
+
     // Undo any work associateWithController did.
     for (MTRServerAttribute * attr in _attributes) {
         [attr invalidate];
@@ -342,6 +379,8 @@ - (void)registerMatterCluster
 {
     assertChipStackLockedByCurrentThread();
 
+    std::lock_guard lock(_lock);
+
     if (!registerAttributeAccessOverride(_attributeAccessInterface.get())) {
         // This should only happen if we somehow managed to register an
         // AttributeAccessInterface for the same (endpoint, cluster) pair.
@@ -354,45 +393,93 @@ - (void)unregisterMatterCluster
 {
     assertChipStackLockedByCurrentThread();
 
+    std::lock_guard lock(_lock);
+
     if (_attributeAccessInterface != nullptr) {
         unregisterAttributeAccessOverride(_attributeAccessInterface.get());
     }
 }
 
 - (NSArray<MTRAccessGrant *> *)accessGrants
 {
+    std::lock_guard lock(_lock);
+
     return [_accessGrants allObjects];
 }
 
 - (NSArray<MTRServerAttribute *> *)attributes
 {
+    std::lock_guard lock(_lock);
+
     return [_attributes copy];
 }
 
-- (void)setParentEndpoint:(EndpointId)endpoint
+- (BOOL)addToEndpoint:(chip::EndpointId)endpoint
 {
+    std::lock_guard lock(_lock);
+
+    if (_parentEndpoint != kInvalidEndpointId) {
+        MTR_LOG_ERROR("Cannot add cluster " ChipLogFormatMEI " to endpoint %" PRIu32 "; already added to endpoint %" PRIu32,
+            ChipLogValueMEI(_clusterID.unsignedLongLongValue), endpoint, _parentEndpoint);
+        return NO;
+    }
+
     _parentEndpoint = endpoint;
     // Update it on all the attributes, in case the attributes were added to us
     // before we were added to the endpoint.
     for (MTRServerAttribute * attr in _attributes) {
         [attr updateParentCluster:ConcreteClusterPath(endpoint, static_cast<ClusterId>(_clusterID.unsignedLongLongValue))];
     }
+    return YES;
+}
+
+- (chip::EndpointId)parentEndpoint
+{
+    std::lock_guard lock(_lock);
+    return _parentEndpoint;
 }
 
 - (Span<const EmberAfAttributeMetadata>)matterAttributeMetadata
 {
     // This is always called after our _matterAttributeMetadata has been set up
     // by associateWithController.
+    std::lock_guard lock(_lock);
     return Span<const EmberAfAttributeMetadata>(_matterAttributeMetadata.data(), _matterAttributeMetadata.size());
 }
 
+- (void)setAcceptedCommands:(NSArray<NSNumber *> *)acceptedCommands
+{
+    std::lock_guard lock(_lock);
+    _acceptedCommands = [acceptedCommands copy];
+}
+
+- (NSArray<NSNumber *> *)acceptedCommands
+{
+    std::lock_guard lock(_lock);
+    return [_acceptedCommands copy];
+}
+
+- (void)setGeneratedCommands:(NSArray<NSNumber *> *)generatedCommands
+{
+    std::lock_guard lock(_lock);
+    _generatedCommands = [generatedCommands copy];
+}
+
+- (NSArray<NSNumber *> *)generatedCommands
+{
+    std::lock_guard lock(_lock);
+    return [_generatedCommands copy];
+}
+
 - (CommandId *)matterAcceptedCommands
 {
+    std::lock_guard lock(_lock);
     return _matterAcceptedCommandList.get();
 }
 
 - (CommandId *)matterGeneratedCommands
 {
+    std::lock_guard lock(_lock);
     return _matterGeneratedCommandList.get();
 }
 
@@ -413,6 +500,13 @@ - (CommandId *)matterGeneratedCommands
 
 - (NSString *)description
 {
+    std::lock_guard lock(_lock);
+    return [self _descriptionWhileLocked];
+}
+
+- (NSString *)_descriptionWhileLocked
+{
+    os_unfair_lock_assert_owner(&_lock);
     return [NSString stringWithFormat:@"<MTRServerCluster endpoint %u, id " ChipLogFormatMEI ">",
                      _parentEndpoint, ChipLogValueMEI(_clusterID.unsignedLongLongValue)];
 }

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h

@@ -57,40 +57,46 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)invalidate;
 
 /**
- * The access grants the Matter stack can observe.  Only modified while in
- * Initializing state or on the Matter queue.
+ * Add the cluster to an endpoint with the given endpoint ID.  Will return NO
+ * if the cluster is already added to an endpoint.
  */
-@property (nonatomic, strong, readonly) NSSet<MTRAccessGrant *> * matterAccessGrants;
+- (BOOL)addToEndpoint:(chip::EndpointId)endpoint;
+
+/**
+ * The access grants the Matter stack can observe.  Only modified while
+ * associating with a controller or on the Matter queue.
+ */
+@property (atomic, copy, readonly) NSArray<MTRAccessGrant *> * matterAccessGrants;
 
 /**
  * parentEndpoint will be kInvalidEndpointId until the cluster is added to an endpoint.
  */
-@property (nonatomic, assign) chip::EndpointId parentEndpoint;
+@property (atomic, assign, readonly) chip::EndpointId parentEndpoint;
 
 /**
  * The attribute metadata for the cluster.  Only valid after associateWithController: has succeeded.
  */
-@property (nonatomic, assign, readonly) chip::Span<const EmberAfAttributeMetadata> matterAttributeMetadata;
+@property (atomic, assign, readonly) chip::Span<const EmberAfAttributeMetadata> matterAttributeMetadata;
 
 /**
  * The list of accepted command IDs.
  */
-@property (nonatomic, copy, nullable) NSArray<NSNumber *> * acceptedCommands;
+@property (atomic, copy, nullable) NSArray<NSNumber *> * acceptedCommands;
 
 /**
  * The list of generated command IDs.
  */
-@property (nonatomic, copy, nullable) NSArray<NSNumber *> * generatedCommands;
+@property (atomic, copy, nullable) NSArray<NSNumber *> * generatedCommands;
 
 /**
  * The list of accepted commands IDs in the format the Matter stack needs.
  */
-@property (nonatomic, assign, nullable, readonly) chip::CommandId * matterAcceptedCommands;
+@property (atomic, assign, nullable, readonly) chip::CommandId * matterAcceptedCommands;
 
 /**
  * The list of generated commands IDs in the format the Matter stack needs.
  */
-@property (nonatomic, assign, nullable, readonly) chip::CommandId * matterGeneratedCommands;
+@property (atomic, assign, nullable, readonly) chip::CommandId * matterGeneratedCommands;
 
 @end