Fix: don't ignore received blocks for pending wants

[dirkmc]

Fixes #172

[dirkmc]

One concern with this approach is that we ask each session in turn if it is interested in the CID. If there are N sessions, then (N - 1) / N times the session will not have the CID. As noted in the comment in the code in session.go:149, this may cause performance issues:

go-bitswap/session/session.go

Lines 144 to 172 in e72b289

	// InterestedIn returns true if this session is interested in the given Cid.
	func (s *Session) InterestedIn(c cid.Cid) bool {
	if s.interest.Contains(c) {
	return true
	}
	// TODO: PERF: this is using a channel to guard a map access against race
	// conditions. This is definitely much slower than a mutex, though its unclear
	// if it will actually induce any noticeable slowness. This is implemented this
	// way to avoid adding a more complex set of mutexes around the liveWants map.
	// note that in the average case (where this session *is* interested in the
	// block we received) this function will not be called, as the cid will likely
	// still be in the interest cache.
	resp := make(chan bool, 1)
	select {
	case s.interestReqs <- interestReq{
	c: c,
	resp: resp,
	}:
	case <-s.ctx.Done():
	return false
	}
	select {
	case want := <-resp:
	return want
	case <-s.ctx.Done():
	return false
	}
	}

One option would be to simply replace the s.interest LRU with a map[cid.Cid]struct{} (guarded by a lock)

[Stebalien]

One option would be to simply replace the s.interest LRU with a map[cid.Cid]struct{} (guarded by a lock).

We're going to have to check this anyways when we figure out where to send the blocks. However, it's slightly worse in this case as we'll need to call this on duplicates as well.

I'd like to drop the interest cache and provide a way to check this without using channels anyways so I'm all for fixing this.

To handle, the many sessions issue, we could even have a second, global & reference counted interest set.

I believe we'll still have races where we'll receive multiple blocks quickly before we record that we've received them but I believe that's a problem anyways.

[dirkmc]

I guess the most important property is that if session.GetBlocks() has ever been called on a session, then when a corresponding block is received, we should always make sure the session receives the block.

The session stores all the CIDs it has ever been asked for in liveWants, tofetch and pastWants, so we shouldn't actually need a separate set to determine if the session is interested.

We just need to make sure we take a lock each time we manipulate or query any of those sets of CIDs.

[dirkmc]

I created a PR #184 that branches off this PR, to explore refactoring want management into a separate class, so that it's easier to put locks around the want queues. With locks in place we can avoid going through Session's event loop when calling InterestedIn() and IsWanted().

EDIT: I merged the want management PR into this PR. Now it just puts want management behind locks

[dirkmc]

Tests are failing because of some flakiness that is fixed in #185

[Stebalien]

Nits you can ignore (or not). But this really does feel simpler

[Stebalien]

My only real comment is on the goroutines. I'd leave those out (unless you feel that there's a strong need for them).

[Stebalien]

Hm. I'm not sure of a better way to do this but this is a bit funky. But it's probably fine.