aws / aws-iot-device-sdk-js-v2

Next generation AWS IoT Client SDK for Node.js using the AWS Common Runtime
Apache License 2.0
217 stars 97 forks source link

MQTT Manual Acknowledgement #450

Open t-moe opened 10 months ago

t-moe commented 10 months ago

Describe the issue

When looking at the api doc of mqtt5.Mqtt5Client::on('messageReceived',...) and mqtt.MqttClientConnection::subscribe its not clear to me how errors in the callback are handled. Is the message acknowledged anyways, even if an error occurred during processing?

Lets say I temporarily cannot process the data in my client. Should I throw an error from my message callback? E.g. I would expect that if I throw an error in my callback, that the callback is invoked again, with the same message (at least if using QoS=1)

Links

https://aws.github.io/aws-iot-device-sdk-js-v2/node/classes/mqtt5.Mqtt5Client.html#MESSAGE_RECEIVED https://aws.github.io/aws-iot-device-sdk-js-v2/node/classes/mqtt.MqttClientConnection.html#subscribe

bretambrose commented 10 months ago

The SDK clients do not support bridging.

I've thought about it some and if we did add bridging support I don't think it would necessarily match your expectations. The way I envision it working is something along the lines of:

"Here's a publish and a function object. Call the function when you want the client to send the ack."

Seems sensible for Qos 1, but I haven't had reason to think about Qos 2 much (given IoT Core's lack of support there).

t-moe commented 10 months ago

Thanks for your answer. It seems that the sdk clients always auto acknowledge the messages then. This was unclear to me from the documentation.

But I like your suggestion. This is also how rumqttc does it:

Adding this to aws-iot-device-sdk could probably be done in a non api breaking change, if manualAcks=false per default.

Could this be added?

bretambrose commented 10 months ago

In theory, yes. It would likely result in configuration that takes a differently-typed publish callback. It's a pretty significant amount of work, so not likely to happen soon. Let's leave this open for others to add their interest; this isn't the first time someone has asked about bridging functionality.

I should also add that if it does get added, it is more likely to be added to the MQTT5 client than the MQTT311 client (which is a lot more brittle and hard to extend)

t-moe commented 10 months ago

I see that this takes some work to implement, if you want to pass an additional argument to the publish callback. Thats why I linked the rumqttc API, because there the callback signature stays the same no matter if you are using manual or automatic acknowledgements ;).

Anyway, thanks for considering adding support for this, in whichever form suits you best.

bretambrose commented 5 months ago

Tracking issue here: https://github.com/awslabs/aws-c-mqtt/issues/368