Added error handling to imcomingStore.put callback in _handlePublish. #864

redboltz · 2018-08-29T07:28:39Z

Added asynchronous result callback function and error handling in
_handlePubrel similar to _handlePublish.

Added asynchronous result callback function and error handling in _handlePubrel similar to _handlePublish.

redboltz · 2018-08-29T07:31:29Z

Store is a interface.

https://github.com/redboltz/MQTT.js/blob/master/lib/store.js always completes writing when put() is returned. But other Store might not complete writing and notify complete using callback function.

mcollina

This will need unit tests.

mcollina · 2018-08-29T07:30:57Z

lib/client.js

@@ -1082,12 +1085,16 @@ MqttClient.prototype._handlePubrel = function (packet, callback) {
  that.incomingStore.get(packet, function (err, pub) {
    if (!err && pub.cmd !== 'pubrel') {
      that.emit('message', pub.topic, pub.payload, pub)
-      that.incomingStore.put(packet)
-      that.handleMessage(pub, function (err) {
+      that.incomingStore.put(packet, function (err) {


I'd prefer if we called handleMessage before sending a PUBCOMP.

The updated code calls handleMessage before sending a PUBCOMP. The sequence is not changed before/after update.

mcollina · 2018-08-29T07:31:49Z

lib/client.js

-      this.incomingStore.put(packet, function () {
+      this.incomingStore.put(packet, function (err) {
+        if (err) {
+          return done && done(err)


can done be null? If that is true, than can you please assign it to a noop function?

I'm not sure but the existing code https://github.com/mqttjs/MQTT.js/blob/master/lib/client.js#L974 cheking it. So I applied the same manner.

I've checked call sequence.
handlePublish is called only from https://github.com/mqttjs/MQTT.js/blob/master/lib/client.js#L336 in handlePacket. And handlePacket is called only from https://github.com/mqttjs/MQTT.js/blob/master/lib/client.js#L292.
At that point, nextTickWork is passed as the 2nd parameter, callback.

nextTickWork is defined https://github.com/mqttjs/MQTT.js/blob/master/lib/client.js#L283. It cannot be null.

So null checking is not required. I will remove it from my update and existing code.

I removed done && on my environment.
Then I got errors on the following test:
https://github.com/mqttjs/MQTT.js/blob/master/test/abstract_client.js#L953

Can I remove the test?
Or revert as follows?

return done && done(err)

Or

MqttClient.prototype._handlePublish = function (packet, done = function (){}) {

?

I choose

MqttClient.prototype._handlePublish = function (packet, done = function (){}) {

Because

Someone might depend on existing test. So I shouldn't remove it.

You mentioned using empty function is better for empty callback at Added error handling to imcomingStore.put callback in _handlePublish. #864 (comment)

To support ES5 and older, I fixed as follows:

MqttClient.prototype._handlePublish = function (packet, done) { done = typeof done !== 'undefined' ? done : function () {}

There is a handy nop() function https://github.com/mqttjs/MQTT.js/blob/master/lib/client.js#L66.

I didn't notice it. I replaced it.

mcollina · 2018-08-29T07:32:39Z

lib/client.js

-        that._sendPacket(comp, callback)
+        that.handleMessage(pub, function (err) {
+          if (err) {
+            return callback && callback(err)


can you please assign an empty function to callback if it's not present and remove the two checks?

Yes, let me clarify. You mean

that.incomingStore.get(packet, function (err, pub) { if (!err && pub.cmd !== 'pubrel') { that.emit('message', pub.topic, pub.payload, pub) that.incomingStore.put(packet, function (err) { if (err) { return callback(err) } that.handleMessage(pub, function (err) { if (err) { return callback(err) } that._sendPacket(comp, callback) }) }) } else { that._sendPacket(comp, callback) } })

right?

Sorry I pasted wrong code.

Finally I fixed the code as follows:

MqttClient.prototype._handlePubrel = function (packet, callback) { callback = typeof callback !== 'undefined' ? callback : function () {} var mid = packet.messageId var that = this var comp = {cmd: 'pubcomp', messageId: mid} that.incomingStore.get(packet, function (err, pub) { if (!err && pub.cmd !== 'pubrel') { that.emit('message', pub.topic, pub.payload, pub) that.incomingStore.put(packet, function (err) { if (err) { return callback(err) } that.handleMessage(pub, function (err) { if (err) { return callback(err) } that._sendPacket(comp, callback) }) }) } else { that._sendPacket(comp, callback) } }) }

codecov · 2018-08-29T07:37:23Z

Codecov Report

Merging #864 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #864      +/-   ##
=========================================
+ Coverage   94.05%   94.1%   +0.04%     
=========================================
  Files           8       8              
  Lines         757     763       +6     
  Branches      188     190       +2     
=========================================
+ Hits          712     718       +6     
  Misses         45      45

Impacted Files	Coverage Δ
lib/client.js	`97.46% <100%> (+0.03%)`	⬆️

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 0c9cd0d...d7391a9. Read the comment docs.

mcollina · 2018-08-29T07:55:16Z

It’s not clear what you mean. Can you pass in a custom object to add this test?

redboltz · 2018-08-29T11:40:27Z

It’s not clear what you mean. Can you pass in a custom object to add this test?

Ok I will add test that contains cutstom Store.

Added unit test.

Default paramter is introduced since ES6. Replaced the way that supports older ES,

redboltz · 2018-08-29T14:38:01Z

The original code doesn't use Store.put() callback.
https://github.com/mqttjs/MQTT.js/pull/864/files#diff-50cfa59973c04321b5da0c6da0fdf4feL1085

AsyncStore such as mqtt-level-store might take a time to complete put(). Even if so, callback give the trigger to complete.

This test check it.
https://github.com/mqttjs/MQTT.js/pull/864/files#diff-16702ac352d8772fe21f4f9c1de7e900R1038
https://github.com/mqttjs/MQTT.js/pull/864/files#diff-16702ac352d8772fe21f4f9c1de7e900R1054

mcollina · 2018-08-29T17:34:00Z

test/abstract_client.js

+      AsyncStore.prototype.put = function (packet, cb) {
+        setTimeout(function () {
+          cb(new Error('Error'))
+        }, 200)


can you shorten these? A process.nextTick might be enough.

redboltz · 2018-08-29T23:54:32Z

One travis-ci failed. (nodejs6)

https://travis-ci.org/mqttjs/MQTT.js/jobs/422322347#L1785

  1) MqttSecureClient
       offline messages
         should queue qos != 0 messages:
     Uncaught Error: write EPIPE
      at exports._errnoException (util.js:1020:11)
      at WriteWrap.afterWrite (net.js:806:14)

It seems that it is not related the fix.

redboltz · 2018-08-30T00:23:29Z

I've checked all tests has been passed on my travis-ci.
https://travis-ci.org/redboltz/MQTT.js/builds/422322332

redboltz · 2018-08-30T16:21:36Z

Thank you for merging.

Added error handling to imcomingStore.put callback in _handlePublish.

a1af37a

Added asynchronous result callback function and error handling in _handlePubrel similar to _handlePublish.

mcollina reviewed Aug 29, 2018

View reviewed changes

redboltz added 2 commits August 29, 2018 22:36

Refined null callback handling.

aa5647d

Added unit test.

Fixed the way to provide default empty function.

a981d02

Default paramter is introduced since ES6. Replaced the way that supports older ES,

mcollina reviewed Aug 29, 2018

View reviewed changes

Replaced setTimeout() with process.nextTick()

4a7e90b

Replaced function () {} with nop

d7391a9

mcollina merged commit adff840 into mqttjs:master Aug 30, 2018

snyk-bot mentioned this pull request Mar 2, 2020

[Snyk] Upgrade mqtt from 2.16.0 to 2.18.8 terafin/mqtt-relay#3

Closed

Added error handling to imcomingStore.put callback in _handlePublish. #864

Added error handling to imcomingStore.put callback in _handlePublish. #864

Uh oh!

Conversation

redboltz commented Aug 29, 2018

Uh oh!

redboltz commented Aug 29, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina commented Aug 29, 2018

Uh oh!

redboltz commented Aug 29, 2018

Uh oh!

redboltz commented Aug 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redboltz commented Aug 29, 2018

Uh oh!

redboltz commented Aug 30, 2018

Uh oh!

redboltz commented Aug 30, 2018

Uh oh!

Uh oh!

codecov bot commented Aug 29, 2018 •

edited

Loading