add blocks to the blockstore before returning them from blockservice sessions. #4169
Conversation
…sessions. fixes #4062 (yay!) License: MIT Signed-off-by: Steven Allen <[email protected]>
| k := blk.Cid() | ||
| ks := []*cid.Cid{k} | ||
| for _, s := range bs.SessionsForBlock(k) { | ||
| s.receiveBlockFrom(from, blk) |
There was a problem hiding this comment.
hrm... the change from "" to from here is on purpose? Whats the reasoning? ( i dont remember all the implications without my coffee)
There was a problem hiding this comment.
I merged two functions. from will be "" when the user calls HasBlock but will be the peer from which we received the block when called from within bitswap.
Basically, we had two functions, HasBlock and a go routine inside ReceiveMessage where:
- The second function would:
- Notify all interested sessions that we had received the block.
- Remove the block from sessoins' wantlists.
- Call the first function which would:
- Put the block in the blockstore.
- Publish a notification on
bs.notifications. - Notify the session that we received the block (again) but claiming to have received it from "". This was to prevent a race condition where we'd notify the session that we received the block before putting it in the blockstore and then someone would try to get the block from the blockstore, not find it, and ask bitswap for it. I believe we should also have removed this block from the session but I'm not sure why we didn't (the new code does).
- Record the block in the bitswap engine.
- Reprovide the block.
I got rid of the first function, moved step 3 into step 7, and made it notify the session with the correct peer in step 7 if we received the block from the network.
The bug was that race condition in step 7. The fix works iff you keep the same bitswap instance but, if you replace the bitswap instance with a new one, the caller will not find the block in the blockstore and ask the new bitswap instance for the block (which doesn't know about it).
There was a problem hiding this comment.
okay, that sounds good. Any way you could write a test?
There was a problem hiding this comment.
TestFetchGraph? 😄 It's a bit hard to test a race condition...
There was a problem hiding this comment.
(this is fixing a test failure so there is, by definition, a test case).
fixes #4062 (yay!)
License: MIT
Signed-off-by: Steven Allen [email protected]