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.

Sign up for GitHub

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
Merged

Resend PUBACK and PUBREC irrespective of the DUP flag #83

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7892145
Allow PUBACK and PUBREC to be resent irrespective of the DUP flag
yourslab Oct 6, 2020
1fb2e9d
Add log message when packet id already exists but DUP=0
yourslab Oct 6, 2020
3247e0f
Update error log
yourslab Oct 6, 2020
fe324d9
Name the publish packet as duplicate in the log
yourslab Oct 6, 2020
0ff0521
Change to LogInfo
yourslab Oct 7, 2020
53b6130
Change log message to specify the part of the specification
yourslab Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion source/core_mqtt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

{
status = MQTTSuccess;
duplicatePublish = true;
Expand All @@ -1063,8 +1063,14 @@ static MQTTStatus_t handleIncomingPublish( MQTTContext_t * pContext,
* for the duplicate incoming publish. */
publishRecordState = MQTT_CalculateStatePublish( MQTT_RECEIVE,
publishInfo.qos );

LogDebug( ( "Incoming publish packet with packet id %u already exists.",
packetIdentifier ) );

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.

}
}
else
{
Expand Down