QoS 1 should only acknowledge if the handlers did not throw an Exception

[danielmarschall]

Today I noticed a problem where messages have been "lost", and it turns out that in the QoS 1 message-received-handler, there was an Exception. PHP-MQTT-Client has acknowledged the QoS 1 message although my code failed to process it.

This patch fixes the problem: When the message-received-handler fails with an Exception, then the message will not be acknowledged.

I am not 100% sure if I should also handle the case "Subscriber callback threw exception for published message". I have included this case since I think the message should only be acknowledged if the whole code/callbacks/handlers are processed smoothly without any errors.

What do you think?

[Namoshek]

I don't think this is how we should handle exceptions. MQTT is a communication protocol and the processing of messages done by clients is out of scope. The specification describes the message flow for message delivery, not for successful message processing. And the message clearly has been delivered successfully (i.e. was received by the client) when an exception occurs during callback execution, even though it might has not been processed successfully.

Furthermore, letting the broker retry sending a message to the client in all error cases might even be more harmful than useful. In case of transient exceptions (e.g. database is down), retrying might work. But in case of non-transient exceptions (e.g. division by zero, null pointer, etc.), retrying is worthless. If a lot of such problematic messages are not being acknowledged, the delivery queue for the client will fill at the broker and the broker will start to drop new messages. That's even worse, because the client never had a chance to even look at those messages.

My preferred solution would therefore be a client-side solution:

• a retry within the callback (e.g. a closure that is executed up to n times)
• the message is pushed to an in-process queue for background processing by a loop event handler
• the message is pushed to an out-of-process queue (e.g. Redis) for background processing by other processes
• a combination of the above

---

Regarding the subscription event handlers:
These are hooks, i.e. they are completely optional and should never ever impact the execution of the application itself. If PHP offered asynchronous processing support similar to other languages, I would even call these callbacks asynchronously to avoid locking up the main event loop. This is, however, not possible and the current implementation is the pragmatic solution.