[Bug]: wrong connection string with a port puts mqtt client into an endless loop

[maximivanov]

MQTTjs Version

5.4.0

Broker

Doesn't matter (testing connection to a wrong hostname)

Environment

NodeJS

Description

When trying to connect to a wrong URL that contains a port, the client never throws an error

Minimal Reproduction

Verify versions:

❯ node --version
v20.9.0

❯ npm --version
10.2.5

❯ npm list | grep mqtt
├── mqtt@5.4.0

Add a script:

import * as mqtt from 'mqtt';

// this is a non existent hostname and it's part of the test
const url = 'mqtt://a7dc229b79e0425222318eb495f0dddd.s1.eu.hivemq.cloud:8883';

const mqttClient = await mqtt.connectAsync(url, {
  connectTimeout: 5000,
  username: 'does not matter',
  password: 'does not matter',
});

await mqttClient.endAsync();

Run the script:

DEBUG='mqttjs*' node mqtt-connect-issue.mjs

Expected: process errors after 5 seconds because the hostname is not reachable.

Actual: process never exits.

Note: if you remove the port part (:8883) from the connection string, it will correctly time out and throw after 5 seconds.

Debug logs

❯ DEBUG='mqttjs*' node mqtt-connect-issue.mjs
  mqttjs connecting to an MQTT broker... +0ms
  mqttjs:client MqttClient :: version: undefined +0ms
  mqttjs:client MqttClient :: environment node +0ms
  mqttjs:client MqttClient :: options.protocol mqtt +0ms
  mqttjs:client MqttClient :: options.protocolVersion 4 +0ms
  mqttjs:client MqttClient :: options.username does not matter +0ms
  mqttjs:client MqttClient :: options.keepalive 60 +0ms
  mqttjs:client MqttClient :: options.reconnectPeriod 1000 +0ms
  mqttjs:client MqttClient :: options.rejectUnauthorized undefined +0ms
  mqttjs:client MqttClient :: options.properties.topicAliasMaximum undefined +0ms
  mqttjs:client MqttClient :: clientId mqttjs_a4da9d11 +1ms
  mqttjs:client MqttClient :: setting up stream +0ms
  mqttjs:client connect :: calling method to clear reconnect +1ms
  mqttjs:client _clearReconnect : clearing reconnect timer +0ms
  mqttjs:client connect :: using streamBuilder provided to client to create stream +0ms
  mqttjs calling streambuilder for mqtt +5ms
  mqttjs:tcp port 8883 and host a7dc229b79e0425222318eb495f0dddd.s1.eu.hivemq.cloud +0ms
  mqttjs:client connect :: pipe stream to writable stream +7ms
  mqttjs:client connect: sending packet `connect` +3ms
  mqttjs:client _writePacket :: packet: {
  mqttjs:client   cmd: 'connect',
  mqttjs:client   protocolId: 'MQTT',
  mqttjs:client   protocolVersion: 4,
  mqttjs:client   clean: true,
  mqttjs:client   clientId: 'mqttjs_a4da9d11',
  mqttjs:client   keepalive: 60,
  mqttjs:client   username: 'does not matter',
  mqttjs:client   password: 'does not matter',
  mqttjs:client   properties: undefined
  mqttjs:client } +0ms
  mqttjs:client _writePacket :: emitting `packetsend` +2ms
  mqttjs:client _writePacket :: writing to stream +0ms
  mqttjs:client _writePacket :: writeToStream result true +18ms
  mqttjs:client (mqttjs_a4da9d11)stream :: on close +188ms
  mqttjs:client _flushVolatile :: deleting volatile messages from the queue and setting their callbacks as error function +1ms
  mqttjs:client stream: emit close to MqttClient +0ms
  mqttjs:client close :: connected set to `false` +0ms
  mqttjs:client close :: clearing connackTimer +0ms
  mqttjs:client close :: clearing ping timer +0ms
  mqttjs:client close :: calling _setupReconnect +0ms
  mqttjs:client _setupReconnect :: emit `offline` state +0ms
  mqttjs:client _setupReconnect :: set `reconnecting` to `true` +0ms
  mqttjs:client _setupReconnect :: setting reconnectTimer for 1000 ms +0ms
  mqttjs:client reconnectTimer :: reconnect triggered! +1s
  mqttjs:client _reconnect: emitting reconnect to client +0ms
  mqttjs:client _reconnect: calling connect +0ms
  mqttjs:client connect :: calling method to clear reconnect +0ms
  mqttjs:client _clearReconnect : clearing reconnect timer +0ms
  mqttjs:client connect :: using streamBuilder provided to client to create stream +0ms
  mqttjs calling streambuilder for mqtt +1s
  mqttjs:tcp port 8883 and host a7dc229b79e0425222318eb495f0dddd.s1.eu.hivemq.cloud +1s
  mqttjs:client connect :: pipe stream to writable stream +1ms
  mqttjs:client connect: sending packet `connect` +0ms
  ...

[robertsLando]

@maximivanov Just to know, is this a regression or was it failing also with older versions?

[maximivanov]

@robertsLando I never tried connecting to a hostname with port while on a previous version so not sure if the issue exists in versions before 5.4.0.

However there was a similar problem that was fixed somewhere in between 5.0.3 and 5.4.0:

Trying to connect to a mqtts://wrong-host-without-port timed out correctly.
Trying to connect to a mqtt://wrong-host-without-port hanged trying to reconnect indefinitely.
(both time out as expected in 5.4.0).

Hope it helps.

[robertsLando]

Wait but you have set a reconnect period of 1 second and I don't see any error here as it exactly retries after 1 second that the connection is closed:

mqttjs:client reconnectTimer :: reconnect triggered! +1s
  mqttjs:client _reconnect: emitting reconnect to client +0ms

So this is correct. You should pass allowRetries to false (it's the third argument of connectAsync function) if you want to skip reconnect and handle it manually

[maximivanov]

@robertsLando thanks for getting back with the recommendation. I'm a bit confused though - why does it consider it a reconnect if it never connected in the first place (remember the host in the example above is made up and is wrong). Why do I see a connect timeout if I remove the port from that exact connection string?

I would like to have the reconnect functionality (the app is a long running process) but only once we know the initial connection was successful - is there a way to do this?

[robertsLando]

I would like to have the reconnect functionality (the app is a long running process) but only once we know the initial connection was successful - is there a way to do this?

This is not possible and changing the bahaviour of this would be breaking: what if for any reason your broker is down or your internet connection is not working well on client on first try? It would just give up thinking that the broker is unreachable but that's wrong. The alternative is to put this under a configuration option but I really don't think it's so much useful

I think your best choice, if you really want this, is to try pinging the server to see if it's up or not before connecting to MQTT broker

[maximivanov]

@robertsLando does having a configuration option to specify the maximum number of reconnect attempts make sense? My main concern is right now the only options are:

• await mqtt.connectAsync(...) blocks if the host/port is incorrect and there's no way to terminate it (default behavior)
• Opting out of retries completely is not ideal either

If there was a way to make it throw after N unsuccessful connection attempts that would help address the issue:

try {
  const mqttClient = await mqtt.connectAsync(url, {
    reconnectPeriod: 1000,
    maxReconnectAttempts: 10,
  });
} catch (error) {
  // ... 11 seconds later
  // error is "Reached max number of reconnect attempts (10)"
}

[robertsLando]

Yeah it could make sense, feel free to submit a PR and remember to add a test for that

[maximivanov]

Having trouble with the workarounds suggested above.

1. Doing a connection test prior to calling mqtt.connectAsync(). Socket connection to the given host/port succeeds (no error, no timeout) so it cannot be used as an indication.

2. "You should pass allowRetries to false" - I couldn't find anything like allowRetries in the documentation or TS types.

3. Setting reconnectPeriod: 0 in the connectAsync options. This is a weird one, looks like the client does not retry indeed, however does it just exit from the process? Proof:

const mqtt = require('mqtt');

// this is a non existent hostname and it's part of the test
const url = 'mqtt://a7dc229b79e0425222318eb495f0dddd.s1.eu.hivemq.cloud:8883';

async function main() {
  console.log(new Date());
  await new Promise((resolve) => { setTimeout(resolve, 1000); });
  console.log(new Date());
  
  const mqttClient = await mqtt.connectAsync(url, {
    connectTimeout: 5000,
    username: 'does not matter',
    password: 'does not matter',
    reconnectPeriod: 0,
  });
  
  console.log(new Date());
  await new Promise((resolve) => { setTimeout(resolve, 1000); });
  console.log(new Date());  

  await mqttClient.endAsync();
}

main();

Run the script:

~/src/local/mqtt-wrong-host-infinite-connection-loop                                                                                                                                         18:48:47
❯ DEBUG='mqttjs*' node main.js

2024-04-10T17:49:04.834Z
2024-04-10T17:49:05.842Z
  mqttjs connecting to an MQTT broker... +0ms
  mqttjs:client MqttClient :: version: undefined +0ms
  mqttjs:client MqttClient :: environment node +0ms
  mqttjs:client MqttClient :: options.protocol mqtt +1ms
  mqttjs:client MqttClient :: options.protocolVersion 4 +0ms
  mqttjs:client MqttClient :: options.username does not matter +0ms
  mqttjs:client MqttClient :: options.keepalive 60 +0ms
  mqttjs:client MqttClient :: options.reconnectPeriod 0 +0ms
  mqttjs:client MqttClient :: options.rejectUnauthorized undefined +0ms
  mqttjs:client MqttClient :: options.properties.topicAliasMaximum undefined +0ms
  mqttjs:client MqttClient :: clientId mqttjs_12961e0b +0ms
  mqttjs:client MqttClient :: setting up stream +0ms
  mqttjs:client connect :: calling method to clear reconnect +1ms
  mqttjs:client _clearReconnect : clearing reconnect timer +0ms
  mqttjs:client connect :: using streamBuilder provided to client to create stream +0ms
  mqttjs calling streambuilder for mqtt +4ms
  mqttjs:tcp port 8883 and host a7dc229b79e0425222318eb495f0dddd.s1.eu.hivemq.cloud +0ms
  mqttjs:client connect :: pipe stream to writable stream +8ms
  mqttjs:client connect: sending packet `connect` +1ms
  mqttjs:client _writePacket :: packet: {
  mqttjs:client   cmd: 'connect',
  mqttjs:client   protocolId: 'MQTT',
  mqttjs:client   protocolVersion: 4,
  mqttjs:client   clean: true,
  mqttjs:client   clientId: 'mqttjs_12961e0b',
  mqttjs:client   keepalive: 60,
  mqttjs:client   username: 'does not matter',
  mqttjs:client   password: 'does not matter',
  mqttjs:client   properties: undefined
  mqttjs:client } +0ms
  mqttjs:client _writePacket :: emitting `packetsend` +1ms
  mqttjs:client _writePacket :: writing to stream +0ms
  mqttjs:client _writePacket :: writeToStream result true +19ms
  mqttjs:client (mqttjs_12961e0b)stream :: on close +112ms
  mqttjs:client _flushVolatile :: deleting volatile messages from the queue and setting their callbacks as error function +0ms
  mqttjs:client stream: emit close to MqttClient +0ms
  mqttjs:client close :: connected set to `false` +0ms
  mqttjs:client close :: clearing connackTimer +0ms
  mqttjs:client close :: clearing ping timer +0ms
  mqttjs:client close :: calling _setupReconnect +0ms
  mqttjs:client _setupReconnect :: doing nothing... +0ms

~/src/local/mqtt-wrong-host-infinite-connection-loop                                                                                                                                         18:49:06
❯ echo $?
0

As you can see the second pair of dates is never printed, yet the process exit code is 0.

More generally should this issue be reopened? Having an async function (connectAsync, publishAsync) block indefinitely because of the external conditions (mqtt connection cannot be established, whatever) without a way to terminate it does not sound right.

@robertsLando

[robertsLando]

"You should pass allowRetries to false" - I couldn't find anything like allowRetries in the documentation or TS types.

MQTT.js/src/lib/connect/index.ts

Lines 187 to 191 in fc8fafb

	function connectAsync(
	brokerUrl: string | IClientOptions,
	opts?: IClientOptions,
	allowRetries = true,
	): Promise<MqttClient> {

this is a weird one, looks like the client does not retry indeed, however does it just exit from the process

Did you tried to reproduce the issue using mqtt connect (not async) and handle it your own?

More generally should this issue be reopened? Having an async function (connectAsync, publishAsync) block indefinitely because of the external conditions (mqtt connection cannot be established, whatever) without a way to terminate it does not sound right.

Would you like to submit a PR?

[gervaisj]

@robertsLando Regarding:

You should pass allowRetries to false

It seems the allowRetries parameter is not visible (and thus not usable) due to it being defined in the implementation, and not in a signature.

I believe simply adding a signature would to the trick.

[maximivanov]

In most cases you still want retries to account for transient network errors so allowRetries: false does not sound like a great solution. Being able to provide the number of retries (as opposed to retrying forever) might be a more robust approach.

I looked at the code but unfortunately couldn't figure out how to add the retry count logic within reasonable time and had to move on.

[robertsLando]

@maximivanov Just add an event listener to reconnect and stop when it reaches a specific number of retries?