Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Refactor SessionInterestManager #384

Merged
merged 2 commits into from
May 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 106 additions & 21 deletions internal/sessioninterestmanager/sessioninterestmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,29 @@ package sessioninterestmanager
import (
"sync"

bsswl "github.com/ipfs/go-bitswap/internal/sessionwantlist"
blocks "github.com/ipfs/go-block-format"

cid "github.com/ipfs/go-cid"
)

// SessionInterestManager records the CIDs that each session is interested in.
type SessionInterestManager struct {
lk sync.RWMutex
interested *bsswl.SessionWantlist
wanted *bsswl.SessionWantlist
lk sync.RWMutex
wants map[cid.Cid]map[uint64]bool
}

// New initializes a new SessionInterestManager.
func New() *SessionInterestManager {
return &SessionInterestManager{
interested: bsswl.NewSessionWantlist(),
wanted: bsswl.NewSessionWantlist(),
// Map of cids -> sessions -> bool
//
// The boolean indicates whether the session still wants the block
// or is just interested in receiving messages about it.
//
// Note that once the block is received the session no longer wants
// the block, but still wants to receive messages from peers who have
// the block as they may have other blocks the session is interested in.
wants: make(map[cid.Cid]map[uint64]bool),
}
}

Expand All @@ -30,8 +35,15 @@ func (sim *SessionInterestManager) RecordSessionInterest(ses uint64, ks []cid.Ci
sim.lk.Lock()
defer sim.lk.Unlock()

sim.interested.Add(ks, ses)
sim.wanted.Add(ks, ses)
// For each key
for _, c := range ks {
// Record that the session wants the blocks
if want, ok := sim.wants[c]; ok {
want[ses] = true
} else {
sim.wants[c] = map[uint64]bool{ses: true}
}
}
}

// When the session shuts down it calls RemoveSessionInterest().
Expand All @@ -40,26 +52,68 @@ func (sim *SessionInterestManager) RemoveSession(ses uint64) []cid.Cid {
sim.lk.Lock()
defer sim.lk.Unlock()

sim.wanted.RemoveSession(ses)
return sim.interested.RemoveSession(ses)
// The keys that no session is interested in
deletedKs := make([]cid.Cid, 0)

// For each known key
for c := range sim.wants {
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
// Remove the session from the list of sessions that want the key
delete(sim.wants[c], ses)

// If there are no more sessions that want the key
if len(sim.wants[c]) == 0 {
// Clean up the list memory
delete(sim.wants, c)
// Add the key to the list of keys that no session is interested in
deletedKs = append(deletedKs, c)
}
}

return deletedKs
}

// When the session receives blocks, it calls RemoveSessionWants().
func (sim *SessionInterestManager) RemoveSessionWants(ses uint64, wants []cid.Cid) {
func (sim *SessionInterestManager) RemoveSessionWants(ses uint64, ks []cid.Cid) {
sim.lk.Lock()
defer sim.lk.Unlock()

sim.wanted.RemoveSessionKeys(ses, wants)
// For each key
for _, c := range ks {
// If the session wanted the block
if wanted, ok := sim.wants[c][ses]; ok && wanted {
// Mark the block as unwanted
sim.wants[c][ses] = false
}
}
}

// When a request is cancelled, the session calls RemoveSessionInterested().
// Returns the keys that no session is interested in any more.
func (sim *SessionInterestManager) RemoveSessionInterested(ses uint64, wants []cid.Cid) []cid.Cid {
func (sim *SessionInterestManager) RemoveSessionInterested(ses uint64, ks []cid.Cid) []cid.Cid {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something here is very strange. I think the meaning of "session interest" has changed over time. In the past, I believe it included everything the session wanted, but hadn't necessarily asked for.

I prefer the new interpretation "things the session is interested in hearing about", but I think we've made a mistake: we're canceling blocks when we become uninterested in them.

Shouldn't we be canceling blocks when they become unwanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true it's a little convoluted. We actually want to do both:

  • When the client calls Session.GetBlocks() the session registers interest in those blocks
  • When we receive blocks / HAVEs / DONT_HAVEs we want to know which sessions are interested in them
  • When the session receives a block it no longer wants the block, and sends a cancel.
    Note that the session is still interested in the block (but doesn't want the block), because the session wants to add peers who had the block to the session, so as to query them for other blocks that the session wants
  • When the client cancels Session.GetBlocks() the session is no longer interested in those blocks, and should also send a cancel for them
  • When the session shuts down, it is no longer interested in the remaining blocks and sends cancels for them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that this function returns CIDs (for cancellation) that no sessions are interested in. However, we really want to return CIDs that no session wants.

I think we're technically fine because:

  1. We cancel blocks when we receive them.
  2. We'll lose interest in blocks when GetBlocks is canceled.

But it's still a bit strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're saying, it's a little strange that we don't just cancel something that no one wants.

When we receive a block we send a cancel to the peer, and mark it as unwanted in the SessionInterestManager. We don't remove it entirely from the SessionInterestManager because if other peers subsequently respond with a HAVE or a block, we want to be able to tell the Session so that it can expand its pool of peers who may have blocks its interested in (this is particularly important in the discovery phase).

On the other hand if the request is cancelled, then the Session is not interested at all in hearing about subsequent blocks or HAVEs for the CIDs in the request, so in that case we remove the CIDs entirely from the SessionInterestManager.

Maybe the naming could be a clearer, I'm not sure that wanted / interested fully captures the concepts.

sim.lk.Lock()
defer sim.lk.Unlock()

sim.wanted.RemoveSessionKeys(ses, wants)
return sim.interested.RemoveSessionKeys(ses, wants)
// The keys that no session is interested in
deletedKs := make([]cid.Cid, 0, len(ks))

// For each key
for _, c := range ks {
// If there is a list of sessions that want the key
if _, ok := sim.wants[c]; ok {
// Remove the session from the list of sessions that want the key
delete(sim.wants[c], ses)

// If there are no more sessions that want the key
if len(sim.wants[c]) == 0 {
// Clean up the list memory
delete(sim.wants, c)
// Add the key to the list of keys that no session is interested in
deletedKs = append(deletedKs, c)
}
}
}

return deletedKs
}

// The session calls FilterSessionInterested() to filter the sets of keys for
Expand All @@ -68,9 +122,20 @@ func (sim *SessionInterestManager) FilterSessionInterested(ses uint64, ksets ...
sim.lk.RLock()
defer sim.lk.RUnlock()

// For each set of keys
kres := make([][]cid.Cid, len(ksets))
for i, ks := range ksets {
kres[i] = sim.interested.SessionHas(ses, ks).Keys()
// The set of keys that at least one session is interested in
has := make([]cid.Cid, 0, len(ks))

// For each key in the list
for _, c := range ks {
// If there is a session that's interested, add the key to the set
if _, ok := sim.wants[c][ses]; ok {
has = append(has, c)
}
}
kres[i] = has
}
return kres
}
Expand All @@ -81,12 +146,19 @@ func (sim *SessionInterestManager) SplitWantedUnwanted(blks []blocks.Block) ([]b
sim.lk.RLock()
defer sim.lk.RUnlock()

// Get the wanted block keys
ks := make([]cid.Cid, len(blks))
// Get the wanted block keys as a set
wantedKs := cid.NewSet()
for _, b := range blks {
ks = append(ks, b.Cid())
c := b.Cid()
// For each session that is interested in the key
for ses := range sim.wants[c] {
// If the session wants the key (rather than just being interested)
if wanted, ok := sim.wants[c][ses]; ok && wanted {
// Add the key to the set
wantedKs.Add(c)
}
}
}
wantedKs := sim.wanted.Has(ks)

// Separate the blocks into wanted and unwanted
wantedBlks := make([]blocks.Block, 0, len(blks))
Expand All @@ -112,5 +184,18 @@ func (sim *SessionInterestManager) InterestedSessions(blks []cid.Cid, haves []ci
ks = append(ks, haves...)
ks = append(ks, dontHaves...)

return sim.interested.SessionsFor(ks)
// Create a set of sessions that are interested in the keys
sesSet := make(map[uint64]struct{})
for _, c := range ks {
for s := range sim.wants[c] {
sesSet[s] = struct{}{}
}
}

// Convert the set into a list
ses := make([]uint64, 0, len(sesSet))
for s := range sesSet {
ses = append(ses, s)
}
return ses
}
141 changes: 0 additions & 141 deletions internal/sessionwantlist/sessionwantlist.go

This file was deleted.

Loading