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

Do not set up subscription when setting a delegate for a MTRDevice wi… #33559

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -685,11 +685,17 @@ - (void)_setDSTOffsets:(NSArray<MTRTimeSynchronizationClusterDSTOffsetStruct *>
#define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN (10 * 60) // 10 minutes (for now)
#define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX (60 * 60) // 60 minutes

- (BOOL)_subscriptionsAllowed
{
return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
woody-apple marked this conversation as resolved.
Show resolved Hide resolved
}

- (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queue
{
MTR_LOG_INFO("%@ setDelegate %@", self, delegate);

BOOL setUpSubscription = YES;
// We should not set up a subscription for device controllers over XPC.
woody-apple marked this conversation as resolved.
Show resolved Hide resolved
BOOL setUpSubscription = [self _subscriptionsAllowed];
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved

// For unit testing only
#ifdef DEBUG
Expand Down Expand Up @@ -933,6 +939,14 @@ - (void)_changeInternalState:(MTRInternalDeviceState)state
}
}

#ifdef DEBUG
- (MTRInternalDeviceState)_getInternalState
{
std::lock_guard lock(self->_lock);
return _internalDeviceState;
}
#endif

// First Time Sync happens 2 minutes after reachability (this can be changed in the future)
#define MTR_DEVICE_TIME_UPDATE_INITIAL_WAIT_TIME_SEC (60 * 2)
- (void)_handleSubscriptionEstablished
Expand Down Expand Up @@ -1680,6 +1694,11 @@ - (void)_setupSubscription
{
os_unfair_lock_assert_owner(&self->_lock);

if (![self _subscriptionsAllowed]) {
MTR_LOG("_setupSubscription: Subscriptions not allowed. Do not set up subscription");
return;
}

#ifdef DEBUG
id delegate = _weakDelegate.strongObject;
Optional<System::Clock::Seconds32> maxIntervalOverride;
Expand Down
30 changes: 30 additions & 0 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -3618,6 +3618,36 @@ - (void)test034_TestMTRDeviceHistoricalEvents
XCTAssertTrue(eventReportsReceived > 0);
}

- (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC
{
NSString * const MTRDeviceControllerId = @"MTRController";
__auto_type remoteController = [MTRDeviceController
sharedControllerWithID:MTRDeviceControllerId
xpcConnectBlock:^NSXPCConnection * _Nonnull {
return nil;
}];

__auto_type * device = [MTRDevice deviceWithNodeID:kDeviceId deviceController:remoteController];
dispatch_queue_t queue = dispatch_get_main_queue();

// We should not set up a subscription when creating a MTRDevice with a remote controller.
XCTestExpectation * subscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up"];
subscriptionExpectation.inverted = YES;

__auto_type * delegate = [[MTRDeviceTestDelegate alloc] init];

XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed);
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved

delegate.onAttributeDataReceived = ^(NSArray<NSDictionary<NSString *, id> *> * attributeReport) {
[subscriptionExpectation fulfill];
};

[device setDelegate:delegate queue:queue];
[self waitForExpectations:@[ subscriptionExpectation ] timeout:30];
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved

XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed);
}

@end

@interface MTRDeviceEncoderTests : XCTestCase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,31 @@ NS_ASSUME_NONNULL_BEGIN
@end

@interface MTRDevice (TestDebug)
typedef NS_ENUM(NSUInteger, MTRInternalDeviceState) {
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
// Unsubscribed means we do not have a subscription and are not trying to set one up.
MTRInternalDeviceStateUnsubscribed = 0,
// Subscribing means we are actively trying to establish our initial subscription (e.g. doing
// DNS-SD discovery, trying to establish CASE to the peer, getting priming reports, etc).
MTRInternalDeviceStateSubscribing = 1,
// InitialSubscriptionEstablished means we have at some point finished setting up a
// subscription. That subscription may have dropped since then, but if so it's the ReadClient's
// responsibility to re-establish it.
MTRInternalDeviceStateInitialSubscriptionEstablished = 2,
// Resubscribing means we had established a subscription, but then
// detected a subscription drop due to not receiving a report on time. This
// covers all the actions that happen when re-subscribing (discovery, CASE,
// getting priming reports, etc).
MTRInternalDeviceStateResubscribing = 3,
// LaterSubscriptionEstablished meant that we had a subscription drop and
// then re-created a subscription.
MTRInternalDeviceStateLaterSubscriptionEstablished = 4,
};

- (void)unitTestInjectEventReport:(NSArray<NSDictionary<NSString *, id> *> *)eventReport;
- (void)unitTestInjectAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport fromSubscription:(BOOL)isFromSubscription;
- (NSUInteger)unitTestAttributesReportedSinceLastCheck;
- (void)unitTestClearClusterData;
- (MTRInternalDeviceState)_getInternalState;
@end
#endif

Expand Down
Loading