From accd78e38aa82c8cc1ea04029e56494276776c87 Mon Sep 17 00:00:00 2001 From: Bert Kleewein Date: Mon, 14 Mar 2022 15:53:53 -0700 Subject: [PATCH] fix: fix regression from #1401 and allow CI test failures to break gitthub workflow (#1443) * fix: fix regression from #1401 and allow CI test failures to break github workflow * temporary rollback to verify break works * put change back in * add comment to fix * fix misspelling --- .github/workflows/mqttjs-test.yml | 1 - lib/client.js | 13 ++++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/mqttjs-test.yml b/.github/workflows/mqttjs-test.yml index afb33c76a..b5d18f330 100644 --- a/.github/workflows/mqttjs-test.yml +++ b/.github/workflows/mqttjs-test.yml @@ -30,7 +30,6 @@ jobs: - run: npm ci - name: Node Tests run: npm run test:node - continue-on-error: true env: CI: true DEBUG: "mqttjs" diff --git a/lib/client.js b/lib/client.js index 294cb1149..e90bd2261 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1583,7 +1583,18 @@ MqttClient.prototype._handleAck = function (packet) { const that = this let err - if (!cb || cb === nop) { + // Checking `!cb` happens to work, but it's not technically "correct". + // + // Why? This code assumes that "no callback" is the same as that "we're not + // waiting for responses" (puback, pubrec, pubcomp, suback, or unsuback). + // + // It would be better to check `if (!this.outgoing[messageId])` here, but + // there's no reason to change it and risk (another) regression. + // + // The only reason this code works is becaues code in MqttClient.publish, + // MqttClinet.subscribe, and MqttClient.unsubscribe ensures that we will + // have a callback even if the user doesn't pass one in.) + if (!cb) { debug('_handleAck :: Server sent an ack in error. Ignoring.') // Server sent an ack in error, ignore it. return