v.io/syncbase/x/ref/services/syncbase/localblobstore/fs_cablobstore: Bug fix: blob reader code reading at EOF
I had a bug in the blob reader code triggered by performing a read at the end of file mark.
It would return an error indicating an illegal seek position, rather than simmply returning EOF.
This change returns EOF, as expected.
It also introduces a test that would have caught this.
It emnulates part of what syncbase does, by creating en empty blob,
and the using ResumeBlobWriter() to extend it. This process happens to
read the empty blob, and hence hist the bug.
The test is called from the tests of both the fs_cablobstore (implementation
layer) and localblobstore (the interface layer) so that the test can be
performed easily on other future implementations.
Change-Id: Ie97ebecba82fd5259c5a6380a7f9cf6f48a41f0d
diff --git a/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore.go b/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore.go
index d3386a0..4a67bf3 100644
--- a/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore.go
+++ b/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore.go
@@ -1050,6 +1050,8 @@
} else if err == io.EOF {
err = nil
}
+ } else if at == br.desc.size { // Reading at the end of the file, past the last fragment.
+ err = io.EOF
} else {
err = verror.New(errIllegalPositionForRead, br.ctx, br.pos, br.desc.size)
}
diff --git a/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore_test.go b/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore_test.go
index 75bc54e..0d964c3 100644
--- a/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore_test.go
+++ b/x/ref/services/syncbase/localblobstore/fs_cablobstore/fs_cablobstore_test.go
@@ -69,3 +69,29 @@
// Test it.
localblobstore_testlib.WriteViaChunks(t, ctx, bs)
}
+
+// This test case checks that empty blobs can be created, then extended via
+// ResumeBlobWriter.
+func TestCreateAndResume(t *testing.T) {
+ ctx, shutdown := test.V23Init()
+ defer shutdown()
+
+ // Make a temporary directory.
+ var err error
+ var testDirName string
+ testDirName, err = ioutil.TempDir("", "localblobstore_test")
+ if err != nil {
+ t.Fatalf("localblobstore_test: can't make tmp directory: %v\n", err)
+ }
+ defer os.RemoveAll(testDirName)
+
+ // Create an fs_cablobstore.
+ var bs localblobstore.BlobStore
+ bs, err = fs_cablobstore.Create(ctx, testDirName)
+ if err != nil {
+ t.Fatalf("fs_cablobstore.Create failed: %v", err)
+ }
+
+ // Test it.
+ localblobstore_testlib.CreateAndResume(t, ctx, bs)
+}
diff --git a/x/ref/services/syncbase/localblobstore/localblobstore_test.go b/x/ref/services/syncbase/localblobstore/localblobstore_test.go
index 49beec8..a258c0f 100644
--- a/x/ref/services/syncbase/localblobstore/localblobstore_test.go
+++ b/x/ref/services/syncbase/localblobstore/localblobstore_test.go
@@ -69,3 +69,29 @@
// Test it.
localblobstore_testlib.WriteViaChunks(t, ctx, bs)
}
+
+// This test case checks that empty blobs can be created, then extended via
+// ResumeBlobWriter.
+func TestCreateAndResume(t *testing.T) {
+ ctx, shutdown := test.V23Init()
+ defer shutdown()
+
+ // Make a temporary directory.
+ var err error
+ var testDirName string
+ testDirName, err = ioutil.TempDir("", "localblobstore_test")
+ if err != nil {
+ t.Fatalf("localblobstore_test: can't make tmp directory: %v\n", err)
+ }
+ defer os.RemoveAll(testDirName)
+
+ // Create an fs_cablobstore.
+ var bs localblobstore.BlobStore
+ bs, err = fs_cablobstore.Create(ctx, testDirName)
+ if err != nil {
+ t.Fatalf("fs_cablobstore.Create failed: %v", err)
+ }
+
+ // Test it.
+ localblobstore_testlib.CreateAndResume(t, ctx, bs)
+}
diff --git a/x/ref/services/syncbase/localblobstore/localblobstore_testlib/localblobstore_testlib.go b/x/ref/services/syncbase/localblobstore/localblobstore_testlib/localblobstore_testlib.go
index 481bea1..d6c1d9d 100644
--- a/x/ref/services/syncbase/localblobstore/localblobstore_testlib/localblobstore_testlib.go
+++ b/x/ref/services/syncbase/localblobstore/localblobstore_testlib/localblobstore_testlib.go
@@ -820,3 +820,70 @@
checkBlobAgainstReader(t, ctx, bs[1], blob0, NewRandReader(1, totalLength, 0, io.EOF), 6)
checkBlobChunksAgainstReader(t, ctx, bs[1], blob0, NewRandReader(1, totalLength, 0, io.EOF), 7)
}
+
+// checkBlobContent() checks that the named blob has the specified content.
+func checkBlobContent(t *testing.T, ctx *context.T, bs localblobstore.BlobStore, blobName string, content []byte) {
+ var err error
+ var br localblobstore.BlobReader
+ var data []byte
+ if br, err = bs.NewBlobReader(ctx, blobName); err != nil {
+ t.Fatalf("localblobstore.NewBlobReader failed: %v\n", err)
+ }
+ if data, err = ioutil.ReadAll(br); err != nil && err != io.EOF {
+ t.Fatalf("Read on br failed: %v\n", err)
+ }
+ if !bytes.Equal(data, content) {
+ t.Fatalf("Read on %q got %q, wanted %v\n", blobName, data, content)
+ }
+ if err = br.Close(); err != nil {
+ t.Fatalf("br.Close failed: %v\n", err)
+ }
+}
+
+// CreateAndResume() tests that it's possible to create a blob with
+// NewBlobWriter(), immediately close it, and then resume writing with
+// ResumeBlobWriter. This test is called out because syncbase does this, and
+// it exposed a bug in the reader code, which could not cope with a request to
+// read starting at the very end of a file, thus returning no bytes.
+func CreateAndResume(t *testing.T, ctx *context.T, bs localblobstore.BlobStore) {
+ var err error
+
+ // Create an empty, unfinalized blob.
+ var bw localblobstore.BlobWriter
+ if bw, err = bs.NewBlobWriter(ctx, ""); err != nil {
+ t.Fatalf("localblobstore.NewBlobWriter failed: %v\n", err)
+ }
+ blobName := bw.Name()
+ if err = bw.CloseWithoutFinalize(); err != nil {
+ t.Fatalf("bw.CloseWithoutFinalize failed: %v\n", verror.DebugString(err))
+ }
+
+ checkBlobContent(t, ctx, bs, blobName, nil)
+
+ // Reopen the blob, but append no bytes (an empty byte vector).
+ if bw, err = bs.ResumeBlobWriter(ctx, blobName); err != nil {
+ t.Fatalf("localblobstore.ResumeBlobWriter failed: %v\n", err)
+ }
+ if err = bw.AppendFragment(localblobstore.BlockOrFile{Block: []byte("")}); err != nil {
+ t.Fatalf("bw.AppendFragment failed: %v", err)
+ }
+ if err = bw.CloseWithoutFinalize(); err != nil {
+ t.Fatalf("bw.Close failed: %v\n", err)
+ }
+
+ checkBlobContent(t, ctx, bs, blobName, nil)
+
+ // Reopen the blob, and append a non-empty sequence of bytes.
+ content := []byte("some content")
+ if bw, err = bs.ResumeBlobWriter(ctx, blobName); err != nil {
+ t.Fatalf("localblobstore.ResumeBlobWriter.Close failed: %v\n", err)
+ }
+ if err = bw.AppendFragment(localblobstore.BlockOrFile{Block: content}); err != nil {
+ t.Fatalf("bw.AppendFragment failed: %v", err)
+ }
+ if err = bw.Close(); err != nil {
+ t.Fatalf("bw.Close failed: %v\n", err)
+ }
+
+ checkBlobContent(t, ctx, bs, blobName, content)
+}
