[Bug]: Errors do not result in an error event

[SystemParadox]

MQTTjs Version

5.8.0

Broker

other

Environment

NodeJS

Description

For many kinds of error all you get is a close event - the error event is not emitted so you have no idea what the problem is.

Adding DEBUG=mqttjs:* shows the following:

mqttjs:client _writePacket :: emitting `packetsend`
mqttjs:client _writePacket :: writing to stream
mqttjs:client _writePacket :: writeToStream result false
mqttjs:client streamErrorHandler :: error Username is required to use password
mqttjs:client noop :: Error: Username is required to use password

I should not have to enable DEBUG to see this, the error event should have been emitted to inform the rest of my application.

I checked the source and for some reason this.stream.on('error') seems to noop anything without an err.code which seems very wrong to me.

Minimal Reproduction

One simple way to reproduce is to try connecting with a password but no username, but it seems to suppress almost everything.

Debug logs

mqttjs:client _writePacket :: emitting `packetsend`
mqttjs:client _writePacket :: writing to stream
mqttjs:client _writePacket :: writeToStream result false
mqttjs:client streamErrorHandler :: error Username is required to use password
mqttjs:client noop :: Error: Username is required to use password

[robertsLando]

This error comes from mqtt-packet stream: https://github.com/mqttjs/mqtt-packet/blob/1f2cf7f98ae036fae97096299d6237ae442ff9d8/writeToStream.js#L204

Also here we are listening to mqtt-packet errors, the problem is that emitting ALL errors causes problems in browsers as explained in the comment inside streamErrorHandler.

What you could do is to subscribe to stream errors manually:

// initialize client

client.stream.on('errror', (err) => {
  // do what you want
})

[SystemParadox]

Hi, thanks for the reply.

This error comes from mqtt-packet stream: https://github.com/mqttjs/mqtt-packet/blob/1f2cf7f98ae036fae97096299d6237ae442ff9d8/writeToStream.js#L204

Yes but that's an internal dependency that I have no interaction with - mqtt should be passing errors up to me where I can see them, either by throwing or by other documented means such as the error event. It is very frustrating when software hides errors like this.

Also here we are listening to mqtt-packet errors, the problem is that emitting ALL errors causes problems in browsers as explained in the comment inside streamErrorHandler.

Yes I saw // emitting errors on browsers seems to create issues. But this also seems very wrong. We should not be hiding errors on browsers either, and there should not be any reason to do so. Exactly what issue does this cause? Surely we can do something to fix that issue rather than resorting to suppressing errors.

Even if there is some strange browser issue then it should be checking whether it's in a browser, not fudging it with err.code like this. And it shouldn't just noop on the browser, it should log it with console.error so people have some idea what's going on. Or better yet, emit an event... like an error event?!?

What you could do is to subscribe to stream errors manually:

// initialize client

client.stream.on('errror', (err) => {
  // do what you want
})

1. I shouldn't be reaching down into internal implementation details like this.
2. This doesn't work with auto reconnect, since the stream is recreated each time. I either have to handle reconnections manually, or override streamBuilder.

[robertsLando]

@SystemParadox I agree with your statements above, would you like to send a PR to fix the above issue?

Last time I checked that seems that emitting errors caused unwanted disconnections so I just preferred to keep the things like this to prevent regressions. Consider that I already improved errors emitted in #1752 #1781 #1798.

What I don't understand is why this isn't picked up by

MQTT.js/src/lib/client.ts

Line 862 in 3e252e7

parser.on('error', this.emit.bind(this, 'error'))

that should emit the error as you want, the source code calls stream.destroy(err) so err should be emitted based on nodejs docs

[SystemParadox]

What I don't understand is why this isn't picked up by

MQTT.js/src/lib/client.ts

Line 862 in 3e252e7

parser.on('error', this.emit.bind(this, 'error'))

that should emit the error as you want, the source code calls stream.destroy(err) so err should be emitted based on nodejs docs

As far as I can work out parser.on('error') only catches errors parsing incoming packets. But there are lots of things in mqtt-packet that call stream.destroy so that's directly on stream and we end up in streamErrorHandler where it gets ignored.

Last time I checked that seems that emitting errors caused unwanted disconnections so I just preferred to keep the things like this to prevent regressions. Consider that I already improved errors emitted in #1752 #1781 #1798.

Seems like you've had lots of reports due to hiding errors!

I saw various mentioning of the browser issue but nothing that gave me any more info as to what the problem actually is.

I really feel like we should just bin this check and consistently always emit errors. But I'm not using this in a browser at all so I can't do any testing here which isn't ideal.

Side question - how do you get the test runner to only run some tests? .only doesn't seem to do anything? --test-only and --test-name-pattern seem to just make it run nothing?

[SystemParadox]

I've added a test but it's failing for websockets with Error: WebSocket was closed before the connection was established. Possibly this is the "browser issue", that WebSocket doesn't work properly for stream.destroy(err)?

[robertsLando]

Seems like you've had lots of reports due to hiding errors!

Just to be clear, I'm not the creator of MQTTjs, I'm a developer like you that uses this package, I maintain this repo since ~2 years now, it was almost unmaintained when I asked the permissions to continue the development :) This is an issue that exists from far before I started working on this

I've added a test but it's failing for websockets with Error: WebSocket was closed before the connection was established. Possibly this is the "browser issue", that WebSocket doesn't work properly for stream.destroy(err)?

I think it is, yeah, and I'm not sure that's the only one.

Side question - how do you get the test runner to only run some tests? .only doesn't seem to do anything? --test-only and --test-name-pattern seem to just make it run nothing?

I suggest you to give a look at: https://github.com/mqttjs/MQTT.js/blob/main/DEVELOPMENT.md , use --test-name-pattern