This repository was archived by the owner on Sep 8, 2025. It is now read-only.
p2p: A couple fixes to KademliaProtocol/PeerPool #767
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pipermerriam
suggested changes
May 24, 2018
p2p/peer.py
Outdated
| if self._discovery_lookup_running: | ||
| self.logger.debug("Node discovery lookup already in progress, not running another") | ||
| return | ||
| self._discovery_lookup_running = True |
There was a problem hiding this comment.
Can we use an asyncio.Lock for this instead of a true/false? They support use as a context manager which allows for a cleaner API here.
There was a problem hiding this comment.
Definitely! Should've thought of that myself
p2p/peer.py
Outdated
| async def maybe_lookup_random_node(self) -> None: | ||
| if self._discovery_last_lookup + self._discovery_lookup_interval > time.time(): | ||
| return | ||
| if self._discovery_lookup_running: |
There was a problem hiding this comment.
nitpick
Can you make this an elif
gsalgado
force-pushed
the
peer-pool-discovery-fixes
branch
from
de5ee62 to
e34496c
Compare
p2p/peer.py
Outdated
| elif self._discovery_lookup_running.locked(): | ||
| self.logger.debug("Node discovery lookup already in progress, not running another") | ||
| return | ||
| with (await self._discovery_lookup_running): |
There was a problem hiding this comment.
is this equvalent to async with self.discovery_lookup_running?
That's what I found here: https://www.pythonsheets.com/notes/python-asyncio.html
There was a problem hiding this comment.
Looks like it: https://www.python.org/dev/peps/pep-0492/#asynchronous-context-managers-and-async-with
I'll switch to async with; I think that reads better
pipermerriam
approved these changes
May 24, 2018
- Ensure PeerPool never runs multiple node discovery lookups in parallel - KademliaProtocol.bond() no longer triggers self.populate_not_full_buckets() when bondig fails as that can potentially create a never-ending bonding spiral
gsalgado
force-pushed
the
peer-pool-discovery-fixes
branch
from
e34496c to
2d60366
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
self.populate_not_full_buckets() when bondig fails as that can
potentially create a never-ending bonding spiral
The
AlreadyWaitingerrors (#737) seem to be gone with these changes