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 4 commits
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
8 changes: 8 additions & 0 deletions mongo/gridfs/download_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ func (ds *DownloadStream) Close() error {
}

ds.closed = true
if ds.cursor != nil {
return ds.cursor.Close(context.Background())
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change. I was initially slightly concerned. This adds blocking work with context.Background(), which could block indefinitely. But previously this would always return immediately.

Though, if the cursor is exhausted, this should do no work (killCursors won't be sent to the server). If the cursor is not exhausted, this still seems preferable to leaking cursors server-side in the event of an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good point and valid concern. But, leaking cursors doesn't seem great; and on the driver side, not closing the cursor in this case will leave a session open (client.NumberSessionsInProgress() > 0).

}
return nil
}

Expand Down Expand Up @@ -226,6 +229,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)
return ds.cursor.Err()
}
return errNoMoreChunks
}

Expand Down
29 changes: 29 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,34 @@ 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

fileName := "read-error-test"
fileData := make([]byte, 17000000)

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

dataReader := bytes.NewReader(fileData)
_, err = bucket.UploadFromStream(fileName, dataReader)
assert.Nil(mt, err, "UploadFromStream error: %v", err)

ds, err := bucket.OpenDownloadStreamByName(fileName)
assert.Nil(mt, err, "OpenDownloadStreamByName error: %v", err)

err = ds.SetReadDeadline(time.Now().Add(-1 * time.Second))
assert.Nil(mt, err, "SetReadDeadline error: %v", err)

p := make([]byte, len(fileData))
_, 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