Skip to content

Commit

Permalink
Fix handling of zero length arrays
Browse files Browse the repository at this point in the history
Previously it was hanging. This logic is unfortunately gnarly.
  • Loading branch information
ammario committed Dec 12, 2023
1 parent df92b6f commit 3e523d8
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 9 deletions.
20 changes: 20 additions & 0 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,26 @@ func TestClient_NotFound(t *testing.T) {
require.Equal(t, "", got)
}

func TestClient_List(t *testing.T) {
t.Parallel()

_, client := redtest.StartRedisServer(t)

ctx := context.Background()

arr, err := client.Command(ctx, "LRANGE", "foo", "-1", -1).Strings()
require.NoError(t, err)
require.Empty(t, arr)

n, err := client.Command(ctx, "RPUSH", "foo", "bar").Int()
require.NoError(t, err)
require.Equal(t, 1, n)

arr, err = client.Command(ctx, "LRANGE", "foo", "-1", -1).Strings()
require.NoError(t, err)
require.Equal(t, []string{"bar"}, arr)
}

func TestClient_Race(t *testing.T) {
t.Parallel()

Expand Down
33 changes: 24 additions & 9 deletions result.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ func (r *Result) Strings() ([]string, error) {
return nil, fmt.Errorf("read array length: %w", err)
}

if ln < 0 {
if ln <= 0 {
return nil, nil
}

Expand Down Expand Up @@ -309,18 +309,27 @@ func (r *Result) writeTo(w io.Writer) (int64, replyType, error) {
}

isNewArray := typ == '*'
var newArraySize int
if isNewArray {
var err error
// New array
n, err := strconv.Atoi(string(s))
newArraySize, err = strconv.Atoi(string(s))
if err != nil {
return 0, typ, fmt.Errorf("invalid array length %q", s)
}
r.arrayStack = append(r.arrayStack, n)
if newArraySize > 0 {
r.arrayStack = append(r.arrayStack, newArraySize)
}
}

var n int
n, r.err = w.Write(s)
if !isNewArray {
// On new arrays, we don't want to advance any state
// as we've just modified the array stack.
incrRead()
} else if newArraySize == 0 {
// Nil array, so we want to advance pipeline.
incrRead()
}
return int64(n), typ, r.err
Expand Down Expand Up @@ -397,10 +406,11 @@ func (r *Result) ArrayLength() (int, error) {
}

// -1 is a nil array.
if gotN < 0 {
if gotN <= 0 {
return gotN, nil
}

// Sanity check that we've populated the array stack correctly.
if r.arrayStack[len(r.arrayStack)-1] != gotN {
// This should be impossible.
return 0, fmt.Errorf("array stack mismatch (expected %d, got %d)", r.arrayStack[len(r.arrayStack)-1], gotN)
Expand Down Expand Up @@ -432,16 +442,21 @@ func (r *Result) Next() bool {
r.mu.Lock()
defer r.mu.Unlock()

return r.next()
return r.hasMore()
}

// next returns true if there are more results to read.
func (r *Result) next() bool {
// hasMore returns true if there are more results to read.
func (r *Result) hasMore() bool {
if r.err != nil {
return false
}

return r.pipeline.at < r.pipeline.end || len(r.arrayStack) > 0
var arrays int
for _, n := range r.arrayStack {
arrays += n
}

return r.pipeline.at < r.pipeline.end || arrays > 0
}

// Close releases all resources associated with the result.
Expand All @@ -458,7 +473,7 @@ func (r *Result) Close() error {
}

func (r *Result) close() error {
for r.next() {
for r.hasMore() {
// Read the result into discard so that the connection can be reused.
_, _, err := r.writeTo(io.Discard)
if errors.Is(err, errClosed) {
Expand Down

0 comments on commit 3e523d8

Please sign in to comment.