add a test to blockservice to demonstate GetBlocks failure.

blockservice/blocks_test.go

@@ -11,7 +11,9 @@ import (
 	blocks "github.com/jbenet/go-ipfs/blocks"
 	blockstore "github.com/jbenet/go-ipfs/blocks/blockstore"
 	bitswap "github.com/jbenet/go-ipfs/exchange/bitswap"
+	tn "github.com/jbenet/go-ipfs/exchange/bitswap/testnet"
 	offline "github.com/jbenet/go-ipfs/exchange/offline"
+	"github.com/jbenet/go-ipfs/routing/mock"
 	u "github.com/jbenet/go-ipfs/util"
 )
 
@@ -61,4 +63,42 @@ func TestBlocks(t *testing.T) {
 }
 
 func TestGetBlocks(t *testing.T) {
+	net := tn.VirtualNetwork()
+	rs := mock.VirtualRoutingServer()
+	sg := bitswap.NewSessionGenerator(net, rs)
+	bg := bitswap.NewBlockGenerator()
+
+	instances := sg.Instances(4)
+	blks := bg.Blocks(50)
+	// TODO: verify no duplicates
+
+	var servs []*BlockService
+	for _, i := range instances {
+		bserv, err := New(i.Blockstore, i.Exchange)
+		if err != nil {
+			t.Fatal(err)
+		}
+		servs = append(servs, bserv)
+	}
+
+	var keys []u.Key
+	for _, blk := range blks {
+		keys = append(keys, blk.Key())
+		servs[0].AddBlock(blk)
+	}
+
+	for i := 1; i < 4; i++ {
+		ctx, _ := context.WithTimeout(context.TODO(), time.Second*5)
+		out := servs[i].GetBlocks(ctx, keys)
+		gotten := make(map[u.Key]*blocks.Block)
+		for blk := range out {
+			if _, ok := gotten[blk.Key()]; ok {
+				t.Fatal("Got duplicate block!")
+			}
+			gotten[blk.Key()] = blk
+		}
+		if len(gotten) != len(blks) {
+			t.Fatalf("Didnt get enough blocks back: %d/%d", len(gotten), len(blks))
+		}
+	}
 }

exchange/bitswap/bitswap.go

@@ -262,7 +262,6 @@ func (bs *bitswap) ReceiveMessage(ctx context.Context, p peer.Peer, incoming bsm
 		}
 	}
 
-	first := true
 	for _, key := range incoming.Wantlist() {
 		// TODO: might be better to check if we have the block before checking
 		//			if we should send it to someone
@@ -273,11 +272,9 @@ func (bs *bitswap) ReceiveMessage(ctx context.Context, p peer.Peer, incoming bsm
 				// Create a separate message to send this block in
 				blkmsg := bsmsg.New()
 
-				if first {
-					for _, k := range bs.wantlist.Keys() {
-						blkmsg.AddWanted(k)
-					}
-					first = false
+				// TODO: only send this the first time
+				for _, k := range bs.wantlist.Keys() {
+					blkmsg.AddWanted(k)
 				}
 
 				blkmsg.AddBlock(block)
@@ -287,16 +284,6 @@ func (bs *bitswap) ReceiveMessage(ctx context.Context, p peer.Peer, incoming bsm
 		}
 	}
 
-	// If they send us a block, we should guarantee that we send
-	// them our updated want list one way or another
-	if len(incoming.Blocks()) > 0 && first {
-		message := bsmsg.New()
-		for _, k := range bs.wantlist.Keys() {
-			message.AddWanted(k)
-		}
-		return p, message
-	}
-
 	return nil, nil
 }
 

exchange/bitswap/bitswap_test.go

-package bitswap_test
+package bitswap
 
 import (
 	"bytes"
@@ -7,13 +7,8 @@ import (
 	"time"
 
 	context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context"
-	. "github.com/jbenet/go-ipfs/exchange/bitswap"
 
-	ds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore"
-	ds_sync "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore/sync"
 	blocks "github.com/jbenet/go-ipfs/blocks"
-	blockstore "github.com/jbenet/go-ipfs/blocks/blockstore"
-	exchange "github.com/jbenet/go-ipfs/exchange"
 	tn "github.com/jbenet/go-ipfs/exchange/bitswap/testnet"
 	peer "github.com/jbenet/go-ipfs/peer"
 	mock "github.com/jbenet/go-ipfs/routing/mock"
@@ -170,7 +165,7 @@ func PerformDistributionTest(t *testing.T, numInstances, numBlocks int) {
 	}
 }
 
-func getOrFail(bitswap instance, b *blocks.Block, t *testing.T, wg *sync.WaitGroup) {
+func getOrFail(bitswap Instance, b *blocks.Block, t *testing.T, wg *sync.WaitGroup) {
 	if _, err := bitswap.blockstore.Get(b.Key()); err != nil {
 		_, err := bitswap.exchange.GetBlock(context.Background(), b.Key())
 		if err != nil {
@@ -246,84 +241,3 @@ func TestSendToWantingPeer(t *testing.T) {
 		t.Fatal("Expected to receive alpha from me")
 	}
 }
-
-func NewBlockGenerator() BlockGenerator {
-	return BlockGenerator{}
-}
-
-type BlockGenerator struct {
-	seq int
-}
-
-func (bg *BlockGenerator) Next() *blocks.Block {
-	bg.seq++
-	return blocks.NewBlock([]byte(string(bg.seq)))
-}
-
-func (bg *BlockGenerator) Blocks(n int) []*blocks.Block {
-	blocks := make([]*blocks.Block, 0)
-	for i := 0; i < n; i++ {
-		b := bg.Next()
-		blocks = append(blocks, b)
-	}
-	return blocks
-}
-
-func NewSessionGenerator(
-	net tn.Network, rs mock.RoutingServer) SessionGenerator {
-	return SessionGenerator{
-		net: net,
-		rs:  rs,
-		seq: 0,
-	}
-}
-
-type SessionGenerator struct {
-	seq int
-	net tn.Network
-	rs  mock.RoutingServer
-}
-
-func (g *SessionGenerator) Next() instance {
-	g.seq++
-	return session(g.net, g.rs, []byte(string(g.seq)))
-}
-
-func (g *SessionGenerator) Instances(n int) []instance {
-	instances := make([]instance, 0)
-	for j := 0; j < n; j++ {
-		inst := g.Next()
-		instances = append(instances, inst)
-	}
-	return instances
-}
-
-type instance struct {
-	peer       peer.Peer
-	exchange   exchange.Interface
-	blockstore blockstore.Blockstore
-}
-
-// session creates a test bitswap session.
-//
-// NB: It's easy make mistakes by providing the same peer ID to two different
-// sessions. To safeguard, use the SessionGenerator to generate sessions. It's
-// just a much better idea.
-func session(net tn.Network, rs mock.RoutingServer, id peer.ID) instance {
-	p := peer.WithID(id)
-
-	adapter := net.Adapter(p)
-	htc := rs.Client(p)
-	bstore := blockstore.NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
-
-	const alwaysSendToPeer = true
-	ctx := context.TODO()
-
-	bs := New(ctx, p, adapter, htc, bstore, alwaysSendToPeer)
-
-	return instance{
-		peer:       p,
-		exchange:   bs,
-		blockstore: bstore,
-	}
-}

exchange/bitswap/testutils.go

+package bitswap
+
+import (
+	"code.google.com/p/go.net/context"
+	ds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore"
+	ds_sync "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore/sync"
+	"github.com/jbenet/go-ipfs/blocks"
+	"github.com/jbenet/go-ipfs/blocks/blockstore"
+	"github.com/jbenet/go-ipfs/exchange"
+	tn "github.com/jbenet/go-ipfs/exchange/bitswap/testnet"
+	"github.com/jbenet/go-ipfs/peer"
+	"github.com/jbenet/go-ipfs/routing/mock"
+)
+
+/*
+TODO: This whole file needs somewhere better to live.
+The issue is that its very difficult to move it somewhere else
+without creating circular dependencies.
+Additional thought required.
+*/
+
+func NewBlockGenerator() BlockGenerator {
+	return BlockGenerator{}
+}
+
+type BlockGenerator struct {
+	seq int
+}
+
+func (bg *BlockGenerator) Next() *blocks.Block {
+	bg.seq++
+	return blocks.NewBlock([]byte(string(bg.seq)))
+}
+
+func (bg *BlockGenerator) Blocks(n int) []*blocks.Block {
+	blocks := make([]*blocks.Block, 0)
+	for i := 0; i < n; i++ {
+		b := bg.Next()
+		blocks = append(blocks, b)
+	}
+	return blocks
+}
+
+func NewSessionGenerator(
+	net tn.Network, rs mock.RoutingServer) SessionGenerator {
+	return SessionGenerator{
+		net: net,
+		rs:  rs,
+		seq: 0,
+	}
+}
+
+type SessionGenerator struct {
+	seq int
+	net tn.Network
+	rs  mock.RoutingServer
+}
+
+func (g *SessionGenerator) Next() Instance {
+	g.seq++
+	return session(g.net, g.rs, []byte(string(g.seq)))
+}
+
+func (g *SessionGenerator) Instances(n int) []Instance {
+	instances := make([]Instance, 0)
+	for j := 0; j < n; j++ {
+		inst := g.Next()
+		instances = append(instances, inst)
+	}
+	return instances
+}
+
+type Instance struct {
+	Peer       peer.Peer
+	Exchange   exchange.Interface
+	Blockstore blockstore.Blockstore
+}
+
+// session creates a test bitswap session.
+//
+// NB: It's easy make mistakes by providing the same peer ID to two different
+// sessions. To safeguard, use the SessionGenerator to generate sessions. It's
+// just a much better idea.
+func session(net tn.Network, rs mock.RoutingServer, id peer.ID) Instance {
+	p := peer.WithID(id)
+
+	adapter := net.Adapter(p)
+	htc := rs.Client(p)
+	bstore := blockstore.NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
+
+	const alwaysSendToPeer = true
+	ctx := context.TODO()
+
+	bs := New(ctx, p, adapter, htc, bstore, alwaysSendToPeer)
+
+	return Instance{
+		Peer:       p,
+		Exchange:   bs,
+		Blockstore: bstore,
+	}
+}