Skip to content

Fix timeout check ordering in sendMessageVector#356

Open
benjacam wants to merge 2 commits intoFreeRTOS:mainfrom
benjacam:fix-timeout-ordering
Open

Fix timeout check ordering in sendMessageVector#356
benjacam wants to merge 2 commits intoFreeRTOS:mainfrom
benjacam:fix-timeout-ordering

Conversation

@benjacam
Copy link

Fix timeout check ordering in sendMessageVector

Description

I am running your library with MQTT_SEND_TIMEOUT_MS=0. My intention is to not block at all when attempting a send.
The library is running in a RTOS thread, and the getTimeFunction is returning the RTOS tick timer value (which is updated in another thread). In this scenario, the writev transport function successfully completed the requested send. But during the send, the other thread incremented the timer once. This means, that when this line executes:
if( calculateElapsedTime( pContext->getTime(), startTime ) > MQTT_SEND_TIMEOUT_MS )
the code considers the timeout elapsed, and breaks from the while loop, before advancing pIoVectIterator and decrementing vectorsToBeSent. This means that the packet was sent, but it isn't cleared from the pending vectors.

The fix is to move the calculateElapsedTime to the very end of the while loop, after the vect state is advanced.

Test Steps

Set MQTT_SEND_TIMEOUT_MS 0. Observe logs: "sendMessageVector: Unable to send packet: Timed out." , but also add to this log the values of bytesSentOrError and bytesToSend. Eventually, you will see a log where the timeout is reported, but bytesSentOrError ==bytesToSend. Or add assertion in the body of the if( calculateElapsedTime.

Checklist:

  • [ x] I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AniruddhaKanhere
Copy link
Member

Hello @benjacam, thank you for taking the time to report this issue.

You explanation and the change makes sense to me. Let us run the workflows and see if they catch any issues.

Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

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

There is a minor update which I would like to propose. Can you please take a look @benjacam?

Co-authored-by: Aniruddha Kanhere <kanherea@amazon.com>
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.

2 participants