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

Resend PUBACK and PUBREC irrespective of the DUP flag #83

Merged
merged 6 commits into from
Oct 7, 2020

Conversation

yourslab
Copy link
Contributor

@yourslab yourslab commented Oct 6, 2020

In the OASIS MQTT Version 3.1.1 OASIS Standard, the following is stated:

After it has sent a PUBACK Packet the Receiver MUST treat any incoming PUBLISH packet that contains the same Packet Identifier as being a new publication, irrespective of the setting of its DUP flag.

Also, when AWS IoT Core resends a PUBLISH with the same packet ID as the first that wasn't PUBACK'd, the DUP flag will always be set to 0. Therefore, our library will return MQTTStateCollision and will NOT send a PUBACK in response.

For QoS 2, the OASIS spec does not seem to explicitly state anything about whether or not to resend a PUBREC in response to a resent PUBLISH with the DUP flag set to 0. However, I think the same logic still applies, so the PUBREC must still be resent in that case. Feel free to disagree.

@yourslab yourslab requested a review from muneebahmed10 October 6, 2020 21:53
@muneebahmed10
Copy link
Contributor

After it has sent a PUBACK Packet the Receiver MUST treat any incoming PUBLISH packet that contains the same Packet Identifier as being a new publication, irrespective of the setting of its DUP flag.

We already do this, since MQTTStateCollision can't be returned after the puback has been sent (since it's removed from the record). If the AWS IoT broker doesn't set the dup flag, it sounds like that's the fault of the broker. However, I'm fine with making this change if it doesn't violate the MQTT spec

@@ -1054,7 +1054,7 @@ static MQTTStatus_t handleIncomingPublish( MQTTContext_t * pContext,
* state engine. This will be handled by ignoring the
* #MQTTStateCollision status from the state engine. The publish
* data is not passed to the application. */
else if( ( status == MQTTStateCollision ) && ( publishInfo.dup == true ) )
else if( status == MQTTStateCollision )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're making this change specifically for AWS IoT Core, we should at least log a warning, if not an error. If the DUP flag isn't set, it seems to be a protocol violation (3.3.1.1):

The DUP flag MUST be set to 1 by the Client or Server when it attempts to re-deliver a PUBLISH Packet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the updated logs.


if( publishInfo.dup == false )
{
LogError( ( "DUP flag is 0 for duplicate incoming publish packet." ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a more ominous sounding error message like "Protocol violation: ..."? I think we should be clear that this isn't according to the spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would simply say "DUP flag is 0 for duplicate packet (MQTT-3.3.1.-1)" that gives all the breadcrumbs you need to figure it out. If you just google MQTT-3.3.1.-1 you will hit it directly.

muneebahmed10
muneebahmed10 previously approved these changes Oct 7, 2020
muneebahmed10
muneebahmed10 previously approved these changes Oct 7, 2020
@@ -955,7 +955,7 @@ static MQTTStatus_t deserializeConnack( const MQTTPacketInfo_t * pConnack,
if( ( pRemainingData[ 0 ] & MQTT_PACKET_CONNACK_SESSION_PRESENT_MASK )
== MQTT_PACKET_CONNACK_SESSION_PRESENT_MASK )
{
LogWarn( ( "CONNACK session present bit set." ) );
LogInfo( ( "CONNACK session present bit set." ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm pretty sure this was a leftover from v4_beta, where the session present flag wasn't given back to the application

@yourslab yourslab changed the title Allow PUBACK and PUBREC to be resent irrespective of the DUP flag Resend PUBACK and PUBREC irrespective of the DUP flag Oct 7, 2020
@yourslab yourslab merged commit c0fadc4 into FreeRTOS:master Oct 7, 2020
muneebahmed10 added a commit that referenced this pull request Oct 28, 2020
* Fix typo from #74

* Move error test to happy path due to #83
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

Successfully merging this pull request may close these issues.

3 participants