#593 fix GetObject seek to support http.ServeContent (#599)

* #593 fix GetObject seek to support http.ServeContent
* Enhance unit test for object Seeker

Author: hexadecy

api-get-object.go

@@ -212,10 +212,12 @@ type Object struct {
 	reqCh      chan<- getRequest
 	resCh      <-chan getResponse
 	doneCh     chan<- struct{}
-	prevOffset int64
 	currOffset int64
 	objectInfo ObjectInfo
 
+	// Ask lower level to initiate data fetching based on currOffset
+	seekData bool
+
 	// Keeps track of closed call.
 	isClosed bool
 
@@ -258,6 +260,10 @@ func (o *Object) doGetRequest(request getRequest) (getResponse, error) {
 	if response.Error != nil {
 		return response, response.Error
 	}
+
+	// Data are ready on the wire, no need to reinitiate connection in lower level
+	o.seekData = false
+
 	return response, nil
 }
 
@@ -266,8 +272,6 @@ func (o *Object) doGetRequest(request getRequest) (getResponse, error) {
 func (o *Object) setOffset(bytesRead int64) error {
 	// Update the currentOffset.
 	o.currOffset += bytesRead
-	// Save the current offset as previous offset.
-	o.prevOffset = o.currOffset
 
 	if o.currOffset >= o.objectInfo.Size {
 		return io.EOF
@@ -303,22 +307,9 @@ func (o *Object) Read(b []byte) (n int, err error) {
 		readReq.isFirstReq = true
 	}
 
-	// Verify if offset has changed and currOffset is greater than
-	// previous offset. Perhaps due to Seek().
-	offsetChange := o.prevOffset - o.currOffset
-	if offsetChange < 0 {
-		offsetChange = -offsetChange
-	}
-	if offsetChange > 0 {
-		// Fetch the new reader at the current offset again.
-		readReq.Offset = o.currOffset
-		readReq.DidOffsetChange = true
-	} else {
-		// No offset changes no need to fetch new reader, continue
-		// reading.
-		readReq.DidOffsetChange = false
-		readReq.Offset = 0
-	}
+	// Ask to establish a new data fetch routine based on seekData flag
+	readReq.DidOffsetChange = o.seekData
+	readReq.Offset = o.currOffset
 
 	// Send and receive from the first request.
 	response, err := o.doGetRequest(readReq)
@@ -430,8 +421,6 @@ func (o *Object) ReadAt(b []byte, offset int64) (n int, err error) {
 	if !o.objectInfoSet {
 		// Update the currentOffset.
 		o.currOffset += bytesRead
-		// Save the current offset as previous offset.
-		o.prevOffset = o.currOffset
 	} else {
 		// If this was not the first request update
 		// the offsets and compare against objectInfo
@@ -492,8 +481,6 @@ func (o *Object) Seek(offset int64, whence int) (n int64, err error) {
 			return 0, err
 		}
 	}
-	// Save current offset as previous offset.
-	o.prevOffset = o.currOffset
 
 	// Switch through whence.
 	switch whence {
@@ -527,6 +514,10 @@ func (o *Object) Seek(offset int64, whence int) (n int64, err error) {
 	if o.prevErr == io.EOF {
 		o.prevErr = nil
 	}
+
+	// Ask lower level to fetch again from source
+	o.seekData = true
+
 	// Return the effective offset.
 	return o.currOffset, nil
 }

api_functional_v4_test.go

@@ -1255,6 +1255,7 @@ func TestGetObjectReadSeekFunctional(t *testing.T) {
 
 	// Generate data more than 32K
 	buf := bytes.Repeat([]byte("2"), rand.Intn(1<<20)+32*1024)
+	bufSize := len(buf)
 
 	// Save the data
 	objectName := randString(60, rand.NewSource(time.Now().UnixNano()), "")
@@ -1263,10 +1264,21 @@ func TestGetObjectReadSeekFunctional(t *testing.T) {
 		t.Fatal("Error:", err, bucketName, objectName)
 	}
 
-	if n != int64(len(buf)) {
+	if n != int64(bufSize) {
 		t.Fatalf("Error: number of bytes does not match, want %v, got %v\n", len(buf), n)
 	}
 
+	defer func() {
+		err = c.RemoveObject(bucketName, objectName)
+		if err != nil {
+			t.Fatal("Error: ", err)
+		}
+		err = c.RemoveBucket(bucketName)
+		if err != nil {
+			t.Fatal("Error:", err)
+		}
+	}()
+
 	// Read the data back
 	r, err := c.GetObject(bucketName, objectName)
 	if err != nil {
@@ -1277,77 +1289,86 @@ func TestGetObjectReadSeekFunctional(t *testing.T) {
 	if err != nil {
 		t.Fatal("Error:", err, bucketName, objectName)
 	}
-	if st.Size != int64(len(buf)) {
+	if st.Size != int64(bufSize) {
 		t.Fatalf("Error: number of bytes in stat does not match, want %v, got %v\n",
 			len(buf), st.Size)
 	}
 
-	offset := int64(2048)
-	n, err = r.Seek(offset, 0)
-	if err != nil {
-		t.Fatal("Error:", err, offset)
-	}
-	if n != offset {
-		t.Fatalf("Error: number of bytes seeked does not match, want %v, got %v\n",
-			offset, n)
-	}
-	n, err = r.Seek(0, 1)
-	if err != nil {
-		t.Fatal("Error:", err)
-	}
-	if n != offset {
-		t.Fatalf("Error: number of current seek does not match, want %v, got %v\n",
-			offset, n)
-	}
-	_, err = r.Seek(offset, 2)
-	if err == nil {
-		t.Fatal("Error: seek on positive offset for whence '2' should error out")
-	}
-	n, err = r.Seek(-offset, 2)
-	if err != nil {
-		t.Fatal("Error:", err)
-	}
-	if n != st.Size-offset {
-		t.Fatalf("Error: number of bytes seeked back does not match, want %d, got %v\n", st.Size-offset, n)
-	}
-
-	var buffer1 bytes.Buffer
-	if _, err = io.CopyN(&buffer1, r, st.Size); err != nil {
-		if err != io.EOF {
-			t.Fatal("Error:", err)
+	// This following function helps us to compare data from the reader after seek
+	// with the data from the original buffer
+	cmpData := func(r io.Reader, start, end int) {
+		if end-start == 0 {
+			return
 		}
-	}
-	if !bytes.Equal(buf[len(buf)-int(offset):], buffer1.Bytes()) {
-		t.Fatal("Error: Incorrect read bytes v/s original buffer.")
-	}
-
-	// Seek again and read again.
-	n, err = r.Seek(offset-1, 0)
-	if err != nil {
-		t.Fatal("Error:", err)
-	}
-	if n != (offset - 1) {
-		t.Fatalf("Error: number of bytes seeked back does not match, want %v, got %v\n", offset-1, n)
-	}
-
-	var buffer2 bytes.Buffer
-	if _, err = io.CopyN(&buffer2, r, st.Size); err != nil {
-		if err != io.EOF {
-			t.Fatal("Error:", err)
+		buffer := bytes.NewBuffer([]byte{})
+		if _, err := io.CopyN(buffer, r, int64(bufSize)); err != nil {
+			if err != io.EOF {
+				t.Fatal("Error:", err)
+			}
+		}
+		if !bytes.Equal(buf[start:end], buffer.Bytes()) {
+			t.Fatal("Error: Incorrect read bytes v/s original buffer.")
 		}
-	}
-	// Verify now lesser bytes.
-	if !bytes.Equal(buf[2047:], buffer2.Bytes()) {
-		t.Fatal("Error: Incorrect read bytes v/s original buffer.")
 	}
 
-	err = c.RemoveObject(bucketName, objectName)
-	if err != nil {
-		t.Fatal("Error: ", err)
-	}
-	err = c.RemoveBucket(bucketName)
-	if err != nil {
-		t.Fatal("Error:", err)
+	// Generic seek error for errors other than io.EOF
+	seekErr := errors.New("seek error")
+
+	testCases := []struct {
+		offset    int64
+		whence    int
+		pos       int64
+		err       error
+		shouldCmp bool
+		start     int
+		end       int
+	}{
+		// Start from offset 0, fetch data and compare
+		{0, 0, 0, nil, true, 0, 0},
+		// Start from offset 2048, fetch data and compare
+		{2048, 0, 2048, nil, true, 2048, bufSize},
+		// Start from offset larger than possible
+		{int64(bufSize) + 1024, 0, 0, seekErr, false, 0, 0},
+		// Move to offset 0 without comparing
+		{0, 0, 0, nil, false, 0, 0},
+		// Move one step forward and compare
+		{1, 1, 1, nil, true, 1, bufSize},
+		// Move larger than possible
+		{int64(bufSize), 1, 0, seekErr, false, 0, 0},
+		// Provide negative offset with CUR_SEEK
+		{int64(-1), 1, 0, seekErr, false, 0, 0},
+		// Test with whence SEEK_END and with positive offset
+		{1024, 2, int64(bufSize) - 1024, io.EOF, true, 0, 0},
+		// Test with whence SEEK_END and with negative offset
+		{-1024, 2, int64(bufSize) - 1024, nil, true, bufSize - 1024, bufSize},
+		// Test with whence SEEK_END and with large negative offset
+		{-int64(bufSize) * 2, 2, 0, seekErr, true, 0, 0},
+	}
+
+	for i, testCase := range testCases {
+		// Perform seek operation
+		n, err := r.Seek(testCase.offset, testCase.whence)
+		// We expect an error
+		if testCase.err == seekErr && err == nil {
+			t.Fatalf("Test %d, unexpected err value: expected: %v, found: %v", i+1, testCase.err, err)
+		}
+		// We expect a specific error
+		if testCase.err != seekErr && testCase.err != err {
+			t.Fatalf("Test %d, unexpected err value: expected: %v, found: %v", i+1, testCase.err, err)
+		}
+		// If we expect an error go to the next loop
+		if testCase.err != nil {
+			continue
+		}
+		// Check the returned seek pos
+		if n != testCase.pos {
+			t.Fatalf("Test %d, error: number of bytes seeked does not match, want %v, got %v\n", i+1,
+				testCase.pos, n)
+		}
+		// Compare only if shouldCmp is activated
+		if testCase.shouldCmp {
+			cmpData(r, testCase.start, testCase.end)
+		}
 	}
 }