Repository: moscajs/aedes
Pull request #517 (2020-07-06 09:13:31)

fix: don't pass writeQos errors in packet cb
deliverQos/deliver0 functions are bounded in subscribe and used then by mqemitters to deliver messages to clients, the callbacks of those functions should NEVER return an error in callback as it is not catched by mqemitter and could cause unhandled exceptions

coveralls: ## Pull Request Test Coverage Report for [Build 159108069](https://coveralls.io/builds/31877892)

* **-1** of **5**   **(80.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at ?**%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [lib/client.js](https://coveralls.io/builds/31877892/source?filename=lib%2Fclient.js#L182) | 4 | 5 | 80.0%
<!-- | **Total:** | **4** | **5** | **80.0%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/31877892/badge)](https://coveralls.io/builds/31877892) |
| :-- | --: |
| Change from base [Build 147498646](https://coveralls.io/builds/31685110): |  0% |
| Covered Lines: |  |
| Relevant Lines: | 0 |

---
##### 💛  - [Coveralls](https://coveralls.io)

robertsLando: @mcollina ping

Patch:
diff --git a/lib/client.js b/lib/client.js
index f0be5a2..93646c0 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -107,7 +107,7 @@ function Client (broker, conn, req) {
         packet.qos = 0
         write(that, packet, function (err) {
           that._onError(err)
-          cb() // don't pass the error here or it will be thrown be mqemitter
+          cb() // don't pass the error here or it will be thrown by mqemitter
         })
       })
     } else {
@@ -174,9 +174,16 @@ function writeQoS (err, client, packet) {
   if (err) {
     // is this right, or we should ignore thins?
     client.emit('error', err)
-    packet.writeCallback(err)
+    // don't pass the error here or it will be thrown by mqemitter
+    packet.writeCallback()
   } else {
-    write(client, packet, packet.writeCallback)
+    write(client, packet, function (err) {
+      if (err) {
+        client.emit('error', err)
+      }
+      // don't pass the error here or it will be thrown by mqemitter
+      packet.writeCallback()
+    })
   }
 }
 


Test patch:
diff --git a/test/client-pub-sub.js b/test/client-pub-sub.js
index a01c21e..6f9c9a8 100644
--- a/test/client-pub-sub.js
+++ b/test/client-pub-sub.js
@@ -451,6 +451,40 @@ test('subscribe a client programmatically', function (t) {
   })
 })
 
+test('subscribe throws error when QoS > 0', function (t) {
+  t.plan(3)
+
+  const broker = aedes()
+  t.tearDown(broker.close.bind(broker))
+
+  broker.on('clientReady', function (client) {
+    client.subscribe({
+      topic: 'hello',
+      qos: 1
+    }, function (err) {
+      t.error(err, 'no error')
+
+      // makes writeQos throw error
+      client.connected = false
+      client.connecting = false
+
+      broker.publish({
+        topic: 'hello',
+        payload: Buffer.from('world'),
+        qos: 1
+      }, function (err) {
+        t.error(err, 'no error')
+      })
+    })
+  })
+
+  broker.on('clientError', function (client, error) {
+    t.equal(error.message, 'connection closed', 'should throw clientError')
+  })
+
+  connect(setup(broker))
+})
+
 test('subscribe a client programmatically - wildcard', function (t) {
   t.plan(3)
 
@@ -902,7 +936,7 @@ test('custom function in broker.unsubscribe', function (t) {
     }
   })
   function deliver (packet, cb) {
-    t.fail('shoudl not be called')
+    t.fail('should not be called')
     cb()
   }
 })