Fixes in internal logic of sending packets #124

aggarw13 · 2020-12-05T03:58:42Z

This PR fixes 2 different issues, one for sendPacket and one for sendPublish internal functions, both related to sending packets over the network.

Issue 1

For sendPacket, there is a possibility of encountering an infinite loop if the Transport_Send_t always returns zero bytes sent. The PR addresses this issue by adding a timeout logic on retrying zero send attempts. The timeout can be configured through the MQTT_SEND_RETRY_TIMEOUT_MS macro.

Issue 2

For the sendPublish function (that calls the sendPacket function to send the complete packet over the network), there is a bug of treating partial or zero bytes return value of sendPacket as success. This PR updates the logic to treat partial or zero transmission of packets as failure in the call to MQTT_Publish API.

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

…on for ProcessLoop

…heir API doc

…ion always for cbmc.

…coreMQTT into fix/recv-timeout-issue

…Loop

…coreMQTT into fix/recv-timeout-issue

…ite-loop

muneebahmed10 · 2020-12-05T06:01:52Z

test/unit-test/core_mqtt_utest.c


    networkBuffer.size = 1000;
    incomingPacket.type = MQTT_PACKET_TYPE_PUBLISH;
    incomingPacket.remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;

    globalEntryTime = UINT32_MAX - MQTT_OVERFLOW_OFFSET;
-    expectedFinalTime = MQTT_TIMER_CALLS_PER_ITERATION * numIterations - MQTT_OVERFLOW_OFFSET;
+    expectedFinalTime = globalEntryTime + ( numIterations * MQTT_TIMER_CALLS_PER_ITERATION ) + 1;


Nit: Why this change?

This is correcting the calculation of the expected time counter after the call to MQTT_ProcessLoop. The +1 is for the call made to getTime before all iterations of the loop in the API.

What's the correction? This value is the same as before isn't it?
(UINT32_MAX - MQTT_OVERFLOW_OFFSET) + (numIterations * MQTT_TIMER_CALLS_PER_ITERATION ) + 1 is the same as
((0) - MQTT_OVERFLOW_OFFSET) + (numIterations * MQTT_TIMER_CALLS_PER_ITERATION) or
MQTT_TIMER_CALLS_PER_ITERATION * numIterations - MQTT_OVERFLOW_OFFSET

Also, basing expectedFinalTime off of globalEntryTime results in a calculation that always yields the final value, regardless if the counter rolls over back to 0 or not, which is what this test seems to be testing (e.g. If someone accidentally changed the value of globalEntryTime so there's no longer a rollover, this test would still pass).

Ultimately it doesn't matter, since the value is the same as it was before, but that's why this change looked weird to me

Also, basing expectedFinalTime off of globalEntryTime results in a calculation that always yields the final value, regardless if the counter rolls over back to 0 or not,

This is the main motivation from a resiliency and readability perspective.

I was saying it's less resilient since before, accidentally setting globalEntryTime incorrectly could be caught due to the test failing. Now, due to how expectedFinalTime is calculated from globalEntryTime, the test will pass regardless of the initial value of the entry time

muneebahmed10 · 2020-12-05T06:01:54Z

test/unit-test/core_mqtt_utest.c

-    uint8_t numIterations = ( MQTT_TIMER_OVERFLOW_TIMEOUT_MS / MQTT_TIMER_CALLS_PER_ITERATION ) + 1;
+    uint8_t numIterations = ( ( MQTT_TIMER_OVERFLOW_TIMEOUT_MS % MQTT_TIMER_CALLS_PER_ITERATION ) == 0 ) ?
+                            ( MQTT_TIMER_OVERFLOW_TIMEOUT_MS / MQTT_TIMER_CALLS_PER_ITERATION ) :
+                            ( ( MQTT_TIMER_OVERFLOW_TIMEOUT_MS / MQTT_TIMER_CALLS_PER_ITERATION ) + 1 );


Is this just ceil( a / b )? I believe something like ( MQTT_TIMER_OVERFLOW_TIMEOUT_MS + MQTT_TIMER_CALLS_PER_ITERATION - 1 ) / MQTT_TIMER_CALLS_PER_ITERATION would be equivalent and somewhat shorter, right? And a comment explaining that this is just a / b rounded up?

Good suggestion, have made the suggested changes.

muneebahmed10 · 2020-12-05T06:14:49Z

source/core_mqtt.c

@@ -1386,7 +1408,7 @@ static MQTTStatus_t sendPublish( MQTTContext_t * pContext,
                            pContext->networkBuffer.pBuffer,
                            headerSize );

-    if( bytesSent < 0 )
+    if( bytesSent < ( int32_t ) headerSize )


Are you going to make the same change in the other places where sendPacket is called, or will that be a separate PR?

Separate PR to avoid scope screep in this PR.

muneebahmed10 · 2020-12-05T06:24:02Z

source/include/core_mqtt_config_defaults.h

+ * If the timeout expires, the #MQTT_ProcessLoop and #MQTT_ReceiveLoop functions
+ * return #MQTTSendFailed.
+ *
+ * <b>Possible values:</b> Any positive integer up to SIZE_MAX. Recommended to


Now that I think about it, why do we say up to SIZE_MAX if it will only be compared to a uint32_t? On our dev systems, isn't that 2^ 64 - 1? Shouldn't this (and others) be "any positive 32 bit integer"?

Sure, good point. Made the change

muneebahmed10 · 2020-12-05T06:25:34Z

source/interface/transport_interface.h

@@ -183,7 +183,10 @@ typedef int32_t ( * TransportRecv_t )( NetworkContext_t * pNetworkContext,
 * @param[in] pBuffer Buffer containing the bytes to send over the network stack.
 * @param[in] bytesToSend Number of bytes to send over the network.
 *
- * @return The number of bytes sent or a negative error code.
+ * @return The number of bytes sent or a negative value to indicate error.
+ * If no data is transmitted over the network due to a full TX buffer and


Nit: We used @notehere when making a similar comment for TransportRecv_t. Also I think there would have to be an empty line between the @return and @note blocks, judging from the other doxygen comments

Made the API doc for send and receive consistent.

muneebahmed10 · 2020-12-07T12:07:20Z

test/unit-test/core_mqtt_utest.c


    networkBuffer.size = 1000;
    incomingPacket.type = MQTT_PACKET_TYPE_PUBLISH;
    incomingPacket.remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;

    globalEntryTime = UINT32_MAX - MQTT_OVERFLOW_OFFSET;
-    expectedFinalTime = MQTT_TIMER_CALLS_PER_ITERATION * numIterations - MQTT_OVERFLOW_OFFSET;
+    expectedFinalTime = globalEntryTime + ( numIterations * MQTT_TIMER_CALLS_PER_ITERATION ) + 1;


What's the correction? This value is the same as before isn't it?
(UINT32_MAX - MQTT_OVERFLOW_OFFSET) + (numIterations * MQTT_TIMER_CALLS_PER_ITERATION ) + 1 is the same as
((0) - MQTT_OVERFLOW_OFFSET) + (numIterations * MQTT_TIMER_CALLS_PER_ITERATION) or
MQTT_TIMER_CALLS_PER_ITERATION * numIterations - MQTT_OVERFLOW_OFFSET

Also, basing expectedFinalTime off of globalEntryTime results in a calculation that always yields the final value, regardless if the counter rolls over back to 0 or not, which is what this test seems to be testing (e.g. If someone accidentally changed the value of globalEntryTime so there's no longer a rollover, this test would still pass).

Ultimately it doesn't matter, since the value is the same as it was before, but that's why this change looked weird to me

source/core_mqtt.c

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

aggarw13 and others added 30 commits December 3, 2020 03:05

Fix issue of data recv being interrupted

5ec1c2f

Rename elapsedTimeMs variable to its express its new meaning

4205109

Use configuration const for recvExact timeout

0704c72

Remove timeout check from discardPacket and address CI check failures

adce766

Fix more CI check failures

f5aa1b3

Remove another unused local variable

76166d0

Re-instate timeout in discard to reduce scope of changes

1d73bb7

Remove unused variable again

0c5a9e3

Fix failing unit test

34adc86

Rename new config macro, and attempt to fix CBMC failures

2a3cc9a

Doc: Improvement suggestions from code review

70b181a

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

Fix quality check failures

68fcb16

Add test case to check partial network reads with zero timeout durati…

4ba8c53

…on for ProcessLoop

style: Improving naming

b710790

Address complexity failure

f8586dc

Address comments

c59d39b

Doc: Add blocking time equation of Receive/ProcessLoop functions in t…

af1ca02

…heir API doc

Improvement in API doc

86798bf

Set MQTT_RECV_POLLING_TIMEOUT_MS so that recvExact runs in one iterat…

e1de841

…ion always for cbmc.

Merge branch 'main' into fix/recv-timeout-issue

9a792bc

doc: Add information about zero return value for Transport_Recv_t

ebcfb92

Merge branch 'fix/recv-timeout-issue' of https://github.com/aggarw13/…

fe0a071

…coreMQTT into fix/recv-timeout-issue

fix: prevent possibility of infinite loop in timeout logic of Process…

32f0291

…Loop

Merge branch 'main' into fix/recv-timeout-issue

dd91637

Merge branch 'main' into fix/recv-timeout-issue

a4bf796

style: Minor changes

bf6f94d

Merge branch 'fix/recv-timeout-issue' of https://github.com/aggarw13/…

092e7ff

…coreMQTT into fix/recv-timeout-issue

hygiene: minor name fix

f0a1808

fix: Possibility of infinite loop in sendPacket

06669de

Add the new configuration to doxygen

9092419

aggarw13 added 7 commits December 4, 2020 22:49

test: Add mock transport send function that always returns zero

858da68

fix: Issues in sendPacket and sendPublish

71e2a40

test: add test for sendPacket timeout

1f6a138

Update Timer Overflow test

e03f4e5

test: temporarily comment out unused variable

605a83c

test: fix the timer overflow test

685ed2b

Merge remote-tracking branch 'origin/main' into fix/send-packet-infin…

3504b2d

…ite-loop

muneebahmed10 reviewed Dec 5, 2020

View reviewed changes

aggarw13 added 2 commits December 7, 2020 04:13

Address review comments

a82053c

Merge branch 'main' into fix/send-packet-infinite-loop

245ef09

muneebahmed10 previously approved these changes Dec 7, 2020

View reviewed changes

style: make log messages concise

ddbb4b7

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

aggarw13 dismissed muneebahmed10’s stale review via ddbb4b7 December 7, 2020 18:08

muneebahmed10 approved these changes Dec 7, 2020

View reviewed changes

gkwicker approved these changes Dec 7, 2020

View reviewed changes

gkwicker merged commit 0a2172b into FreeRTOS:main Dec 7, 2020

aggarw13 mentioned this pull request Dec 7, 2020

Update APIs to detect partial sends as failures #127

Merged

aggarw13 deleted the fix/send-packet-infinite-loop branch December 22, 2020 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes in internal logic of sending packets #124

Fixes in internal logic of sending packets #124

aggarw13 commented Dec 5, 2020 •

edited

Loading

muneebahmed10 Dec 5, 2020 •

edited

Loading

aggarw13 Dec 7, 2020

muneebahmed10 Dec 7, 2020

aggarw13 Dec 7, 2020

muneebahmed10 Dec 7, 2020 •

edited

Loading

muneebahmed10 Dec 5, 2020

aggarw13 Dec 7, 2020

muneebahmed10 Dec 5, 2020

aggarw13 Dec 7, 2020

muneebahmed10 Dec 5, 2020

aggarw13 Dec 7, 2020

muneebahmed10 Dec 5, 2020

aggarw13 Dec 7, 2020

muneebahmed10 Dec 7, 2020

Fixes in internal logic of sending packets #124

Fixes in internal logic of sending packets #124

Conversation

aggarw13 commented Dec 5, 2020 • edited Loading

Issue 1

Issue 2

muneebahmed10 Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muneebahmed10 Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aggarw13 commented Dec 5, 2020 •

edited

Loading

muneebahmed10 Dec 5, 2020 •

edited

Loading

muneebahmed10 Dec 7, 2020 •

edited

Loading