blockstore: use errors when API contracts are broken

It's technically correct to use panics in these situations,
as the downstream user is clearly breaking the contract documented in
the API's godoc.

However, all these methods return errors already,
so panicking is not our only option here.

There's another reason that makes a panic unfortunate.
ReadWrite.Finalize enables the panic behavior on other methods,
which makes it much easier to run into in production
even when the code and its tests work normally.

Finally, there's some precedent for IO interfaces using errors rather
than panics when they are used after closing.
For example, os.File.Read returns an error if the file is closed.

Note that this change doesn't make panics entirely impossible.
The blockstore could still run into exceptional and unexpected panics,
such as those caused by memory corruption or internal bugs.

v2/blockstore/readonly.go

@@ -24,7 +24,10 @@ import (
 
 var _ blockstore.Blockstore = (*ReadOnly)(nil)
 
-var errZeroLengthSection = fmt.Errorf("zero-length section not allowed by default; see WithZeroLengthSectionAsEOF option")
+var (
+	errZeroLengthSection = fmt.Errorf("zero-length carv2 section not allowed by default; see WithZeroLengthSectionAsEOF option")
+	errReadOnly          = fmt.Errorf("called write method on a read-only carv2 blockstore")
+)
 
 type (
 	// ReadOnly provides a read-only CAR Block Store.
@@ -176,7 +179,7 @@ func (b *ReadOnly) readBlock(idx int64) (cid.Cid, []byte, error) {
 
 // DeleteBlock is unsupported and always panics.
 func (b *ReadOnly) DeleteBlock(_ cid.Cid) error {
-	panic("called write method on a read-only blockstore")
+	return errReadOnly
 }
 
 // Has indicates if the store contains a block that corresponds to the given key.
@@ -303,12 +306,12 @@ func (b *ReadOnly) GetSize(key cid.Cid) (int, error) {
 
 // Put is not supported and always returns an error.
 func (b *ReadOnly) Put(blocks.Block) error {
-	panic("called write method on a read-only blockstore")
+	return errReadOnly
 }
 
 // PutMany is not supported and always returns an error.
 func (b *ReadOnly) PutMany([]blocks.Block) error {
-	panic("called write method on a read-only blockstore")
+	return errReadOnly
 }
 
 // WithAsyncErrorHandler returns a context with async error handling set to the given errHandler.

v2/blockstore/readonly_test.go

@@ -102,9 +102,9 @@ func TestReadOnly(t *testing.T) {
 				require.Equal(t, wantBlock, gotBlock)
 
 				// Assert write operations panic
-				require.Panics(t, func() { subject.Put(wantBlock) })
-				require.Panics(t, func() { subject.PutMany([]blocks.Block{wantBlock}) })
-				require.Panics(t, func() { subject.DeleteBlock(key) })
+				require.Error(t, subject.Put(wantBlock))
+				require.Error(t, subject.PutMany([]blocks.Block{wantBlock}))
+				require.Error(t, subject.DeleteBlock(key))
 			}
 
 			ctx, cancel := context.WithTimeout(context.Background(), time.Second*2)

v2/blockstore/readwrite.go

@@ -23,6 +23,8 @@ import (
 
 var _ blockstore.Blockstore = (*ReadWrite)(nil)
 
+var errFinalized = fmt.Errorf("cannot use a read-write carv2 blockstore after finalizing")
+
 // ReadWrite implements a blockstore that stores blocks in CARv2 format.
 // Blocks put into the blockstore can be read back once they are successfully written.
 // This implementation is preferable for a write-heavy workload.
@@ -284,22 +286,22 @@ func (b *ReadWrite) unfinalize() error {
 	return err
 }
 
-func (b *ReadWrite) panicIfFinalized() {
-	if b.header.DataSize != 0 {
-		panic("must not use a read-write blockstore after finalizing")
-	}
+func (b *ReadWrite) finalized() bool {
+	return b.header.DataSize != 0
 }
 
 // Put puts a given block to the underlying datastore
 func (b *ReadWrite) Put(blk blocks.Block) error {
-	// PutMany already calls panicIfFinalized.
+	// PutMany already checks b.finalized.
 	return b.PutMany([]blocks.Block{blk})
 }
 
 // PutMany puts a slice of blocks at the same time using batching
 // capabilities of the underlying datastore whenever possible.
 func (b *ReadWrite) PutMany(blks []blocks.Block) error {
-	b.panicIfFinalized()
+	if b.finalized() {
+		return errFinalized
+	}
 
 	b.mu.Lock()
 	defer b.mu.Unlock()
@@ -362,31 +364,33 @@ func (b *ReadWrite) Finalize() error {
 }
 
 func (b *ReadWrite) AllKeysChan(ctx context.Context) (<-chan cid.Cid, error) {
-	b.panicIfFinalized()
+	if b.finalized() {
+		return nil, errFinalized
+	}
 
 	return b.ReadOnly.AllKeysChan(ctx)
 }
 
 func (b *ReadWrite) Has(key cid.Cid) (bool, error) {
-	b.panicIfFinalized()
+	if b.finalized() {
+		return false, errFinalized
+	}
 
 	return b.ReadOnly.Has(key)
 }
 
 func (b *ReadWrite) Get(key cid.Cid) (blocks.Block, error) {
-	b.panicIfFinalized()
+	if b.finalized() {
+		return nil, errFinalized
+	}
 
 	return b.ReadOnly.Get(key)
 }
 
 func (b *ReadWrite) GetSize(key cid.Cid) (int, error) {
-	b.panicIfFinalized()
+	if b.finalized() {
+		return 0, errFinalized
+	}
 
 	return b.ReadOnly.GetSize(key)
 }
-
-func (b *ReadWrite) HashOnRead(h bool) {
-	b.panicIfFinalized()
-
-	b.ReadOnly.HashOnRead(h)
-}

v2/blockstore/readwrite_test.go

@@ -494,18 +494,24 @@ func TestReadWritePanicsOnlyWhenFinalized(t *testing.T) {
 	require.True(t, has)
 
 	subject.HashOnRead(true)
-	// Delete should always panic regardless of finalize
-	require.Panics(t, func() { subject.DeleteBlock(oneTestBlockCid) })
+	// Delete should always error regardless of finalize
+	require.Error(t, subject.DeleteBlock(oneTestBlockCid))
 
 	require.NoError(t, subject.Finalize())
-	require.Panics(t, func() { subject.Get(oneTestBlockCid) })
-	require.Panics(t, func() { subject.GetSize(anotherTestBlockCid) })
-	require.Panics(t, func() { subject.Has(anotherTestBlockCid) })
-	require.Panics(t, func() { subject.HashOnRead(true) })
-	require.Panics(t, func() { subject.Put(oneTestBlockWithCidV1) })
-	require.Panics(t, func() { subject.PutMany([]blocks.Block{anotherTestBlockWithCidV0}) })
-	require.Panics(t, func() { subject.AllKeysChan(context.Background()) })
-	require.Panics(t, func() { subject.DeleteBlock(oneTestBlockCid) })
+	require.Error(t, subject.Finalize())
+
+	_, err = subject.Get(oneTestBlockCid)
+	require.Error(t, err)
+	_, err = subject.GetSize(anotherTestBlockCid)
+	require.Error(t, err)
+	_, err = subject.Has(anotherTestBlockCid)
+	require.Error(t, err)
+
+	require.Error(t, subject.Put(oneTestBlockWithCidV1))
+	require.Error(t, subject.PutMany([]blocks.Block{anotherTestBlockWithCidV0}))
+	_, err = subject.AllKeysChan(context.Background())
+	require.Error(t, err)
+	require.Error(t, subject.DeleteBlock(oneTestBlockCid))
 }
 
 func TestReadWriteWithPaddingWorksAsExpected(t *testing.T) {