From a6dc547f3808b4acf5ae2345c1f08c21d38cbc86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Mon, 2 Aug 2021 12:19:18 +0100 Subject: [PATCH] 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 | 11 ++++++---- v2/blockstore/readonly_test.go | 6 +++--- v2/blockstore/readwrite.go | 36 ++++++++++++++++++--------------- v2/blockstore/readwrite_test.go | 26 +++++++++++++++--------- 4 files changed, 46 insertions(+), 33 deletions(-) diff --git a/v2/blockstore/readonly.go b/v2/blockstore/readonly.go index dbf7bf6..2df9a79 100644 --- a/v2/blockstore/readonly.go +++ b/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. diff --git a/v2/blockstore/readonly_test.go b/v2/blockstore/readonly_test.go index ecc30e1..3fd16c8 100644 --- a/v2/blockstore/readonly_test.go +++ b/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) diff --git a/v2/blockstore/readwrite.go b/v2/blockstore/readwrite.go index ba87ee6..1192e74 100644 --- a/v2/blockstore/readwrite.go +++ b/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) -} diff --git a/v2/blockstore/readwrite_test.go b/v2/blockstore/readwrite_test.go index f3016e2..77442de 100644 --- a/v2/blockstore/readwrite_test.go +++ b/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) { -- GitLab