Short buffer read from socket

[mattadams]

If the redis server is quite busy or taking a lot of time to respond to requests it may not push all data across the socket in one go. When this happens the singular command append readBuf [read $sock] in the readEvent proc can result in a partial read of the server's response and cause truncated data to be passed to ParseBuf. Below is an excerpt of the error that this behaviour causes:

Could not set respType: Invalid type byte:  
    while executing
"{*}$errorCallback $msg"
    (class "::retcl" method "Error" line 2)
    invoked from within
"my Error "Could not set respType: $error $startIdx""
    (class "::retcl" method "ParseBuf" line 11)
    invoked from within
"my ParseBuf $buffer $idx"
    (class "::retcl" method "ParseBuf" line 81)
    invoked from within
"my ParseBuf $buffer $idx"
    (class "::retcl" method "ParseBuf" line 81)
    invoked from within
"my ParseBuf $readBuf $idx"
    (class "::retcl" method "readEvent" line 12)
    invoked from within
"::oo::Obj18 readEvent"

The solution here seems to be to allow the latter proc to make another call to [read $sock] if the buffer length is equal to or less than the expected index up to which the buffer has been parsed. I am not sure if this fix causes a potential problem asynchronous requests (it might) but I am largely using synchronous mode in an otherwise threaded program.

Below is a patch that fixes the problem of truncated buffers.

Index: tcltk/retcl.git/retcl.tm
==================================================================
--- tcltk/retcl.git/retcl.tm
+++ tcltk/retcl.git/retcl.tm
@@ -524,11 +524,11 @@
 
         append readBuf [read $sock]
 
         set idx 0
         while {$idx < [string length $readBuf]} {
-            set result [my ParseBuf $readBuf $idx]
+            set result [my ParseBuf $idx]
             if {$result eq {}} {
                 break
             }
 
             lassign $result idx type data
@@ -547,27 +547,32 @@
     # idx   : index up to which the buffer has been parsed
     # type  : type of the object found
     # value : value of the object
     #
     # or the empty string if no complete object could be parsed.
-    method ParseBuf {buffer startIdx} {
+    method ParseBuf {startIdx} {
+        set readBufLength [string length $readBuf]
 
-        if {![string length $buffer]} {
+        if {!$readBufLength} {
             return
         }
 
-        set respCode [string index $buffer $startIdx]
+        if {$readBufLength <= $startIdx} {
+            append readBuf [read $sock]
+        }
+
+        set respCode [string index $readBuf $startIdx]
         set respType [my TypeName $respCode]
 
         switch -- $respCode {
 
             "+" -
             "-" -
             ":" {
                 # Simple Strings, Errors, and Integers are handled
                 # straight forward
-                lassign [my ParseLine $buffer $startIdx+1] eol line
+                lassign [my ParseLine $readBuf $startIdx+1] eol line
                 if {$eol == -1} {
                     return
                 }
                 return [list [expr {$eol+2}] $respType $line]
             }
@@ -574,11 +579,11 @@
 
             "$" {
                 # Bulk Strings, the number of characters is specified in the
                 # first line. We handle Null values and empty strings right
                 # away.
-                lassign [my ParseLine $buffer $startIdx+1] eol bulkLen
+                lassign [my ParseLine $readBuf $startIdx+1] eol bulkLen
                 if {$eol == -1} {
                     return
                 }
 
                 # Null Bulk String
@@ -592,21 +597,21 @@
                 }
 
                 # Non-empty Bulk String
                 incr eol 2
                 set endIdx [expr {$eol+$bulkLen-1}]
-                if {[string length $buffer] < [expr {$endIdx+2}]} {
+                if {[string length $readBuf] < [expr {$endIdx+2}]} {
                     # Need to wait for more input
                     return
                 }
-                return [list [expr {$endIdx+3}] $respType [string range $buffer $eol $endIdx]]
+                return [list [expr {$endIdx+3}] $respType [string range $readBuf $eol $endIdx]]
             }
 
             "*" {
                 # Arrays, the number of elements is specified in the first
                 # line.
-                lassign [my ParseLine $buffer $startIdx+1] eol arrLen
+                lassign [my ParseLine $readBuf $startIdx+1] eol arrLen
                 if {$eol == -1} {
                     return
                 }
 
                 # Null Array
@@ -620,12 +625,13 @@
                 }
 
                 # Non-empty Array
                 set idx [expr {$eol+2}]
                 set elems [list]
+
                 while {$arrLen} {
-                    set elem [my ParseBuf $buffer $idx]
+                    set elem [my ParseBuf $idx]
                     if {$elem eq {}} {
                         return {}
                     }
 
                     lappend elems [lindex $elem 2]
@@ -635,11 +641,11 @@
 
                 return [list $idx $respType $elems]
             }
 
             default {
-                puts "Unhandled type: $buffer"
+                puts "Unhandled type: $readBuf"
             }
         }
     }
 
     method ParseLine {buffer startIdx} {

[mattadams]

Note that the buffer length check may need to be performed multiple times and should actually appear as:

-       if {$readBufLength <= $startIdx} {
+       while {$readBufLength <= $startIdx} {
            append readBuf [read $sock]
+          set readBufLength [string length $readBuf]
        }

[gahr]

Hi @mattadams, thanks for the report! I designed ParseBuf to return with an empty value in case of an incomplete read, so the next readEvent has a chance to append another bunch of bytes to readBuf and try again. Obviously, this isn't working in your case. It is not clear to me why, yet. Also, it's been a while since I worked on this codebase, so it'll take me some time to familiarize myself with it again :) Do you have a chance to provide the contents of the partial read in your use case?