search

quartiq / minimq

Minimal no_std MQTT v5.0 client implementation
MIT License
143 stars 16 forks source link

Respect server "Retain Available" #137

Closed ryan-summers closed 1 year ago

ryan-summers commented 1 year ago

The server may indicate if it does not support retained messages. If this is the case, we should not permit transmission of retained messages.

jordens commented 1 year ago

Won't the server then just set a PUBACK Reason Code? Instead of pre-checking all user messages for server compatibility, I'd rather (a) expose a way to get info about the server and (b) trust that the server errors back.

ryan-summers commented 1 year ago

3.2.2.3.5 Retain Available 37 (0x25) Byte, Identifier of Retain Available.

Followed by a Byte field. If present, this byte declares whether the Server supports retained messages. A value of 0 means that retained messages are not supported. A value of 1 means retained messages are supported. If not present, then retained messages are supported. It is a Protocol Error to include Retain Available more than once or to use a value other than 0 or 1.

If a Server receives a CONNECT packet containing a Will Message with the Will Retain set to 1, and it does not support retained messages, the Server MUST reject the connection request. It SHOULD send CONNACK with Reason Code 0x9A (Retain not supported) and then it MUST close the Network Connection [MQTT-3.2.2-13].

A Client receiving Retain Available set to 0 from the Server MUST NOT send a PUBLISH packet with the RETAIN flag set to 1 [MQTT-3.2.2-14]. If the Server receives such a packet, this is a Protocol Error. The Server SHOULD send a DISCONNECT with Reason Code of 0x9A (Retain not supported) as described in section 4.13.

Based on this analysis, we don't get a PubAck, we get an immediate Disconnect in the best case, which could result in a connect -> pub -> disconnect cycle for firmware.

jordens commented 1 year ago

Ack. The question is still whether we need to enforce this at the library level. I'd still just leave it to the user. What else are you going to do in practice in an application? Assume your application relies on retained messages.

ryan-summers commented 1 year ago

Yeah, I agree here. Closing this as unnecessary

jordens commented 1 year ago

Doesn't the argument above (https://github.com/quartiq/minimq/issues/137#issuecomment-1651711160) also apply to #136, #134, #135?

ryan-summers commented 1 year ago

This is somewhat of an interesting point. I guess the downside of not being proactive is that we may cause the user to be unexpectedly disconnected from the broker, whereas if we are proactive we can prevent that.

I'm not certain that we will be able to provide meaningful error codes outside of MQTT session resets if we hit some of these edge cases, which may result in a bad user experience.

jordens commented 1 year ago

Being proactive also involves creating new error codes, increasing the number of error-paths. And as mentioned, in practice the application likely depends on certain QoS, packet size, and rates and have no choice other than to behave as it would on session resets: not work/crash/break. Let's do the enforcement locally where it involves no to very little overhead (e.g. max qos) but defer to the server it the overhead of tracking the requirement has a significant impact.

ryan-summers commented 1 year ago

Being proactive also involves creating new error codes, increasing the number of error-paths.

Yeah. I was thinking about this as well. I don't think a reasonable user application would proactively have error handling paths for QoS-too-high. I think ideally, we may have the capabilities to auto-downgrade i.e. QoS.

And as mentioned, in practice the application likely depends on certain QoS, packet size, and rates and have no choice other than to behave as it would on session resets: not work/crash/break.

I don't think this is true. For example, the Miniconf MQTT client uses QoS levels, but doesn't strictly need them. It would work just fine without them, and we would be fine if the QoS got downgraded. A user application may use miniconf, but not actually care about QoS levels at all.

jordens commented 1 year ago

Agreed. I meant "depend" in the sense that it doesn't account for the possibility of QoS-too-high (either as a connection termination loop or as an error from minimq). I would leave auto-downgrade to the application. Doing it in minimq would amount to ignoring the desired QoS if it exceeds the limit.

ryan-summers commented 1 year ago

Yeah, I think the solution would be an opt-in by the application if they know they don't care about QoS. I'll open an issue for it. Will take a look at the other parameters if they matter, but things like "retain" and "max packet size" we may just opt for having the server disconnect us.