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.
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h
index 7d8c27b..f9c7e7c 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAttribute.h
@@ -24,6 +24,8 @@
* 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 @@
*/
- (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
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h
index 02b0f4b..591d3af 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.h
@@ -23,6 +23,8 @@
/**
* 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,9 +92,9 @@
*/
+ (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
@@ -100,12 +102,12 @@
*
* 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
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm
index c0fba9e..7439c0f 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm
+++ b/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 @@
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 @@
return nil;
}
+ _lock = OS_UNFAIR_LOCK_INIT;
_clusterID = [clusterID copy];
_clusterRevision = [revision copy];
_accessGrants = [[NSMutableSet alloc] init];
@@ -141,6 +158,8 @@
- (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,6 +168,7 @@
NSSet * grants = [_accessGrants copy];
[deviceController asyncDispatchToMatterQueue:^{
+ std::lock_guard lock(self->_lock);
self->_matterAccessGrants = grants;
}
errorHandler:nil];
@@ -156,6 +176,8 @@
- (void)addAccessGrant:(MTRAccessGrant *)accessGrant
{
+ std::lock_guard lock(self->_lock);
+
[_accessGrants addObject:accessGrant];
[self updateMatterAccessGrants];
@@ -163,13 +185,24 @@
- (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)associateWithController:(nullable MTRDeviceController *)controller
{
+ std::lock_guard lock(self->_lock);
+
MTRDeviceController * existingController = _deviceController;
if (existingController != nil) {
#if MTR_PER_CONTROLLER_STORAGE_ENABLED
@@ -313,7 +348,7 @@
_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;
@@ -321,6 +356,8 @@
- (void)invalidate
{
+ std::lock_guard lock(_lock);
+
// Undo any work associateWithController did.
for (MTRServerAttribute * attr in _attributes) {
[attr invalidate];
@@ -342,6 +379,8 @@
{
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,6 +393,8 @@
{
assertChipStackLockedByCurrentThread();
+ std::lock_guard lock(_lock);
+
if (_attributeAccessInterface != nullptr) {
unregisterAttributeAccessOverride(_attributeAccessInterface.get());
}
@@ -361,38 +402,84 @@
- (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 @@
- (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)];
}
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h
index 4ae3d0d..253885b 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster_Internal.h
@@ -57,40 +57,46 @@
- (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
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm
index c58b1ac..c755323 100644
--- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm
+++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm
@@ -154,20 +154,18 @@
return NO;
}
- if (serverCluster.parentEndpoint != kInvalidEndpointId) {
- MTR_LOG_ERROR("Cannot add cluster to endpoint %llu; already added to endpoint %" PRIu32, _endpointID.unsignedLongLongValue, serverCluster.parentEndpoint);
- return NO;
- }
-
for (MTRServerCluster * existingCluster in _serverClusters) {
if ([existingCluster.clusterID isEqual:serverCluster.clusterID]) {
- MTR_LOG_ERROR("Cannot add second cluster with ID %llx on endpoint %llu", serverCluster.clusterID.unsignedLongLongValue, _endpointID.unsignedLongLongValue);
+ MTR_LOG_ERROR("Cannot add second cluster with ID " ChipLogFormatMEI " on endpoint %llu", ChipLogValueMEI(serverCluster.clusterID.unsignedLongLongValue), _endpointID.unsignedLongLongValue);
return NO;
}
}
+ if (![serverCluster addToEndpoint:static_cast<EndpointId>(_endpointID.unsignedLongLongValue)]) {
+ return NO;
+ }
[_serverClusters addObject:serverCluster];
- serverCluster.parentEndpoint = static_cast<EndpointId>(_endpointID.unsignedLongLongValue);
+
return YES;
}
@@ -401,7 +399,7 @@
NSMutableArray<MTRAccessGrant *> * grants = [[_matterAccessGrants allObjects] mutableCopy];
for (MTRServerCluster * cluster in _serverClusters) {
if ([cluster.clusterID isEqual:clusterID]) {
- [grants addObjectsFromArray:[cluster.matterAccessGrants allObjects]];
+ [grants addObjectsFromArray:cluster.matterAccessGrants];
}
}
