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

Ensure broadcast when remaining peer becomes unavailable #272

Merged
merged 6 commits into from
Mar 2, 2020

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Feb 28, 2020

Fixes #259

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 28, 2020

The test failure (panic) in CI might be related - we're now telling it to broadcast when the last peer in a session disconnects, so that might be having some after-effects. I will dig in.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Is there any way to reduce the state a bit? Ideally, we'd:

  1. Process updates.
  2. Look at the current state and react to it.

(not blocking, I'm just trying to simplify bitswap a bit)

internal/session/sessionwantsender.go Show resolved Hide resolved
internal/session/sessionwantsender.go Show resolved Hide resolved
internal/session/sessionwantsender.go Show resolved Hide resolved
@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 28, 2020

Yeah I agree, there's a lot going on in this class. I've tried to break out bits of it into different classes (eg peerAvailabilityManager) but it still feels too complicated to keep track of everything.

Maybe we can break out the DONT_HAVE tracking into two classes:

  • track consecutive DONT_HAVEs (and eject peers that get too many in a row)
  • track want exhaustion (every peer sent a DONT_HAVE for a particular want)

@Stebalien
Copy link
Member

I'm more concerned about threading updates around than having a lot of state. Really, having related state all in one place makes it easier for me to keep track of it.

It's not really an issue with this patch, just something that's creeping up on us.

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 28, 2020

It turns out the problem was with the timing of a test that would verify the session will call FindProviders() after being disconnected from a peer that had the blocks. I changed the logic of the tests to not rely on timing.

While I was doing so I realized that when the session receives "exhausted" wants (wants for whom all peers have sent a DONT_HAVE) it would broadcast a request for all pending wants, instead of just those specific exhausted wants. So I also fixed that issue.

internal/session/sessionwantsender.go Outdated Show resolved Hide resolved
@dirkmc dirkmc merged commit 0ba089b into master Mar 2, 2020
@dirkmc dirkmc deleted the fix/sess-brdcst branch March 2, 2020 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session should broadcast when all peers are removed
2 participants