Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer #653

Merged
merged 5 commits into from
May 4, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions mongo/gridfs/download_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,11 @@ func (ds *DownloadStream) GetFile() *File {
func (ds *DownloadStream) fillBuffer(ctx context.Context) error {
if !ds.cursor.Next(ctx) {
ds.done = true
// Check for cursor error, otherwise there are no more chunks.
if ds.cursor.Err() != nil {
ds.cursor.Close(ctx)
benjirewis marked this conversation as resolved.
Show resolved Hide resolved
return ds.cursor.Err()
}
return errNoMoreChunks
}

Expand Down
35 changes: 35 additions & 0 deletions mongo/integration/gridfs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"io"
"math/rand"
"runtime"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -369,6 +370,40 @@ func TestGridFS(x *testing.T) {
_, err = bucket.OpenDownloadStream(oid)
assert.Equal(mt, gridfs.ErrMissingChunkSize, err, "expected error %v, got %v", gridfs.ErrMissingChunkSize, err)
})
mt.Run("cursor error during read after downloading", func(mt *mtest.T) {
// To simulate a cursor error we upload a file larger than the 16MB default batch size,
// so Read() will actually execute a getMore against the server. Since the ReadDeadline is
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC the test itself won't send a getMore. It should fail on the first iteration of the cursor.

Suggested change
// so Read() will actually execute a getMore against the server. Since the ReadDeadline is
// so the underlying cursor remains open on the server. Since the ReadDeadline is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right. Previously I was using a failpoint on getMore, but the timeout error just comes from iterating the cursor. Changed.

// set in the past, this should cause a timeout.
benjirewis marked this conversation as resolved.
Show resolved Hide resolved

// Create file to upload.
fileName := "read-error-test"
fileData := make([]byte, 17000000)

// Create bucket.
bucket, err := gridfs.NewBucket(mt.DB)
assert.Nil(mt, err, "NewBucket error: %v", err)
defer func() { _ = bucket.Drop() }()
benjirewis marked this conversation as resolved.
Show resolved Hide resolved

// Open data reader from file and upload to bucket.
dataReader := bytes.NewReader(fileData)
_, err = bucket.UploadFromStream(fileName, dataReader)
assert.Nil(mt, err, "UploadFromStream error: %v", err)

// Open a download stream to fileName.
ds, err := bucket.OpenDownloadStreamByName(fileName)
assert.Nil(mt, err, "OpenDownloadStreamByName error: %v", err)

// Set a read deadline in the past to cause a timeout.
err = ds.SetReadDeadline(time.Now().Add(-1 * time.Second))
assert.Nil(mt, err, "SetReadDeadline error: %v", err)

// Try to read.
p := make([]byte, 17000000)
divjotarora marked this conversation as resolved.
Show resolved Hide resolved
_, err = ds.Read(p)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you parameterize this test case, or add a test for Skip as well? This reminded me about GODRIVER-1806, and how Read and Skip are near duplicates, but Skip avoids extra copies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah great catch. Read and Skip both call fillBuffer, where the bug in error surfacing was. Added a different test case for Skip (wasn't sure how to parameterize given Skip and Read have different function signatures).

assert.NotNil(mt, err, "expected error from Read, got nil")
assert.True(mt, strings.Contains(err.Error(), "context deadline exceeded"),
"expected error to contain 'context deadline exceeded', got %v", err.Error())
})
})

mt.RunOpts("bucket collection accessors", noClientOpts, func(mt *mtest.T) {
Expand Down