RFC: Better Error Messages

blocks/blockstore/arc_cache.go

@@ -3,12 +3,13 @@ package blockstore
 import (
 	"context"
 
+	"github.com/ipfs/go-ipfs/errs"
+
 	"gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
 	"gx/ipfs/QmRg1gKTHzc3CZXSKzem8aR4E3TubFhbgXwfVuWnSK5CC5/go-metrics-interface"
 	lru "gx/ipfs/QmVYxfoJQiZijTgPNHCHgHELvQpbsJNTg6Crmc3dQkj3yy/golang-lru"
-	ds "gx/ipfs/QmdHG8MAuARdGHxx4rPQASLcvhz24fzjSQq7AJRAQEorq5/go-datastore"
 )
 
 // arccache wraps a BlockStore with an Adaptive Replacement Cache (ARC) for
@@ -36,13 +37,13 @@ func newARCCachedBS(ctx context.Context, bs Blockstore, lruSize int) (*arccache,
 
 func (b *arccache) DeleteBlock(k *cid.Cid) error {
 	if has, ok := b.hasCached(k); ok && !has {
-		return ErrNotFound
+		return errs.ErrCidNotFound
 	}
 
 	b.arc.Remove(k) // Invalidate cache before deleting.
 	err := b.blockstore.DeleteBlock(k)
 	switch err {
-	case nil, ds.ErrNotFound, ErrNotFound:
+	case nil, errs.ErrCidNotFound:
 		b.addCache(k, false)
 		return err
 	default:
@@ -84,15 +85,15 @@ func (b *arccache) Has(k *cid.Cid) (bool, error) {
 func (b *arccache) Get(k *cid.Cid) (blocks.Block, error) {
 	if k == nil {
 		log.Error("nil cid in arc cache")
-		return nil, ErrNotFound
+		return nil, errs.ErrCidNotFound
 	}
 
 	if has, ok := b.hasCached(k); ok && !has {
-		return nil, ErrNotFound
+		return nil, errs.ErrCidNotFound
 	}
 
 	bl, err := b.blockstore.Get(k)
-	if bl == nil && err == ErrNotFound {
+	if bl == nil && err == errs.ErrCidNotFound {
 		b.addCache(k, false)
 	} else if bl != nil {
 		b.addCache(k, true)

blocks/blockstore/arc_cache_test.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"testing"
 
+	"github.com/ipfs/go-ipfs/errs"
+
 	"gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
@@ -129,11 +131,11 @@ func TestGetAndDeleteFalseShortCircuit(t *testing.T) {
 
 	trap("get hit datastore", cd, t)
 
-	if bl, err := arc.Get(exampleBlock.Cid()); bl != nil || err != ErrNotFound {
+	if bl, err := arc.Get(exampleBlock.Cid()); bl != nil || err != errs.ErrCidNotFound {
 		t.Fatal("get returned invalid result")
 	}
 
-	if arc.DeleteBlock(exampleBlock.Cid()) != ErrNotFound {
+	if arc.DeleteBlock(exampleBlock.Cid()) != errs.ErrCidNotFound {
 		t.Fatal("expected ErrNotFound error")
 	}
 }

blocks/blockstore/blockstore.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 	"sync/atomic"
 
+	errs "github.com/ipfs/go-ipfs/errs"
 	dshelp "github.com/ipfs/go-ipfs/thirdparty/ds-help"
 	blocks "gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 
@@ -31,9 +32,6 @@ var ErrValueTypeMismatch = errors.New("the retrieved value is not a Block")
 // is different than expected.
 var ErrHashMismatch = errors.New("block in storage has different hash than requested")
 
-// ErrNotFound is an error returned when a block is not found.
-var ErrNotFound = errors.New("blockstore: block not found")
-
 // Blockstore wraps a Datastore block-centered methods and provides a layer
 // of abstraction which allows to add different caching strategies.
 type Blockstore interface {
@@ -112,12 +110,12 @@ func (bs *blockstore) HashOnRead(enabled bool) {
 func (bs *blockstore) Get(k *cid.Cid) (blocks.Block, error) {
 	if k == nil {
 		log.Error("nil cid in blockstore")
-		return nil, ErrNotFound
+		return nil, errs.ErrCidNotFound
 	}
 
 	maybeData, err := bs.datastore.Get(dshelp.CidToDsKey(k))
 	if err == ds.ErrNotFound {
-		return nil, ErrNotFound
+		return nil, errs.ErrCidNotFound
 	}
 	if err != nil {
 		return nil, err
@@ -180,7 +178,7 @@ func (bs *blockstore) Has(k *cid.Cid) (bool, error) {
 func (bs *blockstore) DeleteBlock(k *cid.Cid) error {
 	err := bs.datastore.Delete(dshelp.CidToDsKey(k))
 	if err == ds.ErrNotFound {
-		return ErrNotFound
+		return errs.ErrCidNotFound
 	}
 	return err
 }

blocks/blockstore/blockstore_test.go

@@ -6,9 +6,11 @@ import (
 	"fmt"
 	"testing"
 
-	dshelp "github.com/ipfs/go-ipfs/thirdparty/ds-help"
 	blocks "gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 
+	"github.com/ipfs/go-ipfs/errs"
+	dshelp "github.com/ipfs/go-ipfs/thirdparty/ds-help"
+
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
 	u "gx/ipfs/QmSU6eubNdhXjFBJBSksTp8kv8YRub8mGAPv8tVJHmL2EU/go-ipfs-util"
 	ds "gx/ipfs/QmdHG8MAuARdGHxx4rPQASLcvhz24fzjSQq7AJRAQEorq5/go-datastore"
@@ -32,7 +34,7 @@ func TestGetWhenKeyNotPresent(t *testing.T) {
 func TestGetWhenKeyIsNil(t *testing.T) {
 	bs := NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
 	_, err := bs.Get(nil)
-	if err != ErrNotFound {
+	if err != errs.ErrCidNotFound {
 		t.Fail()
 	}
 }

blocks/blockstore/bloom_cache.go

@@ -5,9 +5,11 @@ import (
 	"sync/atomic"
 	"time"
 
-	"gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
+	errs "github.com/ipfs/go-ipfs/errs"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
+	"gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
+
 	"gx/ipfs/QmRg1gKTHzc3CZXSKzem8aR4E3TubFhbgXwfVuWnSK5CC5/go-metrics-interface"
 	bloom "gx/ipfs/QmXqKGu7QzfRzFC4yd5aL9sThYx22vY163VGwmxfp5qGHk/bbloom"
 )
@@ -100,7 +102,7 @@ func (b *bloomcache) Rebuild(ctx context.Context) {
 
 func (b *bloomcache) DeleteBlock(k *cid.Cid) error {
 	if has, ok := b.hasCached(k); ok && !has {
-		return ErrNotFound
+		return errs.ErrCidNotFound
 	}
 
 	return b.blockstore.DeleteBlock(k)
@@ -136,7 +138,7 @@ func (b *bloomcache) Has(k *cid.Cid) (bool, error) {
 
 func (b *bloomcache) Get(k *cid.Cid) (blocks.Block, error) {
 	if has, ok := b.hasCached(k); ok && !has {
-		return nil, ErrNotFound
+		return nil, errs.ErrCidNotFound
 	}
 
 	return b.blockstore.Get(k)

blocks/blockstore/util/remove.go

@@ -6,9 +6,9 @@ import (
 	"io"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
-	ds "gx/ipfs/QmdHG8MAuARdGHxx4rPQASLcvhz24fzjSQq7AJRAQEorq5/go-datastore"
 
 	bs "github.com/ipfs/go-ipfs/blocks/blockstore"
+	"github.com/ipfs/go-ipfs/errs"
 	"github.com/ipfs/go-ipfs/pin"
 )
 
@@ -48,7 +48,7 @@ func RmBlocks(blocks bs.GCBlockstore, pins pin.Pinner, cids []*cid.Cid, opts RmB
 
 		for _, c := range stillOkay {
 			err := blocks.DeleteBlock(c)
-			if err != nil && opts.Force && (err == bs.ErrNotFound || err == ds.ErrNotFound) {
+			if err != nil && opts.Force && err == errs.ErrCidNotFound {
 				// ignore non-existent blocks
 			} else if err != nil {
 				out <- &RemovedBlock{Hash: c.String(), Error: err.Error()}

blockservice/blockservice.go

@@ -9,18 +9,18 @@ import (
 	"fmt"
 
 	"github.com/ipfs/go-ipfs/blocks/blockstore"
+	"github.com/ipfs/go-ipfs/errs"
 	exchange "github.com/ipfs/go-ipfs/exchange"
 	bitswap "github.com/ipfs/go-ipfs/exchange/bitswap"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
 	blocks "gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 	logging "gx/ipfs/QmSpJByNKFX1sCsHBEp3R73FL4NF6FnQTEGyNAXHm2GS52/go-log"
+	pe "gx/ipfs/QmaCt1pmsspjLCLx9FfKwvKHkLBbaDdgEmDkjGNZ2SCxdW/errors"
 )
 
 var log = logging.Logger("blockservice")
 
-var ErrNotFound = errors.New("blockservice: key not found")
-
 // BlockService is a hybrid block datastore. It stores data in a local
 // datastore and may retrieve data from a remote Exchange.
 // It uses an internal `datastore.Datastore` instance to store values.
@@ -173,26 +173,21 @@ func getBlock(ctx context.Context, c *cid.Cid, bs blockstore.Blockstore, f excha
 		return block, nil
 	}
 
-	if err == blockstore.ErrNotFound && f != nil {
-		// TODO be careful checking ErrNotFound. If the underlying
-		// implementation changes, this will break.
+	if pe.Cause(err) == errs.ErrCidNotFound && f != nil {
 		log.Debug("Blockservice: Searching bitswap")
-		blk, err := f.GetBlock(ctx, c)
-		if err != nil {
-			if err == blockstore.ErrNotFound {
-				return nil, ErrNotFound
-			}
-			return nil, err
-		}
-		return blk, nil
+		block, err = f.GetBlock(ctx, c)
 	}
 
-	log.Debug("Blockservice GetBlock: Not found")
-	if err == blockstore.ErrNotFound {
-		return nil, ErrNotFound
+	switch err.(type) {
+	default:
+		err = &errs.RetrievalError{err, c}
+	case *errs.RetrievalError:
+		// noop, avoid wrapping error twice
+	case nil:
+		// noop, no error
 	}
 
-	return nil, err
+	return block, err
 }
 
 // GetBlocks gets a list of blocks asynchronously and returns through

core/commands/ls.go

@@ -10,6 +10,7 @@ import (
 	cmds "github.com/ipfs/go-ipfs/commands"
 	core "github.com/ipfs/go-ipfs/core"
 	e "github.com/ipfs/go-ipfs/core/commands/e"
+	"github.com/ipfs/go-ipfs/errs"
 	offline "github.com/ipfs/go-ipfs/exchange/offline"
 	merkledag "github.com/ipfs/go-ipfs/merkledag"
 	path "github.com/ipfs/go-ipfs/path"
@@ -19,6 +20,7 @@ import (
 
 	node "gx/ipfs/QmPN7cwmpcc4DWXb4KTB9dNAJgjuPY69h3npsMfhRrQL9c/go-ipld-format"
 	"gx/ipfs/QmUyfy4QSr3NXym4etEiRyxBLqqAeKHJuRdi8AACxg63fZ/go-ipfs-cmdkit"
+	pe "gx/ipfs/QmaCt1pmsspjLCLx9FfKwvKHkLBbaDdgEmDkjGNZ2SCxdW/errors"
 )
 
 type LsLink struct {
@@ -134,7 +136,7 @@ The JSON output contains type information.
 				t := unixfspb.Data_DataType(-1)
 
 				linkNode, err := link.GetNode(req.Context(), dserv)
-				if err == merkledag.ErrNotFound && !resolve {
+				if pe.Cause(err) == errs.ErrCidNotFound && !resolve {
 					// not an error
 					linkNode = nil
 				} else if err != nil {

errs/errors.go

@@ -0,0 +1,25 @@
+package errs
+
+import (
+	"errors"
+	"fmt"
+
+	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
+)
+
+var ErrCidNotFound = errors.New("CID not found in local blockstore")
+
+// RetrievalError is an error indicating that a Cid could not be
+// retrived for some reason
+type RetrievalError struct {
+	Err error
+	Cid *cid.Cid
+}
+
+func (e *RetrievalError) Cause() error {
+	return e.Err
+}
+
+func (e *RetrievalError) Error() string {
+	return fmt.Sprintf("could not retrieve %s: %s", e.Cid.String(), e.Err.Error())
+}

exchange/bitswap/bitswap_test.go

@@ -8,8 +8,8 @@ import (
 	"testing"
 	"time"
 
-	blockstore "github.com/ipfs/go-ipfs/blocks/blockstore"
 	blocksutil "github.com/ipfs/go-ipfs/blocks/blocksutil"
+	"github.com/ipfs/go-ipfs/errs"
 	decision "github.com/ipfs/go-ipfs/exchange/bitswap/decision"
 	tn "github.com/ipfs/go-ipfs/exchange/bitswap/testnet"
 	mockrouting "github.com/ipfs/go-ipfs/routing/mock"
@@ -22,6 +22,7 @@ import (
 	travis "gx/ipfs/QmQgLZP9haZheimMHqqAjJh2LhRmNfEoZDfbtkpeMhi9xK/go-testutil/ci/travis"
 	blocks "gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 	p2ptestutil "gx/ipfs/QmUUNDRYXgfqdjxTg79ogkciczU5y4WY1tKMU2vEX9CRN7/go-libp2p-netutil"
+	pe "gx/ipfs/QmaCt1pmsspjLCLx9FfKwvKHkLBbaDdgEmDkjGNZ2SCxdW/errors"
 )
 
 // FIXME the tests are really sensitive to the network delay. fix them to work
@@ -287,7 +288,7 @@ func TestEmptyKey(t *testing.T) {
 	defer cancel()
 
 	_, err := bs.GetBlock(ctx, nil)
-	if err != blockstore.ErrNotFound {
+	if pe.Cause(err) != errs.ErrCidNotFound {
 		t.Error("empty str key should return ErrNotFound")
 	}
 }

exchange/bitswap/get.go

@@ -4,19 +4,19 @@ import (
 	"context"
 	"errors"
 
-	blockstore "github.com/ipfs/go-ipfs/blocks/blockstore"
+	"github.com/ipfs/go-ipfs/errs"
 	notifications "github.com/ipfs/go-ipfs/exchange/bitswap/notifications"
-	blocks "gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
+	blocks "gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 )
 
 type getBlocksFunc func(context.Context, []*cid.Cid) (<-chan blocks.Block, error)
 
 func getBlock(p context.Context, k *cid.Cid, gb getBlocksFunc) (blocks.Block, error) {
 	if k == nil {
 		log.Error("nil cid in GetBlock")
-		return nil, blockstore.ErrNotFound
+		return nil, errs.ErrCidNotFound
 	}
 
 	// Any async work initiated by this function must end when this function

filestore/filestore.go

@@ -10,9 +10,11 @@ package filestore
 import (
 	"context"
 
+	"gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
+
 	"github.com/ipfs/go-ipfs/blocks/blockstore"
+	"github.com/ipfs/go-ipfs/errs"
 	posinfo "github.com/ipfs/go-ipfs/thirdparty/posinfo"
-	"gx/ipfs/QmSn9Td7xgxm9EV7iEjTckpUWmWApggzPxu7eFGWkkpwin/go-block-format"
 
 	cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid"
 	logging "gx/ipfs/QmSpJByNKFX1sCsHBEp3R73FL4NF6FnQTEGyNAXHm2GS52/go-log"
@@ -114,7 +116,7 @@ func (f *Filestore) AllKeysChan(ctx context.Context) (<-chan *cid.Cid, error) {
 // ErrNotFound when the block is not stored.
 func (f *Filestore) DeleteBlock(c *cid.Cid) error {
 	err1 := f.bs.DeleteBlock(c)
-	if err1 != nil && err1 != blockstore.ErrNotFound {
+	if err1 != nil && err1 != errs.ErrCidNotFound {
 		return err1
 	}
 
@@ -125,9 +127,9 @@ func (f *Filestore) DeleteBlock(c *cid.Cid) error {
 	switch err2 {
 	case nil:
 		return nil
-	case blockstore.ErrNotFound:
-		if err1 == blockstore.ErrNotFound {
-			return blockstore.ErrNotFound
+	case errs.ErrCidNotFound:
+		if err1 == errs.ErrCidNotFound {
+			return errs.ErrCidNotFound
 		}
 		return nil
 	default:
@@ -144,7 +146,7 @@ func (f *Filestore) Get(c *cid.Cid) (blocks.Block, error) {
 		return nil, err
 	case nil:
 		return blk, nil
-	case blockstore.ErrNotFound:
+	case errs.ErrCidNotFound:
 		// try filestore
 	}