Client disconnect should clear subscriptions and event name change an…

…d a fix (#1292) * Cleanup subscriptions on Client close ... relevant for disconnect and close/decommission cases * Cleanup new event names on PairedNode * get will throw ... has is needed * get will throw ... has is needed * rename nodeState to state * make linter happy * Do not emit attribute/event changes twice on event * simplification
project-chip · Oct 17, 2024 · fa6aca0 · fa6aca0
1 parent a7495d2
commit fa6aca0
Show file tree

Hide file tree

Showing 6 changed files with 20 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -86,7 +86,8 @@ The main work (all changes without a GitHub username in brackets in the below li
 -   matter.js Controller API:
     -   Breaking: PairedNode instances are now created and directly returned also when the node is not et connected. This do not block code flows anymore for offline devices
     -   Breaking: Because of this  "getConnectedNode()" got renamed to "getPairedNode()"
-    -   Deprecation: The attributeChangedCallback, eventTriggeredCallback and nodeStateChangedCallbacks are deprecated and replaced by new events "attributeChanged", "eventTriggered" and "nodeStateChanged", "structureChanged" and "decommissioned" on PairedNode
+    -   Breaking: "nodeState" property on PairedNode got renamed to "state" 
+    -   Deprecation: The attributeChangedCallback, eventTriggeredCallback and nodeStateChangedCallbacks are deprecated and replaced by new events "attributeChanged", "eventTriggered" and "stateChanged", "structureChanged" and "decommissioned" on PairedNode
     -   Feature: Some more data (like Network interfaces, PowerSources, Thread details) are collected and used when connecting to the nodes
     -   Feature: Based on device type the minimum and maximum subscription interval is now automatically set based on certain best practices. When multiple nodes are subscribed all Thread based devices are initialized by a "4 in parallel queue" to limit the used thread bandwidth.
     -   Feature: Subscribed attribute data are cached for each node and used on reconnects by utilizing dataVersionFilters on read and subscribes to reduce bandwidth on reconnects. The data are no (yet) persisted, so after Controller restart the data are collected anew.

diff --git a/packages/matter.js/src/CommissioningController.ts b/packages/matter.js/src/CommissioningController.ts
@@ -195,7 +195,9 @@ export class CommissioningController extends MatterNode {
         }
         // Initialize the Storage in a compatible way for the legacy API and new style for new API
         // TODO: clean this up when we really implement ControllerNode/ClientNode concepts in new API
-        const controllerStore = environment?.get(ControllerStore) ?? new LegacyControllerStore(storage!);
+        const controllerStore = environment?.has(ControllerStore)
+            ? environment.get(ControllerStore)
+            : new LegacyControllerStore(storage!);
 
         const { netInterfaces, scanners, port } = await configureNetwork({
             ipv4Disabled: this.ipv4Disabled,
@@ -489,7 +491,7 @@ export class CommissioningController extends MatterNode {
 
             const { environment } = this.options.environment;
 
-            if (environment.get(ControllerStore) === undefined) {
+            if (!environment.has(ControllerStore)) {
                 await this.initializeControllerStore();
             }
 

diff --git a/packages/matter.js/src/device/PairedNode.ts b/packages/matter.js/src/device/PairedNode.ts
@@ -233,7 +233,7 @@ export class PairedNode {
         initializedFromRemote: AsyncObservable<[details: DeviceInformationData]>(),
 
         /** Emitted when the state of the node changes. */
-        nodeStateChanged: Observable<[nodeState: NodeStates]>(),
+        stateChanged: Observable<[nodeState: NodeStates]>(),
 
         /**
          * Emitted when an attribute value changes. If the oldValue is undefined then no former value was known.
@@ -244,7 +244,7 @@ export class PairedNode {
         eventTriggered: Observable<[DecodedEventReportValue<any>]>(),
 
         /** Emitted when the structure of the node changes (Endpoints got added or also removed). */
-        nodeStructureChanged: Observable<[void]>(),
+        structureChanged: Observable<[void]>(),
 
         /** Emitted when the node is decommissioned. */
         decommissioned: Observable<[void]>(),
@@ -325,7 +325,7 @@ export class PairedNode {
             // This kicks of the remote initialization and automatic reconnection handling if it can not be connected
             this.initialize().catch(error => {
                 logger.info(`Node ${nodeId}: Error during remote initialization`, error);
-                if (this.nodeState !== NodeStates.Disconnected) {
+                if (this.state !== NodeStates.Disconnected) {
                     this.setConnectionState(NodeStates.WaitingForDeviceDiscovery);
                     this.scheduleReconnect();
                 }
@@ -341,7 +341,7 @@ export class PairedNode {
         return this.#connectionState === NodeStates.Connected;
     }
 
-    get nodeState() {
+    get state() {
         return this.#connectionState;
     }
 
@@ -369,7 +369,7 @@ export class PairedNode {
             return;
         this.#connectionState = state;
         this.options.stateInformationCallback?.(this.nodeId, state as unknown as NodeStateInformation);
-        this.events.nodeStateChanged.emit(state);
+        this.events.stateChanged.emit(state);
         if (state === NodeStates.Disconnected) {
             this.#reconnectDelayTimer?.stop();
             this.#reconnectDelayTimer = undefined;
@@ -638,7 +638,6 @@ export class PairedNode {
 
                 asClusterClientInternal(cluster)._triggerAttributeUpdate(attributeId, value);
                 attributeChangedCallback?.(data, oldValue);
-                this.events.attributeChanged.emit(data, oldValue);
 
                 this.#checkAttributesForNeededStructureUpdate(endpointId, clusterId, attributeId);
             },
@@ -668,7 +667,6 @@ export class PairedNode {
                 asClusterClientInternal(cluster)._triggerEventUpdate(eventId, events);
 
                 eventTriggeredCallback?.(data);
-                this.events.eventTriggered.emit(data);
 
                 this.#checkEventsForNeededStructureUpdate(endpointId, clusterId, eventId);
             },
@@ -777,7 +775,7 @@ export class PairedNode {
     }
 
     private scheduleReconnect(delay?: number) {
-        if (this.nodeState !== NodeStates.WaitingForDeviceDiscovery) {
+        if (this.state !== NodeStates.WaitingForDeviceDiscovery) {
             this.setConnectionState(NodeStates.Reconnecting);
         }
 
@@ -798,7 +796,7 @@ export class PairedNode {
         const allClusterAttributes = await this.readAllAttributes();
         await this.initializeEndpointStructure(allClusterAttributes, true);
         this.options.stateInformationCallback?.(this.nodeId, NodeStateInformation.StructureChanged);
-        this.events.nodeStructureChanged.emit();
+        this.events.structureChanged.emit();
     }
 
     /** Reads all data from the device and create a device object structure out of it. */

diff --git a/packages/nodejs-shell/src/shell/cmd_nodes.ts b/packages/nodejs-shell/src/shell/cmd_nodes.ts
@@ -234,7 +234,7 @@ export default function commands(theNode: MatterNode) {
                             } else {
                                 const basicInfo = node.basicInformation;
                                 console.log(
-                                    `Node ${nodeIdToProcess}: Node Status: ${capitalize(decamelize(NodeStateInformation[node.nodeState], " "))}${basicInfo !== undefined ? ` (${basicInfo.vendorName} ${basicInfo.productName})` : ""}`,
+                                    `Node ${nodeIdToProcess}: Node Status: ${capitalize(decamelize(NodeStateInformation[node.state], " "))}${basicInfo !== undefined ? ` (${basicInfo.vendorName} ${basicInfo.productName})` : ""}`,
                                 );
                             }
                         }

diff --git a/packages/protocol/src/interaction/InteractionClient.ts b/packages/protocol/src/interaction/InteractionClient.ts
@@ -1184,12 +1184,16 @@ export class InteractionClient {
         this.#subscriptionClient.registerSubscriptionUpdateTimer(subscriptionId, timer);
     }
 
-    close() {
+    removeAllSubscriptions() {
         for (const subscriptionId of this.#ownSubscriptionIds) {
             this.removeSubscription(subscriptionId);
         }
     }
 
+    close() {
+        this.removeAllSubscriptions();
+    }
+
     get session() {
         return this.exchangeProvider.session;
     }

diff --git a/packages/protocol/src/peer/PeerSet.ts b/packages/protocol/src/peer/PeerSet.ts
@@ -332,6 +332,7 @@ export class PeerSet implements ImmutableSet<OperationalPeer>, ObservableSet<Ope
             return;
         }
 
+        this.#clients.get(address)?.removeAllSubscriptions();
         await this.#sessions.removeAllSessionsForNode(address, true);
         await this.#channels.removeAllNodeChannels(address);
     }
-Original file line number
+Diff line change
@@ Expand Up @@
                 return;
             }
+            this.#clients.get(address)?.removeAllSubscriptions();
             await this.#sessions.removeAllSessionsForNode(address, true);
             await this.#channels.removeAllNodeChannels(address);
         }
@@ Expand Down @@