bitswap: long-running session leaks

[Wondertan]

We've been running long-running Bitswap sessions and encountered a series of memory leaks that we've successfully identified and fixed in our fork. I plan to start upstreaming them eventually, but I am currently botted with millions of things. Anyway, I want to make you aware of their existence already in case you have the capacity to look into those.

The full branch is located here. Some of them were merged, some of them aren't. The most notable are:

• fix leak ensuring SessionInterestManager is cleaned up
• fix leak in block presence manager along with
• cleanup toFetch cidqueue
• cleanup peerresponsetracker
• rebroadcast only when there are live CIDs - not a leak, resolves issue with timer continue ticking even when there are not CIDs to rebroadcast further

All of those fixes have been tested on hundreds of nodes in production, and things have been running smoothly and steadily for 3 weeks. I am happy to answer if you start working on this sooner than when I get to opening PRs myself.

cc @hsanjuan, @gammazero, @lidel

[gammazero]

I will create separate PR for each of these and review and merge these into our code base, unless you would rather do this from your fork. We are also resolving memory consumption issues with connections, so it would be great to get all the known memory issues handled in at the same time. So let me know soon.

Thank you.

[gammazero]

The first two fixes appear to be incorrect according to the comment in SessionInterestManager:

		// 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.

@Wondertan Please let me know if this needs further investigation.

[gammazero]

See the following PRs:

• Fix leak due to cid queue never getting cleaned up #756
• Remove peer's count of first responses when peer becomes unavailable #757
• Do not reset the broadcast timer if there are no wants #758
• Remove want-block sent tracking from sessionWantSender #759

[github-actions]

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

[Wondertan]

Hey @gammazero, thank you for submitting the fixes.

The first two fixes appear to be incorrect according to the comment in SessionInterestManager:

Re above. The first commit cleans up the SessionInterestManager, otherwise its only cleaned when the session is closed. I don't think that this behavior is correct, even though it might be expected, as per comment you referenced. However, I also don't see my fix contradict to the comment. It says "but still wants to receive messages from peers who have the block as they may have other blocks the session is interested in". Why would cleaning up SessionInterestManager prevent receiving new messages from the peers? It also states other blocks, while we clean particular blocks.

[Wondertan]

6cd1ed9 goes further and cleans up BlockPresenceManager, but specifically for CIDs that are not in interest for any other session. There is no reason to keep CID presence information from peers when the CID has already been processed and no other session needs it.

[Wondertan]

And 70a6503 is pretty severe bug independently of long-running sessions. Check the comment there for description. Besides, one could blow up a peer by sending presences of completely arbitrary CIDs, because Bitswap never checks if those CIDs presences are even relevant(in SIM) and directly stores them. This commit fixes that

[Wondertan]

@gammazero, gentle ping here

[guillaumemichel]

Concerning 6cd1ed9, I agree with @gammazero, removing the cids from the interest list, rather than just the want list would have side effects.

boxo/bitswap/client/internal/sessioninterestmanager/sessioninterestmanager.go

Lines 119 to 142 in 3aa3bee

	// The session calls FilterSessionInterested() to filter the sets of keys for
	// those that the session is interested in
	func (sim *SessionInterestManager) FilterSessionInterested(ses uint64, ksets ...[]cid.Cid) [][]cid.Cid {
	kres := make([][]cid.Cid, len(ksets))
	sim.lk.RLock()
	defer sim.lk.RUnlock()
	// For each set of keys
	for i, ks := range ksets {
	// The set of keys that at least one session is interested in
	var has []cid.Cid
	// For each key in the list
	for _, c := range ks {
	// If the session is interested, add the key to the set
	if _, ok := sim.wants[c][ses]; ok {
	has = append(has, c)
	}
	}
	kres[i] = has
	}
	return kres
	}

This function computes the intersection between the provided cids, and the interest list, not the want list. So removing cids from the interest list instead of the want list would have undesired side effects IIUC.

boxo/bitswap/client/internal/sessioninterestmanager/sessioninterestmanager.go

Lines 178 to 202 in 3aa3bee

	// When the SessionManager receives a message it calls InterestedSessions() to
	// find out which sessions are interested in the message.
	func (sim *SessionInterestManager) InterestedSessions(keySets ...[]cid.Cid) []uint64 {
	sesSet := make(map[uint64]struct{})
	sim.lk.RLock()
	// Create a set of sessions that are interested in the keys
	for _, keySet := range keySets {
	for _, c := range keySet {
	for s := range sim.wants[c] {
	sesSet[s] = struct{}{}
	}
	}
	}
	sim.lk.RUnlock()
	// Convert the set into a list
	ses := make([]uint64, 0, len(sesSet))
	for s := range sesSet {
	ses = append(ses, s)
	}
	return ses
	}

This function lists all the sessions that are interested in this block. Hence, he the session doesn't WANT the block but is INTERESTED in it, the session id should be returned.

IIUC it is a feature that the cids remain in the interest list as long as the session is alive. So changing this would modify the Bitswap client behaviour.

[Wondertan]

undesired side effects IIUC.

@guillaumemichel, can you give an example of a side-effect? I couldn't find of one, but maybe there is

[guillaumemichel]

@Wondertan see #821

[guillaumemichel]

With #833, all mentioned leaks should be addressed @Wondertan.

Closing this issue, feel free to reopen if something is missing.