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

Improving idea of Topic Alias #1300

Closed
redboltz opened this issue Jul 12, 2021 · 4 comments
Closed

Improving idea of Topic Alias #1300

redboltz opened this issue Jul 12, 2021 · 4 comments
Assignees

Comments

@redboltz
Copy link
Contributor

Overview

I found some issue about TopicAlias. It is pretty complecated so I post detailed report here.
I will create a pull request to fix the issue and some of improvements for Topic Alias.

Two independent topic alias maximum

There are two independent TopicAliasMaximum. One is client to broker, the other is broker to client.

client to broker

It is notified by CONNACK packet.

See https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901088
This limits how many topic aliases can be used by the client. The value is decided by the broker.

broker to client

It is notified by CONNECT packet.

See https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901051
This limits how many topic aliases to the client can be used by the broker. The value is decided by the client.
So MQTT.js client option topicAliasMaximum should be this one.

Important note

The two Topic Alias Maximum is not negotiated. They are independent values.
TopicAlias mapping is also independent.
So client to broker topic1-TopicAlias:1 and broker to client topic2-TopicAlias:1 can exist at the same time.

client                           broker
TopicAliasMaximum=10             TopicAliasMaximum=5
(options.topicAliasMaximum=10)
   |                                |
   |                                |
   | CONNECT(TopicAliasMaximum:10)  |
   |------------------------------->|
   |                                |
   |                                |
   | CONNACK(TopicAliasMaximum:5)   |
   |<-------------------------------|
   |                                |
   |                                |
The client can use               The broker can use
1-5 topic aliases.               1-10 topic aliases.
   |                                |
   |                                |
   |  PUBLISH(topic:t1, ta:1)       |   (ta means TopicAlias)
   |------------------------------->|
   |                                |
   |                             The broker needs to manage
   |                             topic:t1 - ta:1 mapping
   |                                |
   |                                |
   |  PUBLISH(topic:t2, ta:1)       |
   |<-------------------------------|
   |                                |
The client needs to manage
topic:t2 - ta:1 mapping

Topic Alias lifetime

The spec said as follows:

https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901113

Topic Alias mappings exist only within a Network Connection and last only for the lifetime of that Network Connection. A receiver MUST NOT carry forward any Topic Alias mappings from one Network Connection to another [MQTT-3.3.2-7].

So the lifetime of mapping is only in the network connection not session.
See https://markmail.org/message/3j47bqjdvcaxdcji

Impact for implementation

Consider the following scenario.

  1. The client sends CONNECT to the broker.
  2. The broker sends CONNACK(Topic Alias Maximum:10) to the client.
  3. The client sends PUBLISH(topic:'t1', ta:1, qos:0) to the broker.
  4. The client sends PUBLISH(topic:'', ta:1, qos:1) to the broker.
  5. Network disconnected.
    The PUBLISH packet in the step4 is stored in the client's outgoingStore.
  6. The client reconnects to the broker. (sends CONNECT)
  7. The broker sends CONNACK(Topic Alias Maximum:10) to the client.
  8. The client re-sends PUBLISH(topic:'', ta:1, qos:1) to the broker.
    It is protocol error.

The spec is a little ambiguous. It only referred to "A receiver MUST NOT carry forward", but sender and receiver is a pair. So the sender must not send topic alias just after reconnection. Consider TopicAliasMaximum at the step7 is different from the step2 case.

I guess that #1213 is caused by this problem.

Here is the solution.
Add the TopicAlias map to the sender (in this case the client).
At the step3, register the topic-alias pair to the map.
At the step4, get the topic from the alias using the map and create the packet clone for storing.
The storing packet has the recovered topic, and remove TopicAlias from the storing packet.
Then send the original packet.
At the step8, resending packet is the stored packet. So it has topic and doesn't have TopicAlias. No protocol error.

The current MQTT.js haven't implemented the topic recover storing.

In addition, the following TopicAliasMaximum handling is not enough.

MQTT.js/lib/client.js

Lines 527 to 540 in d8be59e

if (options.protocolVersion === 5) {
packet.properties = opts.properties
if ((!options.properties && packet.properties && packet.properties.topicAlias) || ((opts.properties && options.properties) &&
((opts.properties.topicAlias && options.properties.topicAliasMaximum && opts.properties.topicAlias > options.properties.topicAliasMaximum) ||
(!options.properties.topicAliasMaximum && opts.properties.topicAlias)))) {
/*
if we are don`t setup topic alias or
topic alias maximum less than topic alias or
server don`t give topic alias maximum,
we are removing topic alias from packet
*/
delete packet.properties.topicAlias
}
}

And corresponding test:
var topicAliasTests = [
{properties: {}, name: 'should allow any topicAlias when no topicAliasMaximum provided in settings'},
{properties: { topicAliasMaximum: 15 }, name: 'should not allow topicAlias > topicAliasMaximum when topicAliasMaximum provided in settings'}
]
topicAliasTests.forEach(function (test) {
it(test.name, function (done) {
this.timeout(15000)
server.once('client', function (serverClient) {
serverClient.on('publish', function (packet) {
if (packet.properties && packet.properties.topicAlias) {
done(new Error('Packet should not have topicAlias'))
return false
} else {
serverClient.end(done)
}
})
})
var opts = {host: 'localhost', port: ports.PORTAND115, protocolVersion: 5, properties: test.properties}
var client = mqtt.connect(opts)
client.publish('t/h', 'Message', { properties: { topicAlias: 22 } })
})
})

The test case should allow any topicAlias when no topicAliasMaximum provided in settings is not valid.
See https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901088

If Topic Alias Maximum is absent or 0, the Client MUST NOT send any Topic Aliases on to the Server [MQTT-3.2.2-18].

Server(broker) to client is the same rule.

Convenient new feature

Now, the client need to manage TopicAlias sending map. It is to recover topic for storing. The map can be used for automatic topic alias applying and automatic topic alias allocation.

Automatic Topic Alias using

If the client sets the option autoUseTopicAlias:true then MQTT.js uses existing topic alias automatically.

example scenario:

1. PUBLISH topic:'t1', ta:1                   (register)
2. PUBLISH topic:'t1'       -> topic:'', ta:1 (auto use existing map entry)
3. PUBLISH topic:'t2', ta:1                   (register overwrite)
4. PUBLISH topic:'t2'       -> topic:'', ta:1 (auto use existing map entry based on the receent map)
5. PUBLISH topic:'t1'                         (t1 is no longer mapped to ta:1)

User doesn't need to manage which topic is mapped to which topic alias.
If the user want to register topic alias, then publish topic with topic alias.
If the user want to use topic alias, then publish topic without topic alias. If there is a mapped topic alias then added it as a property and update the topic to empty string.

Automatic Topic Alias assign

If the client sets the option autoAssignTopicAlias:true then MQTT.js uses existing topic alias automatically.
If no topic alias exists, then assign a new vacant topic alias automatically. If topic alias is fully used, then LRU(Least Recently Used) topic-alias entry is overwritten.

example scenario:

The broker returns CONNACK (TopicAliasMaximum:3)
1. PUBLISH topic:'t1' -> 't1', ta:1 (auto assign t1:1 and register)
2. PUBLISH topic:'t1' -> ''  , ta:1 (auto use existing map entry)
3. PUBLISH topic:'t2' -> 't2', ta:2 (auto assign t1:2 and register. 2 was vacant)
4. PUBLISH topic:'t3' -> 't3', ta:3 (auto assign t1:3 and register. 3 was vacant)
5. PUBLISH topic:'t4' -> 't4', ta:1 (LRU entry is overwritten)

Also user can manually register topic-alias pair using PUBLISH topic:'some', ta:X. It works well with automatic topic alias allocation

redboltz added a commit to redboltz/MQTT.js that referenced this issue Jul 12, 2021
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.
redboltz added a commit to redboltz/MQTT.js that referenced this issue Jul 15, 2021
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
Copy link
Contributor

YoDaMa commented Sep 27, 2021

This is really good @redboltz, sorry I haven't seen this earlier. Going to incorporate this into the vNext work I'm doing.

@YoDaMa YoDaMa self-assigned this Sep 27, 2021
@YoDaMa
Copy link
Contributor

YoDaMa commented Sep 27, 2021

also, did you create a pr associated with this change already?

@redboltz
Copy link
Contributor Author

@YoDaMa Thank you for the response! #1301 is the PR to implement this issue.
The target branch is master. Now all CI tests are passed, no conflict.

YoDaMa pushed a commit that referenced this issue Oct 4, 2021
* Refined Topic Alias support. (Implement #1300)

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.

* Fix typo.

* Fix comment.

* Rename the function name from `removeTopicAlias` to `removeTopicAliasAndRecoverTopicName`.

Add comments for caller side of `removeTopicAliasAndRecoverTopicName`.

* Captalize label.
@YoDaMa
Copy link
Contributor

YoDaMa commented Oct 19, 2021

This has been merged in. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants