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

Initial wantlist handling is racy #51

Closed
Stebalien opened this issue Dec 29, 2018 · 3 comments · Fixed by #72
Closed

Initial wantlist handling is racy #51

Stebalien opened this issue Dec 29, 2018 · 3 comments · Fixed by #72
Assignees
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@Stebalien
Copy link
Member

The race is as follows:

  1. Start processing the "Connect" event.
  2. Read the current broadcast wantlist in:

    go-bitswap/bitswap.go

    Lines 438 to 444 in 916de59

    // Connected/Disconnected warns bitswap about peer connections.
    func (bs *Bitswap) PeerConnected(p peer.ID) {
    initialWants := bs.wm.CurrentBroadcastWants()
    bs.pm.Connected(p, initialWants)
    bs.engine.PeerConnected(p)
    }
  3. Add a new wantlist entry.
  4. Start the peer's message handler:
    func (pm *PeerManager) startPeerHandler(p peer.ID, initialEntries []*wantlist.Entry) PeerQueue {
    mq, ok := pm.peerQueues[p]
    if ok {
    mq.RefIncrement()
    return nil
    }
    mq = pm.createPeerQueue(p)
    pm.peerQueues[p] = mq
    mq.Startup(pm.ctx, initialEntries)
    return mq
    }

In this last function, initialEntries will be missing the wantlist entry from step 3.

@Stebalien Stebalien added the kind/bug A bug in existing code (including security flaws) label Dec 29, 2018
@hannahhoward
Copy link
Contributor

I was specifically trying to avoid two way coupling between the WantManager and the PeerManager, since the WantManager already has a reference to the PeerManager to send messages.

If we're going this route, I'd like to avoid that -- perhaps through SetDelegate similar to the one on bitswap Network

@hannahhoward hannahhoward self-assigned this Jan 10, 2019
@Stebalien
Copy link
Member Author

What if we:

  1. Lazily created message senders on demand.
  2. Instead of notifying the PeerManager of new connections, notify the WantManager. The WantManager would then initiate the MessageSender by sending the initial wantlist to that peer.

hannahhoward added a commit that referenced this issue Feb 19, 2019
fix race conditions while setting up wantlists by creating peer queues on demand

BREAKING CHANGE: PeerManager SendMessage signature changed

fix #51
@ghost ghost added the status/in-progress In progress label Feb 19, 2019
@Stebalien
Copy link
Member Author

I believe this can also cause us to not cancel a want for a block.

hannahhoward added a commit that referenced this issue Feb 20, 2019
fix race conditions while setting up wantlists by creating peer queues on demand

BREAKING CHANGE: PeerManager SendMessage signature changed

fix #51
@ghost ghost removed the status/in-progress In progress label Feb 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants