Update stream_buffer.c (#94)
Add necessary checks when sending data to the stream/message buffer in order to avoid a task deadlock when attempting to write a longer stream/message than the underlying buffer can write.
diff --git a/stream_buffer.c b/stream_buffer.c
index bef8214..449c4fe 100644
--- a/stream_buffer.c
+++ b/stream_buffer.c
@@ -518,7 +518,11 @@
size_t xReturn, xSpace = 0;
size_t xRequiredSpace = xDataLengthBytes;
TimeOut_t xTimeOut;
-
+
+ /* Having a 'isFeasible' variable allows to respect the convention that there is only a return statement at the end. Othewise, return
+ * could be done as soon as we realise the send cannot happen. We will let the call to 'prvWriteMessageToBuffer' dealing with this scenario. */
+ BaseType_t xIsFeasible;
+
configASSERT( pvTxData );
configASSERT( pxStreamBuffer );
@@ -532,13 +536,56 @@
/* Overflow? */
configASSERT( xRequiredSpace > xDataLengthBytes );
+
+ /* In the case of the message buffer, one has to be able to write the complete message as opposed to
+ * a stream buffer for semantic reasons. Check if it is physically possible to write the message given
+ * the length of the buffer. */
+ if(xRequiredSpace > pxStreamBuffer->xLength)
+ {
+ /* The message could never be written because it is greater than the buffer length.
+ * By setting xIsFeasable to FALSE, we skip over the following do..while loop, thus avoiding
+ * a deadlock. The call to 'prvWriteMessageToBuffer' toward the end of this function with
+ * xRequiredSpace greater than xSpace will suffice in not writing anything to the internal buffer.
+ * Now, the function will return 0 because the message could not be written. Should an error code be
+ * returned instead ??? In my opinion, probably.. But the return type doesn't allow for negative
+ * values to be returned. A confusion could exist to the caller. Returning 0 because a timeout occurred
+ * and a subsequent send attempts could eventually succeed, and returning 0 because a write could never
+ * happen because of the size are two scenarios to me :/ */
+ xIsFeasible = FALSE;
+ }
+ else
+ {
+ /* It is possible to write the message completely in the buffer. This is the intended route.
+ * Let's continue with the regular timeout logic. */
+ xIsFeasible = TRUE;
+ }
}
else
{
- mtCOVERAGE_TEST_MARKER();
+ /* In the case of the stream buffer, not being able to completely write the message in the buffer
+ * is an acceptable scenario, but it has to be dealt with properly */
+ if(xRequiredSpace > pxStreamBuffer->xLength)
+ {
+ /* Not enough buffer space. We will attempt to write as much as we can in this run
+ * so that the caller can send the remaining in subsequent calls. We avoid a deadlock by
+ * offering the possibility to take the 'else' branch in the 'if( xSpace < xRequiredSpace )'
+ * condition inside the following do..while loop */
+ xRequiredSpace = pxStreamBuffer->xLength;
+
+ /* TODO FIXME: Is there a check we should do with the xTriggerLevelBytes value ? */
+
+ /* With the adjustment to 'xRequiredSpace', the deadlock is avoided, thus it's now feasible. */
+ xIsFeasible = TRUE;
+ }
+ else
+ {
+ /* It is possible to write the message completely in the buffer. */
+ xIsFeasible = TRUE;
+ }
}
- if( xTicksToWait != ( TickType_t ) 0 )
+ /* Added check against xIsFeasible. If it's not feasible, don't even wait for notification, let the call to 'prvWriteMessageToBuffer' do nothing and return 0 */
+ if( xTicksToWait != ( TickType_t ) 0 && xIsFeasible == TRUE )
{
vTaskSetTimeOutState( &xTimeOut );
