Skip to content

Commit

Permalink
Add missing endWork calls in MTRClusters. (#23659)
Browse files Browse the repository at this point in the history
We were missing endWork bits for command invokes when the command has a data
(not status) response.  This would end up permanently blocking the MTRDevice's
work queue once such a command (e.g. UpdateFabricLabel) was issued.

The changes here are as follows:

1) Rearrange the logic in MTRClusters-src.zapt so that the endWork happens
   regardless of whether hasSpecificResponse is true.
2) Add a deprecation message to the timedInvokeTimeoutMs field in _response_
   command params, since that should never be used (ran into this when writing
   the tests for this PR).
3) Set resubscribeIfLost to NO on various subscriptions in MTRDeviceTests, so if
   another test takes a while we don't fail due to the canceled subscriptions
   re-subscribing and over-fullfilling the subscription expectations.
4) Add a test for a mix of passing and failing commands on MTRCluster instances,
   with some of the commands defined as getting status-only responses and some
   defined as getting data responses.  This test fails without the endWork fix.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Nov 15, 2023
1 parent aedb5d3 commit 7cd7a9e
Show file tree
Hide file tree
Showing 5 changed files with 687 additions and 153 deletions.
10 changes: 5 additions & 5 deletions src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,19 @@ using chip::SessionHandle;
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.device.nodeID controller:self.device.deviceController];
auto * bridge = new MTR{{>callbackName}}CallbackBridge(self.callbackQueue,
^(id _Nullable value, NSError * _Nullable error) {
{{#if hasSpecificResponse}}
{{! This treats completion as taking an id for the data. This is
not great from a type-safety perspective, of course. }}
completion,
completion(value, error);
{{else}}
{{! For now, don't change the bridge API; instead just use an adapter
to invoke our completion handler. This is not great from a
type-safety perspective, of course. }}
^(id _Nullable value, NSError * _Nullable error) {
completion(error);
[workItem endWork];
},
completion(error);
{{/if}}
[workItem endWork];
},
^(ExchangeManager & exchangeManager, const SessionHandle & session, {{>callbackName}}CallbackType successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
chip::Optional<uint16_t> timedInvokeTimeoutMs;
ListFreer listFreer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ MTR_NEWLY_AVAILABLE
* request) within the timeout window.
*
*/
@property (nonatomic, copy, nullable) NSNumber * timedInvokeTimeoutMs;
@property (nonatomic, copy, nullable) NSNumber * timedInvokeTimeoutMs {{#if (isStrEqual source "server")}}MTR_NEWLY_DEPRECATED("Timed invoke does not make sense for server to client commands"){{/if}};

- (instancetype)init;
- (id)copyWithZone:(NSZone * _Nullable)zone;
Expand Down
Loading

0 comments on commit 7cd7a9e

Please sign in to comment.