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

[Darwin] MTRDevice should trigger resubscription on connectivity changes #33016

Merged
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
51 changes: 51 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#import "MTRCommandTimedCheck.h"
#import "MTRConversion.h"
#import "MTRDefines_Internal.h"
#import "MTRDeviceConnectivityMonitor.h"
#import "MTRDeviceControllerOverXPC.h"
#import "MTRDeviceController_Internal.h"
#import "MTRDevice_Internal.h"
Expand Down Expand Up @@ -355,6 +356,7 @@ @implementation MTRDevice {
// _setupSubscription or via the auto-resubscribe behavior of the
// ReadClient). Nil if we have had no such failures.
NSDate * _Nullable _lastSubscriptionFailureTime;
MTRDeviceConnectivityMonitor * _connectivityMonitor;
}

- (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
Expand Down Expand Up @@ -669,6 +671,8 @@ - (void)invalidate
// subscription. In that case, _internalDeviceState will update when the
// subscription is actually terminated.

[self _stopConnectivityMonitoring];

os_unfair_lock_unlock(&self->_lock);
}

Expand Down Expand Up @@ -861,6 +865,9 @@ - (void)_handleSubscriptionEstablished

[self _changeState:MTRDeviceStateReachable];

// No need to monitor connectivity after subscription establishment
[self _stopConnectivityMonitoring];

os_unfair_lock_unlock(&self->_lock);

os_unfair_lock_lock(&self->_timeSyncLock);
Expand Down Expand Up @@ -894,6 +901,9 @@ - (void)_handleResubscriptionNeeded
// former case we recently had a subscription and do not want to be forcing
// retries immediately.
_lastSubscriptionFailureTime = [NSDate now];

// Set up connectivity monitoring in case network routability changes for the positive, to accellerate resubscription
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"accelerate".

@nivi-apple had commented about this too; it was resolved but not changed?

[self _setupConnectivityMonitoring];
}

- (void)_handleSubscriptionReset:(NSNumber * _Nullable)retryDelay
Expand Down Expand Up @@ -1241,6 +1251,44 @@ - (void)_createDataVersionFilterListFromDictionary:(NSDictionary<MTRClusterPath
*count = maxDataVersionFilterSize;
}

- (void)_setupConnectivityMonitoring
{
// Dispatch to own queue first to avoid deadlock with syncGetCompressedFabricID
dispatch_async(self.queue, ^{
// Get the required info before setting up the connectivity monitor
NSNumber * compressedFabricID = [self->_deviceController syncGetCompressedFabricID];
if (!compressedFabricID) {
MTR_LOG_INFO("%@ could not get compressed fabricID", self);
return;
}

// Now lock for _connectivityMonitor
std::lock_guard lock(self->_lock);
if (self->_connectivityMonitor) {
// already monitoring
return;
}

self->_connectivityMonitor = [[MTRDeviceConnectivityMonitor alloc] initWithCompressedFabricID:compressedFabricID nodeID:self.nodeID];
[self->_connectivityMonitor startMonitoringWithHandler:^{
[self->_deviceController asyncDispatchToMatterQueue:^{
[self _triggerResubscribeWithReason:"read-through skipped while not subscribed" nodeLikelyReachable:YES];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That ... does not look like the right reason.

}
errorHandler:nil];
} queue:self.queue];
});
}

- (void)_stopConnectivityMonitoring
{
os_unfair_lock_assert_owner(&_lock);

if (_connectivityMonitor) {
[_connectivityMonitor stopMonitoring];
_connectivityMonitor = nil;
}
}

// assume lock is held
- (void)_setupSubscription
{
Expand Down Expand Up @@ -1462,6 +1510,9 @@ - (void)_setupSubscription
callback->AdoptClusterStateCache(std::move(clusterStateCache));
callback.release();
}];

// Set up connectivity monitoring in case network becomes routable after any part of the subscription process goes into backoff retries.
[self _setupConnectivityMonitoring];
}

#ifdef DEBUG
Expand Down
43 changes: 43 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* Copyright (c) 2023 Project CHIP Authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Again, this was resolved but not changed? I am guessing someone applied suggestions and then (a different? same?) someone force-pushed and overwrote this.

Which means: please go through all prior comments on this PR from @nivi-apple and @ksperling-apple and so on and make sure they get addressed.

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#import <Foundation/Foundation.h>

#import "MTRDefines_Internal.h"

NS_ASSUME_NONNULL_BEGIN

typedef void (^MTRDeviceConnectivityMonitorHandler)(void);

/**
* Class that a matter dns-sd instance name, and monitors connectivity to the device.
*/
MTR_TESTABLE
@interface MTRDeviceConnectivityMonitor : NSObject
- (instancetype)initWithCompressedFabricID:(NSNumber *)compressedFabricID nodeID:(NSNumber *)nodeID;

/**
* Any time a path becomes satisfied or route becomes viable, the registered handler will be called.
*/
- (void)startMonitoringWithHandler:(MTRDeviceConnectivityMonitorHandler)handler queue:(dispatch_queue_t)queue;

/**
* Stops the monitoring. After this method returns no more calls to the handler will be made.
*/
- (void)stopMonitoring;
@end

NS_ASSUME_NONNULL_END
Loading
Loading