[Bug]: disconnecting flag is not reset after Client.end is finished - Error: client disconnecting

TimT1919 commented 3 weeks ago

MQTTjs Version

5.7.3

Broker

mosquitto

Environment

NodeJS

Description

While calling the publish method on a connected client the Error: client disconnecting is thrown by the lib. The reason for this is that the Client.disconnecting flag is not reset to false after the Client.end method is finished.

Workaround: calling the reconnect method instead of the connect method, cause the Client.disconnecting flag is reset to false there

Minimal Reproduction

create a Client with mqtt.connect method using the option manualConnect
call the Client method connect
after connect event is triggered, call the Client end method
after close event is triggered, call the Client connect method
after connect event is triggered, call the Client publish method
-> Error: client disconnecting is thrown

Debug logs

Error: client disconnecting at MqttClient._checkDisconnecting (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/client.js:764:36) at MqttClient.publish (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/client.js:330:18) at MqttClient. (/home/scBot/kaeser-utils/ipc/test.js:29:20) at Object.onceWrapper (node:events:628:26) at MqttClient.emit (node:events:525:35) at MqttClient._onConnect (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/client.js:1078:18) at handleConnack (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/handlers/connack.js:39:29) at handle (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/handlers/index.js:44:35) at work (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/client.js:221:40) at Writable.writable._write (/home/scBot/kaeser-utils/node_modules/mqtt/build/lib/client.js:246:13)

robertsLando commented 3 weeks ago

@TimT1919 Seems an easy fix, want to submit a PR? Also remember to add a test that covers this.

Useful docs for tests: https://github.com/mqttjs/MQTT.js/blob/main/DEVELOPMENT.md

TimT1919 commented 2 weeks ago

@robertsLando I was trying to create a PR. Forked the current main branch, downloaded to my device, installed dependencies, run build script and tryed to run the current unit tests without changes. Running the tests throws the following error:

npm test

> mqtt@5.8.0 test
> npm run test:node

> mqtt@5.8.0 test:node
> npm run unit-test:node && codecov

> mqtt@5.8.0 unit-test:node
> node_modules/.bin/nyc node -r esbuild-register test/runTests.ts

node:internal/modules/cjs/loader:867
      throw new ERR_UNKNOWN_BUILTIN_MODULE(request);
      ^

Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:test/reporters
    at __node_internal_captureLargerStackTrace (node:internal/errors:478:5)
    at new NodeError (node:internal/errors:387:5)
    at Function.Module._load (node:internal/modules/cjs/loader:867:13)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18)
    at Object.<anonymous> (/home/scBot/MQTT.js/test/runTests.ts:4:30)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Module.replacementCompile (/home/scBot/MQTT.js/node_modules/append-transform/index.js:60:13)
    at Module._compile (/home/scBot/MQTT.js/node_modules/esbuild-register/dist/node.js:2258:26)
    at module.exports (/home/scBot/MQTT.js/node_modules/default-require-extensions/js.js:7:9)
    at /home/scBot/MQTT.js/node_modules/append-transform/index.js:64:4
    at newLoader (/home/scBot/MQTT.js/node_modules/esbuild-register/dist/node.js:2262:9)
    at Object.<anonymous> (/home/scBot/MQTT.js/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:22:47 {
  code: 'ERR_UNKNOWN_BUILTIN_MODULE'
}
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |                   
----------|---------|----------|---------|---------|-------------------

Do you have any idea what i did wrong?

robertsLando commented 1 week ago

@TimT1919 NodeJS version? It should be at least 18.x+

TimT1919 commented 1 week ago

@robertsLando tried with version v16.20.2. I will try to run the unit tests with v18.20.3. The requirement for 18.x+ is only for development and testing or?

TimT1919 commented 1 week ago

I can run the test npm test with v18.20.3, but i get a lot of errors from the tests. Error: test did not finish before its parent and was cancelled Did not change the source/tests until now.

robertsLando commented 1 week ago

The requirement for 18.x+ is only for development and testing or?

Yes.

I can run the test npm test with v18.20.3, but i get a lot of errors from the tests. Error: test did not finish before its parent and was cancelled

I still didn't had time to find the reason why this happens but seems tests are flacky and that error randomly pops up, try to run them multiple times and they will work

MaximoLiberata commented 1 week ago

Hi guys, I got the same error. In my case the error will throw in any situation when disconnect and connect method are calling manually, not only when the option manualConnect is setting as true.

Could I help to resolve this bug? I wrote the fix and the test in this commit

robertsLando commented 1 week ago

@MaximoLiberata Could you open a PR with that fix please?

TimT1919 commented 1 week ago

@MaximoLiberata your change would solve the issue exception issue, but not that after end method finished the public disconnecting flag is still true.

it(
  'disconnecting should be reset after endAsync',
  {
  timeout: 5000,
  },
  async function _test(t) {
    client = await mqtt.connectAsync(config)

    assert.isFalse(client.disconnecting);

    const endPromise =  client.endAsync();

    assert.isTrue(client.disconnecting);

    await endPromise;

    assert.isFalse(client.disconnecting)
  },
  )

My issue is that i am not able to execute the tests without errors.

MaximoLiberata commented 1 week ago

@TimT1919 the tests are going to fail some times, I don't know the reason why. I ran the tests multiple times until finish all of them successfully.

robertsLando commented 1 week ago

I tried to fix them countless times without success, whoever solves that issue will have my respect 😆

MaximoLiberata commented 1 week ago

I tried to fix them countless times without success, whoever solves that issue will have my respect 😆

In my case the tests that I got problems were:

Describe Test	Test into describe	Bug
auto reconnect	should always cleanup successfully on reconnection	Hangs out
	check emit error on checkDisconnection w/o callback	It's not contained into a describe method
async methods	publish should throw error	It's never shown in the terminal, but it's executed
in-memory store		Hangs out after execute it

A possible general bug for all of these tests can be the way to execute them, I'm talking about the async promise resolution and close the server and client after each unit test.

robertsLando commented 1 week ago

I tried to execute the single problematic tests multiuple times but I wasn't able to make them fail.

Some tests share the same connection and it could be that some is leaking connection open and that causes issues with tests that follow. If you have some possible fix in mind feel free to give it a try and open a PR, this is something I definetely want to fix 🙏🏼

mqttjs / MQTT.js