TypeError: Cannot read property 'split' of undefined

[dborovcanin]

The error occurs here. Steps to reproduce:

1. Subscribe with QoS2
2. Receive PUBLISH message from the broker
3. Exit client after storing the message, but without sending corresponding PUBREC
4. Restart broker (cleaning message ID cache)
5. Restart client

I tried writing a simple snippet in MQTT.js so that you can get this error, but I was not able to reproduce it with this client library, which made me think that the problem is in MQTT client we use (https://github.com/eclipse/paho.mqtt.golang). Anyway, client errors should not cause the broker to crash and simple fix such as:

 if (!key) {
    return cb(new Error('Unknown key'))
}

is sufficient. I can't tell if other scenarios can lead to this error, but this fix will cover all the cases where the key can't be found in the cache (for whatever reason). I'm willing to send a PR, but I didn't want to do it on my own, without opening this issue.

I'm running the master version of Aedes and v6.0.0 of Aedes-Persistence-Redis natively, along with Redis 5.0.6.

[behrad]

can you propose a PR @dusanb94 ?

[dborovcanin]

Also, this line is important for persistence errors:
https://github.com/mcollina/aedes/blob/5a82b9979a94d4063c9d61accbde4e2b878b2d8a/lib/handlers/pubrec.js#L23
I'm not sure for other errors, but for the one this issue addresses, I think that it's just fine to log the warning and proceed as if the error didn't happen. According to the official MQTT 3.1.1 spec:

When its original sender receives the PUBREC packet, ownership of the Application Message is transferred to the receiver.

[mcollina]

I'm not really sure about that. Essentially, it's the act of receiving the PUBREC that errors. If that fails, we are definitly losing information.

[dborovcanin]

I'm not really sure about that. Essentially, it's the act of receiving the PUBREC that errors. If that fails, we are definitly losing information.

@mcollina Thanks for the quick answer. I'll doublecheck MQTT spec and Aedes code and, if I find that this error handling makes sense, I'll open an issue on Aedes repo so that we can continue this discussion there.

[dborovcanin]

@behrad I've sent a pull request for this issue(#57). Can you, please, review and leave your comments or merge if all good?

[dborovcanin]

@mcollina @behrad I've been testing this issue and the PR I've sent solves the problem in a protocol-compliant way (error handling is described here). However, this does not solve the cause of the problem, which is in step 4 (Restart broker - cleaning message ID cache). If I repeat the whole test skipping this step, everything works. This means that the broker restart messes up the persistence. This also made me think about how the persistence layer scales up on many instances and does this in-memory cache make a problem. Do you have any idea? Is this in-memory cache necessary?

[mcollina]

I'm lost. I think we'd need a failing test.

[dborovcanin]

I have a code snippet, but it's written in Go. As I already mentioned, I was not able to get this error using mqtt.js. Nonetheless, even if there is something buggy on the client-side, the broker must not fail. I'll send you the Go snippet (I can build a binary if you want) and steps so that you can test it.

[drasko]

@mcollina @dusanb94 here is the Go test that breaks the broker: vernemq/vernemq#1312 (comment)

It does not break Verne, but makes Verne disconnect the client.

Same code does not break Mosquitto, nor it disconnects the client (only logs a warning).

Problem is reported to Paho MQTT lib as well, and PR was filed.

Never the less, Aedes should not die when this happens. In the wors case, it should disconnect the client. In the better case it should probably have the same behaviour as Mosquitto (just ignore unexpected/duplicated PUBREC).

[dborovcanin]

Please have in mind this comment, also. As far as I know, this applies to MQTT 3.1.1, as well.