Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect server "Retain Available" #137

Closed
ryan-summers opened this issue Jul 26, 2023 · 10 comments
Closed

Respect server "Retain Available" #137

ryan-summers opened this issue Jul 26, 2023 · 10 comments

Comments

@ryan-summers
Copy link
Member

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
Copy link
Member

jordens commented Jul 26, 2023

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
Copy link
Member Author

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
Copy link
Member

jordens commented Jul 26, 2023

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
Copy link
Member Author

Yeah, I agree here. Closing this as unnecessary

@jordens
Copy link
Member

jordens commented Aug 2, 2023

Doesn't the argument above (#137 (comment)) also apply to #136, #134, #135?

@ryan-summers
Copy link
Member Author

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
Copy link
Member

jordens commented Aug 2, 2023

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
Copy link
Member Author

ryan-summers commented Aug 3, 2023

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
Copy link
Member

jordens commented Aug 3, 2023

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants