Refactor SessionInterestManager #384
Conversation
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- We cancel blocks when we receive them.
- We'll lose interest in blocks when GetBlocks is canceled.
But it's still a bit strange.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
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:
- We cancel blocks when we receive them.
- We'll lose interest in blocks when GetBlocks is canceled.
But it's still a bit strange.
* fix: send cancel when GetBlocks() is cancelled * fix: in SessionManager shutdown nil out sessions * fix: sessionWantSender perf * make sessionWantSender.SignalAvailability() non-blocking * Refactor SessionInterestManager (#384) * refactor: customize SessionInterestManager * refactor: SessionInterestManager perf
* fix: use one less go-routine per session * fix: send cancel when GetBlocks() is cancelled (#383) * fix: send cancel when GetBlocks() is cancelled * fix: in SessionManager shutdown nil out sessions * fix: sessionWantSender perf * make sessionWantSender.SignalAvailability() non-blocking * Refactor SessionInterestManager (#384) * refactor: customize SessionInterestManager * refactor: SessionInterestManager perf
Depends on #383
We no longer need the SessionWantList (it used to be used by the WantManager) so we can get rid of it and consolidate functionality into SessionInterestManager.
This also removes a bunch of unnecessary locking.