GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer #653

benjirewis · 2021-04-27T21:55:49Z

GODRIVER-1925

Checks for a possible cursor error in DownloadStream#FillBuffer instead of always returning errNoMoreChunks when ds.cursor.Next returns false. If there is a cursor error, closes the cursor and returns the underlying error.

Adds a test to simulate a timeout during DownloadStream#Read and assert that context deadline exceeded is returned instead of io.EOF.

mongo/gridfs/download_stream.go

matthewdale

👍

mongo/integration/gridfs_test.go

iwysiu

lgtm! though as a nit, I don't feel like a lot of the comments in gridfs_test.go are necessary

divjotarora

Should we also close ds.cursor in DownloadStream#Close? If a user creates a DownloadStream and then closes it before fully reading the file, we should close the cursor to prevent cursor/session leaks in the application.

mongo/gridfs/download_stream.go

mongo/integration/gridfs_test.go

benjirewis

Removed some extraneous comments from the gridfs test and Closed the underlying cursor in DownloadStream#Close().

mongo/gridfs/download_stream.go

mongo/integration/gridfs_test.go

mongo/gridfs/download_stream.go

kevinAlbs

LGTM with an extra test case for Skip!

kevinAlbs · 2021-05-03T19:31:25Z

mongo/gridfs/download_stream.go

@@ -119,6 +119,9 @@ func (ds *DownloadStream) Close() error {
 	}

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


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.

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).

kevinAlbs · 2021-05-03T19:41:51Z

mongo/integration/gridfs_test.go

@@ -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


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

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

mongo/integration/gridfs_test.go

kevinAlbs · 2021-05-03T19:47:36Z

mongo/integration/gridfs_test.go

+			assert.Nil(mt, err, "SetReadDeadline error: %v", err)
+
+			p := make([]byte, len(fileData))
+			_, err = ds.Read(p)


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.

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).

* 'master' of https://github.com/mongodb/mongo-go-driver: (39 commits) GODRIVER-2004 Add Versioned API connection examples for Docs (mongodb#665) GODRIVER-1961 Run OCSP tests against RHEL 7.0 (mongodb#664) GODRIVER-1844 finer precision for getSecondsSinceEpoch (mongodb#666) GODRIVER-1973 create internal copy of aws v4 signing code (mongodb#657) GODRIVER-1951 Update the Go version for Evergreen builds to 1.16 (mongodb#663) GODRIVER-1949 add more ignored killAllSessions errors for unified tes… (mongodb#658) GODRIVER-1963 remove dropDatabase result (mongodb#660) GODRIVER-1180 Remove legacy transform functions from mongo (mongodb#583) GODRIVER-1937 Update legacy ListCollections to support the BatchSize option for server version 2.6 (mongodb#656) GODRIVER-1933 remove xtrace from shell scripts (mongodb#661) fix README error handling of FindOne (mongodb#636) GODRIVER-1938 update mongocryptd serverSelectionTimeout to 10 seconds (mongodb#659) GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mongodb#653) GODRIVER-1955 create labeledError interface (mongodb#651) GODRIVER-1947 Unmarshal unicode surrogate pairs correctly in UnmarshalExtJSON. (mongodb#649) Changed order of actions in ObjectIDFromHex func (mongodb#637) GODRIVER-1750 Ensure contexts are always cancelled during server monitoring (mongodb#654) GODRIVER-1931 Sync new cursors and SDAM LB tests (mongodb#655) GODRIVER-1981 Sync new transactions tests (mongodb#652) GODRIVER-1931 Run tests against LBs in Evergreen (mongodb#648) ...

…godb#653)

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer

4d24493

benjirewis commented Apr 27, 2021

View reviewed changes

mongo/gridfs/download_stream.go Outdated Show resolved Hide resolved

benjirewis requested review from divjotarora, kevinAlbs, iwysiu and matthewdale April 27, 2021 22:00

benjirewis marked this pull request as ready for review April 27, 2021 22:01

matthewdale approved these changes Apr 28, 2021

View reviewed changes

mongo/integration/gridfs_test.go Show resolved Hide resolved

iwysiu approved these changes Apr 28, 2021

View reviewed changes

divjotarora reviewed Apr 28, 2021

View reviewed changes

mongo/gridfs/download_stream.go Outdated Show resolved Hide resolved

mongo/integration/gridfs_test.go Outdated Show resolved Hide resolved

Benjamin Rewis added 2 commits April 28, 2021 14:59

Remove extraneous comments

297a5b9

Divjot comments

53ebe95

benjirewis commented Apr 28, 2021

View reviewed changes

mongo/gridfs/download_stream.go Outdated Show resolved Hide resolved

mongo/integration/gridfs_test.go Outdated Show resolved Hide resolved

benjirewis requested a review from divjotarora April 28, 2021 19:03

divjotarora reviewed Apr 28, 2021

View reviewed changes

mongo/gridfs/download_stream.go Outdated Show resolved Hide resolved

Always set closed and check if cursor is nil

9e85019

benjirewis requested a review from divjotarora April 28, 2021 20:12

divjotarora approved these changes Apr 28, 2021

View reviewed changes

kevinAlbs approved these changes May 3, 2021

View reviewed changes

Kevin comments

d114af5

benjirewis merged commit 0ecb074 into mongodb:master May 4, 2021

benjirewis deleted the gridFsCursorErrs.1925 branch May 4, 2021 16:07

benjirewis added a commit that referenced this pull request May 4, 2021

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (#653)

a69cc5e

tsedgwick pushed a commit to mongodb-forks/mongo-go-driver that referenced this pull request May 27, 2021

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mon…

3d771d0

…godb#653)

tsedgwick pushed a commit to mongodb-forks/mongo-go-driver that referenced this pull request Jun 1, 2021

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mon…

7a6ada0

…godb#653)

faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mon…

b2bb373

…godb#653)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer #653

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer #653

benjirewis commented Apr 27, 2021

matthewdale left a comment

iwysiu left a comment

divjotarora left a comment

benjirewis left a comment

kevinAlbs left a comment

kevinAlbs May 3, 2021

benjirewis May 4, 2021

kevinAlbs May 3, 2021

benjirewis May 4, 2021

kevinAlbs May 3, 2021

benjirewis May 4, 2021

	// 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

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer #653

GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer #653

Conversation

benjirewis commented Apr 27, 2021

matthewdale left a comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

divjotarora left a comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

kevinAlbs May 3, 2021

Choose a reason for hiding this comment

benjirewis May 4, 2021

Choose a reason for hiding this comment

kevinAlbs May 3, 2021

Choose a reason for hiding this comment

benjirewis May 4, 2021

Choose a reason for hiding this comment

kevinAlbs May 3, 2021

Choose a reason for hiding this comment

benjirewis May 4, 2021

Choose a reason for hiding this comment