Feature mqtt 5 #827

scarry1992 · 2018-05-30T08:12:44Z

Hi. We've implemented the MQTT 5.0 support. Please have a look and merge if it's OK. We will also make similar pull-requests to mqtt-connection and mqtt-packet repos.

mcollina · 2018-06-03T07:30:25Z

Good work, I’ll wait to land this until mqtt-packet and mqtt-connection has been updated and release.

Do you think this is a semver-minor change?

scarry1992 · 2018-06-03T08:18:44Z

HI, thanks for approving my commit. (the main answer in mitt-packet)

scarry1992 · 2018-06-04T07:54:59Z

Hello, can you publish new version of mqtt-connect to npm? And we could update this package as well.

mcollina · 2018-06-08T18:52:20Z

@scarry1992 can you please update this so that tests are passing? Please do not bump the version or merge this. Thanks.

scarry1992 · 2018-06-08T19:03:59Z

@mcollina yes, I can, but I needed in updated version of mqtt-connection. After updating mqtt-connection packet in npm, I will update MQTT.js and all test will be OK. I'm sorry for mqtt-connection merge.

mcollina · 2018-06-08T19:08:51Z

It is now.

codecov · 2018-06-08T19:22:12Z

Codecov Report

Merging #827 into master will increase coverage by 0.26%.
The diff coverage is 96.57%.

@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
+ Coverage   94.13%   94.39%   +0.26%     
==========================================
  Files           8        8              
  Lines         767      874     +107     
  Branches      190      231      +41     
==========================================
+ Hits          722      825     +103     
- Misses         45       49       +4

Impacted Files	Coverage Δ
lib/client.js	`97.27% <96.57%> (-0.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e88644...3fc4120. Read the comment docs.

scarry1992 · 2018-06-08T19:46:21Z

@mcollina Do I need to write more tests?

mcollina · 2018-06-10T16:50:42Z

I think so, yes. Look at the areas that are not covered in codecov.

mcollina · 2018-06-10T16:51:22Z

lib/client.js

+  // auth
+  if (this.options.properties) {
+    if (!this.options.properties.authenticationMethod && this.options.properties.authenticationData) {
+      this.emit('error', new Error('Packet has no Authentication Method'))


we might want to return after this? Emitting 'error' is a distructive action.

I`ll add returning 'this' after emitting error

mcollina · 2018-06-10T16:52:13Z

lib/client.js

+  var options = this.options
+
+  if (options.protocolVersion === 5 && options.properties && options.properties.maximumPacketSize && options.properties.maximumPacketSize < packet.length) {
+    this.emit('error', new Error('exceeding packets size ' + packet.cmd))


if we are emitting error, we should really destroy the client or fully disconnect (and reconnect).

ill add end for connection with reason code, but dont add reconnect, because I get loop, if do this.

Can you please explain it a bit more? I think our goal with this client is to keep operating. If we receive such a bad packet (and we keep receiving it), the client will not be able to operate anymore.

From spec: If a Client receives a packet whose size exceeds this limit, this is a Protocol Error, the Client uses DISCONNECT with Reason Code 0x95 (Packet too large). If you think, that bad idea, I can remove emitting error, but I think thats right. Just tell me what do you think. I think user must proccess that error himself.

mcollina · 2018-06-10T16:52:59Z

lib/client.js

+  //   160: 'Maximum connect time',
+  //   161: 'Subscription Identifiers not supported',
+  //   162: 'Wildcard Subscriptions not supported'
+  // }


why is this block commented here? If it's not needed, remove.

mcollina · 2018-06-10T16:53:49Z

lib/client.js

+  //   162: 'Wildcard Subscriptions not supported'
+  // }
+
+  var args = Array.prototype.slice.call(arguments)


Don't do this. It's super slow in older versions of Node.js (< 6), and not really needed in this case.

I`ll remove it

mcollina · 2018-06-10T16:54:17Z

lib/client.js

+    156: 'Use another server',
+    157: 'Server moved',
+    159: 'Connection rate exceeded'
+  }


this should really go to the top of the file. There is no need to declare this here.

mcollina · 2018-06-10T16:54:49Z

lib/client.js

+      if (pubackRC && pubackRC > 0 && pubackRC !== 16) {
+        err = new Error('Publish error: ' + errors[pubackRC])
+        err.code = pubackRC
+        this.emit('error', err)


we should call the callback with the error, not emitting one.

rebase to call callback

mcollina · 2018-06-10T16:54:58Z

lib/client.js

+      if (pubrecRC && pubrecRC > 0 && pubrecRC !== 16) {
+        err = new Error('Publish error: ' + errors[pubrecRC])
+        err.code = pubrecRC
+        this.emit('error', err)


rebase to call callback

mcollina · 2018-06-10T16:55:39Z

test/server.js

+  var that = this
+  var connection = new Connection(duplex, function () {
+    that.emit('client', connection)
+  })


why this change?

this changed because server`s initialization logic was been changed. Server process packets by version in connect packet

psorowka · 2018-06-20T07:03:00Z

Hi @scarry1992 thanks for the great effort!

I have a question regarding your design choices. I understand that most of the publish property fields (like contentType) are application knowledge and thus must be specified by the outer application when calling publish.

However I believe the topicAlias feature falls into a different category as it basically is a compression feature that requires some special handling for managing and resolving the alias<->topic map. I think this burden should not be put onto the application but it would be convenient if mqttjs would manage the topic alias mapping auto-magically under the hood.

Currently - if I didn't miss anything in your implementation - I understand that the application must give the topicAlias as a parameter to publish and also in the on('message') handler the application must handle cases when topic or topicAlias or both is set and resolve aliases to topics accordingly. Is that correct?

Other question: could you state whether your implementation implements the full set of MQTT5 features or is there anything still to do?

psorowka · 2018-06-20T07:20:36Z

I am also thinking about other convenience features that could reflect some MQTT5 ideas in the client API, e.g.:

automatically set the payloadFormatIndicator flag depending on if the payload parameter is of type String or other
provide additional events that represent specific subscriptions (subscription_id feature) <-- or is that a bad idea from a performance perspective, @mcollina ?

might be more.. but that's just for now.

scarry1992 · 2018-06-27T10:03:00Z

@psorowka I’ve got your point and it does make sense. However, I still think that this logic should be left at the discretion of the MQTT.js user. We can also ask @mcollina opinion and if he believes that this functionality should be implemented, I will do that.

@mcollina , MQTT 5 enables communication between the broker and the client using reason codes, which, in my understanding, means that there should be the opportunity to send puback/pubrel packets manually by a user. Did I get it right? More details here: pubac specs in reason codes section (Implementation specific error).

Please also check my fixes based on the comments in the review.

psorowka · 2018-06-27T19:49:42Z

@scarry1992 what are the arguments towards leaving the alias logic to the user?

My strong opinion is that such kind of communication logic is what a user expects to be handled by a well tested client library instead of heaving to implement that manually in each application.

psorowka · 2018-06-27T20:01:24Z

@scarry1992 as for the PUBACK. As far as I understand, that does not mean that a PUBACK packet should be triggered manually, it's still always the direct response to a PUBLISH packet. As the usual way to receive messages is the event listener, I think it's not possible to set a response from the callback. The only way I can think of would be to offer the ability to add an "interceptor" function that's called before the message event is triggered and may set a reason code.

On the other hand I am really wondering if it's really the intention of the spec to have the client set other reason codes than 0x00 or 0x80.. what should the broker do with the responses...

mcollina · 2018-07-06T22:43:44Z

I'm sorry for the long delay in reviewing this. I've been traveling for most of the last month, with more traveling next month.

@scarry1992 Good work!

MQTT 5 enables communication between the broker and the client using reason codes, which, in my understanding, means that there should be the opportunity to send puback/pubrel packets manually by a user. Did I get it right? More details here: pubac specs in reason codes section (Implementation specific error).

I agree with you.

@psorowka I would like to keep this implementation minimal, so we can cut a new major and then improve on that one. We can even land it and start cutting -rc releases, and iron bugs along the way.

@scarry1992 @psorowka can you please point me to some docs about topic alias?

scarry1992 · 2018-07-07T05:40:44Z

http://docs.oasis-open.org/mqtt/mqtt/v5.0/cs02/mqtt-v5.0-cs02.html#_Toc514345323 - topicAliasMaximum, http://docs.oasis-open.org/mqtt/mqtt/v5.0/cs02/mqtt-v5.0-cs02.html#_Toc514345385 - topicAlias

scarry1992 · 2018-07-07T05:47:59Z

I believe that it is necessary to leave the topicAlias property on the user side because, at the moment, this makes the use of this flag not transparent, and can lead to incorrect behavior with different brokers. Most brokers currently do not support or support partially 5 standard and there may not be implemented this feature, and there is no need to completely stop working with this broker. I myself like the idea of bringing this logic to the library, but so far, it seems to me, you can not do it.

psorowka · 2018-07-07T06:36:36Z

Vernemq is already almost completely supporting everyithing of mqtt5, just for testing.

IMHO topic aliases are one of the coolest features as it reduces the package size significantly.

When releasing this to major we must be very careful that topicaliases are disabled by default (topicAliasMaximum 0) because otherwise a user might receive onMessage events with empty topic strings unexpectedly

scarry1992 · 2018-07-13T15:58:40Z

Hi. I did custom handling reason codes feature.

scarry1992 · 2018-07-17T11:29:10Z

@mcollina Do I need to implement something else?

mcollina · 2018-07-06T22:41:15Z

lib/client.js

+  var options = this.options
+
+  if (options.protocolVersion === 5 && options.properties && options.properties.maximumPacketSize && options.properties.maximumPacketSize < packet.length) {
+    this.emit('error', new Error('exceeding packets size ' + packet.cmd))


Can you please explain it a bit more? I think our goal with this client is to keep operating. If we receive such a bad packet (and we keep receiving it), the client will not be able to operate anymore.

mcollina · 2018-07-19T13:28:39Z

lib/client.js

@@ -93,6 +138,8 @@ function MqttClient (streamBuilder, options) {

  this.options.clientId = (typeof this.options.clientId === 'string') ? this.options.clientId : defaultId()

+  this.options.customHandleAcks = this.options.protocolVersion === 5 ? this.options.customHandleAcks : false


it seems we are manipulating this.options a lot in this function. Would you mind extracting it into a variable var options = this.options?

mcollina · 2018-07-19T13:29:27Z

lib/client.js

+        (!this.options.properties.topicAliasMaximum && opts.properties.topicAlias)))) {
+      delete packet.properties.topicAlias
+    }
+  }


Can you please add a comment explaining this block?
Also, can you extract var options = this.options for readability?

mcollina · 2018-07-19T13:29:50Z

lib/client.js

@@ -427,7 +503,7 @@ MqttClient.prototype.publish = function (topic, message, opts, callback) {
 * @api public
 * @example client.subscribe('topic');
 * @example client.subscribe('topic', {qos: 1});
- * @example client.subscribe({'topic': 0, 'topic2': 1}, console.log);
+ * @example client.subscribe({'topic': {qos: 0}, 'topic2': {qos: 1}}, console.log);


Is this previous form now removed?

mcollina · 2018-07-19T13:31:45Z

lib/client.js

+          that._resubscribeTopics[sub.topic].nl = sub.nl || false
+          that._resubscribeTopics[sub.topic].rap = sub.rap || false
+          that._resubscribeTopics[sub.topic].rh = sub.rh || 0
+        }


we are manipulating a lot _resubscribeTopics. Can you please extract it into a separate variable?

Can you also add the docs for nl , rap and rh? If these are not exposed, can you please add a comment somewhere explaining them?

fixed, added info in readme

mcollina · 2018-07-19T13:32:50Z

lib/client.js

  var packet = {
    cmd: 'unsubscribe',
    qos: 1,
    messageId: this._nextId()
  }
  var that = this
+  var args = Array.prototype.slice.call(arguments)


This is better implemented with:

var args = new Array(arguments.length) for (var i = 0; i < arguments.length; i++) { args[i] = arguments[i] }

because using Array.prototype.slice is extremely unperformant in older Node.js versions.

implemented

mcollina · 2018-07-19T13:33:19Z

lib/client.js

+      if (!this.options.properties) { this.options.properties = {} }
+      this.options.properties.maximumPacketSize = packet.properties.maximumPacketSize
+    }
+  }


Can you extract var options = this.options?

mcollina · 2018-07-19T13:34:52Z

lib/client.js

      break
+    }


All this function becomes very complicated with the customHandleHack function. Can you please refactor so that there is always a default for that, and we avoid all those ifs?

refactored, check pls

mcollina · 2018-07-19T13:35:56Z

lib/client.js

-        that._sendPacket({cmd: 'puback', messageId: mid}, done)
-      })
+      if (this.options.customHandleAcks) {
+        this.options.customHandleAcks(topic, message, packet, function (code) {


the signature for the callback of customHandleAcks must be function (err, code) {}. We should also validate that code is valid, or is this done when generating a PUBACK`?

code validation added

scarry1992 · 2018-07-19T16:57:46Z

I fixed all issues and give answers. Can You check it and release new version?

scarry1992 · 2018-08-28T09:19:42Z

@mcollina, After update my branch, I get error in CI in nodes 4,6,9 "The command "sudo -E apt-add-repository -y "ppa:ubuntu-toolchain-r/test"" failed and exited with 1 during ."

mcollina · 2018-08-28T09:21:56Z

Travis is a bit flaky these days

scarry1992 · 2018-08-28T09:23:19Z

@mcollina, so when can we realese mqtt 5.0?

scarry1992 · 2018-08-28T09:24:47Z

In 4.3.2, 7, 8 nodes tests passed.

mcollina · 2018-08-29T07:17:58Z

Do you know which brokers are currently compatible with this? I would like to do some manual testing.

My goto broker for this type of things is Mosquitto, and it's not supporting MQTT v5, but it should be ready sooner rather than later https://mosquitto.org/blog/2018/05/press-release/.

Regarding the CI, could you: a) drop testing on Node 4, 4.3.2 (lambda), 5, 7, and 9? Also, add Node 10.

psorowka · 2018-08-29T07:20:35Z

@mcollina VerneMQ has a MQTTv5 preview release see https://vernemq.com/mqttv5.html

scarry1992 · 2018-08-29T07:23:56Z

@mcollina, I tested on flespi broker with mqtt 5.0 and that branch in production in my projects allredy.

scarry1992 · 2018-08-29T07:25:12Z

@psorowka, can you testing that branch on VerneMQ youself please?

mcollina · 2018-08-29T07:29:04Z

Could you add a note in the README that MQTT v5 support is experimental as it has not been implemented by brokers yet?

scarry1992 · 2018-08-29T07:30:39Z

@mcollina yes sure

mcollina · 2018-09-07T13:50:25Z

@scarry1992 can you please rebase your changes or merge master? It's conflicting now.

scarry1992 · 2018-09-07T16:55:00Z

@mcollina done)

scarry1992 added 3 commits May 28, 2018 10:59

mqtt 5 init

6b8dda5

fixes, tests, docs

b945a04

docs

e208264

update packages

a71d99a

mcollina reviewed Jun 10, 2018

View reviewed changes

scarry1992 added 2 commits June 12, 2018 18:20

fixes by review, merged new version

c1908ad

tests

60fa3f7

scarry1992 added 5 commits July 13, 2018 11:50

Merge remote-tracking branch 'remotes/origin/master' into feature-mqtt-5

47ba458

merge with master, custom reasonCodes

145aee1

merge with master, custom reasonCodes

38052c0

revert update packages

0dc22a4

revert test

d3e7b86

extend test for just check connection

f24f503

mcollina reviewed Jul 19, 2018

View reviewed changes

fixes by review

ac83cef

scarry1992 added 2 commits August 28, 2018 12:13

updated version

1aac657

Merge branch 'master' into feature-mqtt-5

413a761

Notice about MQTT 5.0 added to README

73450f1

scarry1992 added 2 commits September 7, 2018 19:37

update branch

bbd30a8

Merge branch 'master' into feature-mqtt-5

3fc4120

mcollina merged commit fe5703e into mqttjs:master Sep 7, 2018

sclausen mentioned this pull request Nov 24, 2018

will.properties.willDelayInterval property for IMqttServiceOptions sclausen/ngx-mqtt#121

Closed

5 tasks

DotChris mentioned this pull request Oct 4, 2019

3.0 not support client.subscribe({view_server: 1}) #965

Closed

		@@ -93,6 +138,8 @@ function MqttClient (streamBuilder, options) {

		this.options.clientId = (typeof this.options.clientId === 'string') ? this.options.clientId : defaultId()

		this.options.customHandleAcks = this.options.protocolVersion === 5 ? this.options.customHandleAcks : false

Feature mqtt 5 #827

Feature mqtt 5 #827

Conversation

scarry1992 commented May 30, 2018

mcollina commented Jun 3, 2018

scarry1992 commented Jun 3, 2018

scarry1992 commented Jun 4, 2018

mcollina commented Jun 8, 2018

scarry1992 commented Jun 8, 2018

mcollina commented Jun 8, 2018

codecov bot commented Jun 8, 2018 • edited

Codecov Report

scarry1992 commented Jun 8, 2018

mcollina commented Jun 10, 2018

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

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

scarry1992 Jun 12, 2018 • edited

Choose a reason for hiding this comment

psorowka commented Jun 20, 2018 • edited

psorowka commented Jun 20, 2018

scarry1992 commented Jun 27, 2018 • edited

psorowka commented Jun 27, 2018

psorowka commented Jun 27, 2018

mcollina commented Jul 6, 2018

scarry1992 commented Jul 7, 2018

scarry1992 commented Jul 7, 2018

psorowka commented Jul 7, 2018

scarry1992 commented Jul 13, 2018 • edited

scarry1992 commented Jul 17, 2018

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

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

scarry1992 commented Jul 19, 2018

scarry1992 commented Aug 28, 2018 • edited

mcollina commented Aug 28, 2018

scarry1992 commented Aug 28, 2018 • edited

scarry1992 commented Aug 28, 2018

mcollina commented Aug 29, 2018

psorowka commented Aug 29, 2018

scarry1992 commented Aug 29, 2018

scarry1992 commented Aug 29, 2018

mcollina commented Aug 29, 2018

scarry1992 commented Aug 29, 2018

mcollina commented Sep 7, 2018

scarry1992 commented Sep 7, 2018

codecov bot commented Jun 8, 2018 •

edited

scarry1992 Jun 12, 2018 •

edited

psorowka commented Jun 20, 2018 •

edited

scarry1992 commented Jun 27, 2018 •

edited

scarry1992 commented Jul 13, 2018 •

edited

scarry1992 commented Aug 28, 2018 •

edited

scarry1992 commented Aug 28, 2018 •

edited