Refined Topic Alias support. (Implement #1300) #1301

redboltz · 2021-07-12T01:23:20Z

Add automatic topic alias management functionality.

On PUBLISH sending, the client can automatic using/assin Topic
Alias (optional).
On PUBLISH receiving, the topic parameter of on message handler is
automatically complemented but the packet.topic preserves the original
topic.

Fix invalid tests.

Add automatic topic alias management functionality. - On PUBLISH sending, the client can automatic using/assin Topic Alias (optional). - On PUBLISH receiving, the topic parameter of on message handler is automatically complemented but the packet.topic preserves the original topic. Fix invalid tests.

YoDaMa · 2021-10-04T14:16:21Z

@redboltz my next question, do you think this is changing the behavior of the existing API at all?

YoDaMa · 2021-10-04T14:16:35Z

README.md

@@ -206,7 +206,41 @@ the final connection when it drops.
 The default value is 1000 ms which means it will try to reconnect 1 second
 after losing the connection.

+<a name="topicalias"></a>
+## About Topic Alias management


nit: caps on Management

Thanks. I will fix it.

redboltz · 2021-10-04T14:27:59Z

@redboltz my next question, do you think this is changing the behavior of the existing API at all?

No. I don't think so.

Interface

This PR adds options. Not contains breaking changes for existing APIs.

Behavior

Auto Use/Assign Topic Alias functionality are enabled only if the user set options autoAssignTopicAlias or autoUseTopicAlias.
They are not set by default. So existing APIs behave the same as before.

There is one exception. When QoS1/2 packets that use Topic Alias are inserted into the store, Topic Alias is removed and Topic Name is recovered. It is expected behavior by MQTT spec. So I think this behavior change is a bug fix.

YoDaMa · 2021-10-04T14:28:25Z

lib/client.js

+}
+
+function removeTopicAlias (client, packet) {
+  // remove topic alias because it shouldn't be used on re-sending


can you add a bit of a better comment here. What's the use for this function? Is it removing a topic alias because some other topic has overridden the topic?

This is a kind of utility function for internal use.
I think that the commend should be removed here.
This function removes Topic Alias property and recover Topic Name from the Topic Alias.
Perhaps the function name should be removeTopicAliasAndRecoverTopicName. Is this too long?
I think that the comment should be written the caller side.

lib/client.js

lib/topic-alias-send.js

YoDaMa · 2021-10-04T14:38:07Z

package.json

@@ -73,6 +74,7 @@
    "number-allocator": "^1.0.7",
    "pump": "^3.0.0",
    "readable-stream": "^3.6.0",
+    "rfdc": "^1.3.0",


always hesitant to add more dependencies. Could you explain the purpose of rfdc and collections, and why there isn't a suitable Node.js / JS builtin that can perform their tasks?

rfdc is for clone that I mentioned at #1301 (comment)
It is efficient and straightforward notation. It seems that the library is well maintained and the big number of weekly downloads. See Benchmarks https://www.npmjs.com/package/rfdc

AFAIK, Node.js/JS there is no straightforward way.
One of the built-in way is as follows:

function clone(a) { return JSON.parse(JSON.stringify(a)); }

https://stackoverflow.com/questions/6089058/nodejs-how-to-clone-an-object

it seems to have created alot of issues with collections...

see there :
#homebridge-plugins/homebridge-camera-ffmpeg#1240
#NorthernMan54/homebridge-alexa#481
#dgreif/ring#844
and probably many more...

YoDaMa · 2021-10-04T14:46:27Z

test/client_mqtt5.js

+          qos: 0,
+          properties: { topicAlias: 1 }
+        })
+        // overwrite register topicAlias


nit: "overwrite registered topicAlias"

Thanks. I will fix it.

YoDaMa · 2021-10-04T14:49:20Z

test/client_mqtt5.js

+      // use topicAlias
+      client.publish('', 'Message', { properties: { topicAlias: 1 } })
+      // use topicAlias by autoApplyTopicAlias
+      client.publish('test1', 'Message')


so when you enable autoApplyTopicAlias it will substitute the alias for the message, and existing behaviour without it set to true is that on client.publish('test1', 'Message'), the topicAlias already registered wouldn't be used?

Yes. If autoApplyTopicAlias is true, and the user wants to use Topic Alias, the user needs to set the empty TopicName and add Topic Alias property manually.

YoDaMa · 2021-10-04T14:51:00Z

test/client_mqtt5.js

+      client.publish('test3', 'Message')
+
+      // renew LRU topicAlias
+      client.publish('test4', 'Message')


so here the topic alias will be updated dynamically because autoAssignTopicAlias is used?

YoDaMa

what happens on reconnect? or client.end? is there persistence of the topicAliases? Not 100% on the MQTT spec for this scenario...

YoDaMa · 2021-10-04T14:52:13Z

also, great PR. Thanks for providing this along with tests. @redboltz great job.

YoDaMa · 2021-10-04T14:54:08Z

lib/topic-alias-send.js

+  if (entry) {
+    delete this.topicToAlias[entry.topic]
+  }
+  this.aliasToTopic.set(alias, {'topic': topic, 'alias': alias})


is this redundant to have the set method on aliasToTopic and topicToAlias as a general JS object?

this.aliasToTopic = new LruMap() this.topicToAlias = {}

aliasToTopic is an instance of LruMap(). aliasToTopic requires LRU for overwriting. So it cannot be a simple JS Object.
topicToAlias is JS Object. It doesn't require LRU.

Could you elaborate your question?
Do you mean add set method to topicToAlias ?
And update the code using this/topicToAlias.set(topic, alias) instead of this.topicToAlias[topic] = alias ?
I personally think it is redundant.

yes that makes sense. Ignore my initial question. New question: why {topic: topic, alias: alias}, when the information for the alias is the key?

This is for

return this.aliasToTopic.min().alias

See https://github.com/mqttjs/MQTT.js/pull/1301/files/f8178aab47846ba7f2f34e436bc1ff3d5f23de59#diff-ae7ca6f8f9c9488b9789056d3b1e4aee44861bb0f86c3000c8361bbad8c7bb28R90

lru-map only provide least value not key. I tried to find getting key-value entry way but it seems that it doesn't exist. https://www.collectionsjs.com/lru-map

That's why I added alias not only as key but also as value.

YoDaMa · 2021-10-04T14:57:48Z

lib/topic-alias-send.js

+}
+
+/**
+ * Get LRU topic alias


Expand LRU to "Get Least Recently Used (LRU) topic alias"

Ok, I will fix it.

redboltz · 2021-10-04T15:11:06Z

what happens on reconnect? or client.end? is there persistence of the topicAliases? Not 100% on the MQTT spec for this scenario...

See #1300 (comment) Topic Alias lifetime and Impact for implementation.

Topic Alias lifetime is during connection, not session. So the client/server cannot use Topic Aliases that is used by the previous connection. This is the spec.
In order to satisfy the spec, the client library needs to manage store carefully. So this is the reason for removeTopicAlias().

YoDaMa · 2021-10-04T15:23:57Z

pending nits on comments, this PR looks good to me. Will approve once changes are pushed.

…AndRecoverTopicName`. Add comments for caller side of `removeTopicAliasAndRecoverTopicName`.

redboltz · 2021-10-04T16:43:46Z

pending nits on comments, this PR looks good to me. Will approve once changes are pushed.

I think that I've done all requested fixes. Please check them.

YoDaMa

lgtm

YoDaMa · 2021-10-04T20:38:23Z

thank you @redboltz!

redboltz · 2021-10-05T00:16:06Z

@YoDaMa Thank you for merging !!

I noticed one minor behavior change.

MQTT.js/test/client_mqtt5.js

Lines 68 to 79 in e3e15c3

    
           client.on('message', function (topic, messagee, packet) { 
        
             switch (publishCount++) { 
        
               case 0: 
        
                 assert.strictEqual(topic, 'test1') 
        
                 assert.strictEqual(packet.topic, 'test1') 
        
                 assert.strictEqual(packet.properties.topicAlias, 1) 
        
                 break 
        
               case 1: 
        
                 assert.strictEqual(topic, 'test1') 
        
                 assert.strictEqual(packet.topic, '') 
        
                 assert.strictEqual(packet.properties.topicAlias, 1) 
        
                 break

On message handler, when broker sends an empty TopicName and registered TopicAliasProperty, the first parameter topic is automatically overwritten as registered TopicName. But the third parameter packet preserve the actual received packet.

It always happens even if TopicAlias related options are not set. But I believe that it is expected behavior for users.
Users can know both registered TopicName and actual received packet.

YoDaMa · 2021-12-28T22:53:32Z

oh no! so what should be done? Get Outlook for iOS<https://aka.ms/o0ukef>

________________________________ From: Nebz ***@***.***> Sent: Tuesday, December 28, 2021 8:01:54 AM To: mqttjs/MQTT.js ***@***.***> Cc: Yoseph Maguire (HE/HIM) ***@***.***>; Mention ***@***.***> Subject: Re: [mqttjs/MQTT.js] Refined Topic Alias support. (Implement #1300) (#1301) @NebzHB commented on this pull request.

________________________________ In package.json<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmqttjs%2FMQTT.js%2Fpull%2F1301%23discussion_r775922312&data=04%7C01%7Cyoseph.maguire%40microsoft.com%7Ccd1a906dc5c74e88532f08d9ca0a9b40%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637762969189129568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=81%2F3W7NpkKyK258QdVZIU5tl8%2FbeXq5AqpOjgLLs3Ng%3D&reserved=0>:

@@ -73,6 +74,7 @@

"number-allocator": "^1.0.7", "pump": "^3.0.0", "readable-stream": "^3.6.0", + "rfdc": "^1.3.0", it seems to have created alot of issues with collections... see there : #Sunoo/homebridge-camera-ffmpeg#1240<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSunoo%2Fhomebridge-camera-ffmpeg%2Fissues%2F1240&data=04%7C01%7Cyoseph.maguire%40microsoft.com%7Ccd1a906dc5c74e88532f08d9ca0a9b40%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637762969189129568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GBy8KntoMdjkiqcNMbp2zU7jkuxP6pw776PB2%2FBEIus%3D&reserved=0> #NorthernMan54/homebridge-alexa#481<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FNorthernMan54%2Fhomebridge-alexa%2Fissues%2F481&data=04%7C01%7Cyoseph.maguire%40microsoft.com%7Ccd1a906dc5c74e88532f08d9ca0a9b40%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637762969189129568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=rzpWlk1c6SwOp5msd%2FE1Uoy%2FSXotBqS6EKvJPeSoq3s%3D&reserved=0> #dgreif/ring#844<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdgreif%2Fring%2Fissues%2F844&data=04%7C01%7Cyoseph.maguire%40microsoft.com%7Ccd1a906dc5c74e88532f08d9ca0a9b40%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637762969189129568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=DFIomrq%2BKlbhdjH4ODw8L1Pc8KpgvOVn9EDTG86WyQo%3D&reserved=0> and probably many more... — Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmqttjs%2FMQTT.js%2Fpull%2F1301%23discussion_r775922312&data=04%7C01%7Cyoseph.maguire%40microsoft.com%7Ccd1a906dc5c74e88532f08d9ca0a9b40%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637762969189179564%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5WmTO9euQRc4WdmHEdJwleQVsssO1XqlDdyOFgGjCMo%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAB6VMLB7TD6CHJ4AFRTA3DDUTG7NFANCNFSM5AF3KMIQ&data=04%7C01%7Cyoseph.maguire%40microsoft.com%7Ccd1a906dc5c74e88532f08d9ca0a9b40%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637762969189179564%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=EQ9qfgoJWumPTWJvO1lAvDe%2FAzKgSfEG4beNvxTtE%2FI%3D&reserved=0>. You are receiving this because you were mentioned.Message ID: ***@***.***>

redboltz · 2021-12-28T23:34:14Z

I think that collections should be replaced some other library.
The requirement is https://stackoverflow.com/questions/65948823/sorted-ordered-collection-for-javascript
I started finding the library. If someone has information, it is very welcome.

redboltz · 2021-12-29T01:39:51Z

I think that collections should be replaced some other library. The requirement is https://stackoverflow.com/questions/65948823/sorted-ordered-collection-for-javascript I started finding the library. If someone has information, it is very welcome.

Sorry I looked over montagejs/collections#241 (comment). I said "replacing" but it was my overreaction.
I wait the fix and release.

Just in case, I continue to look for altenative one.

redboltz · 2021-12-29T05:46:17Z

I analyzed the issue and commected montagejs/collections#241 (comment)
When the next version of collections (5.1.13?) would be released, I will update it. (on number-allocator and MQTT.js)
It would be a partial fix of #1385 . "partial" means if other out of MQTT.js package or application use 5.1.12 (current release) or older collections.js, the user still has the problem.

collections.js updates Array.
I personally think that updating (overwriting) intrinsic Array.from() is not good way. I guess that there is some historycal reason (older browser support?).

redboltz force-pushed the add_topic_alias_support branch from 5b89b94 to f8178aa Compare July 15, 2021 08:09

redboltz mentioned this pull request Sep 29, 2021

Improving idea of Topic Alias #1300

Closed

YoDaMa reviewed Oct 4, 2021

View reviewed changes

lib/client.js Show resolved Hide resolved

YoDaMa reviewed Oct 4, 2021

View reviewed changes

lib/topic-alias-send.js Show resolved Hide resolved

YoDaMa reviewed Oct 4, 2021

View reviewed changes

redboltz added 4 commits October 5, 2021 00:45

Fix typo.

7b13334

Fix comment.

3e0deb5

Rename the function name from removeTopicAlias to `removeTopicAlias…

b1a2632

…AndRecoverTopicName`. Add comments for caller side of `removeTopicAliasAndRecoverTopicName`.

Captalize label.

92e1519

YoDaMa approved these changes Oct 4, 2021

View reviewed changes

YoDaMa merged commit c92b877 into mqttjs:master Oct 4, 2021

NorthernMan54 mentioned this pull request Dec 28, 2021

RangeError: Maximum call stack size exceeded #1385

Closed

This was referenced Dec 29, 2021

Updated collections.js related package version. #1386

Merged

Inclusion of "collections" package breaks my project unit tests #1392

Closed

BertKleewein mentioned this pull request Jan 14, 2022

fix drain leak #1401

Merged

This was referenced Feb 21, 2022

server's topicAliasMaximum is not relevant to client's topicAliasMaximum #1183

Closed

Differentiate CONNECT packets properties and CONNACK packets properties #1200

Closed

redboltz mentioned this pull request Aug 16, 2022

Fix #1524 and #1525. #1526

Closed

Refined Topic Alias support. (Implement #1300) #1301

Refined Topic Alias support. (Implement #1300) #1301

Conversation

redboltz commented Jul 12, 2021

YoDaMa commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redboltz commented Oct 4, 2021

Interface

Behavior

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoDaMa left a comment

Choose a reason for hiding this comment

YoDaMa commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redboltz commented Oct 4, 2021

YoDaMa commented Oct 4, 2021

redboltz commented Oct 4, 2021

YoDaMa left a comment

Choose a reason for hiding this comment

YoDaMa commented Oct 4, 2021

redboltz commented Oct 5, 2021

YoDaMa commented Dec 28, 2021 via email

redboltz commented Dec 28, 2021

redboltz commented Dec 29, 2021

redboltz commented Dec 29, 2021