Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConnectionManager events #675

Closed
vasco-santos opened this issue Jun 17, 2020 · 3 comments
Closed

ConnectionManager events #675

vasco-santos opened this issue Jun 17, 2020 · 3 comments
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked

Comments

@vasco-santos
Copy link
Member

Creating this per discussion on ChainSafe/js-libp2p-gossipsub#87 (comment)

There are currently a few inconsistencies with the ConnectionManager events that need to be solved:

it seems that there are some problems with our current approach of updating IPs, and it seems the events currently emitted by the connection manager are not sufficient for tracking IPs to open connections for peers with scores.
issue 1: ConnectionManager doesn't emit on closed connections. ConnectionManager#_maybeDisconnectOne, doesn't emit when a connection is closed, more generally, ConnectionManager isn't tracking/emitting on manually closed connections, but rather only on calls to ConnectionManager#onDisconnect.
issue 2: ConnectionManager emits peer:connect event before storing the emitted connection.
The current approach is to use ConnectionManager#getAll on each peer:connect event for a list of current IPs, but since the event is triggered before the new connection is stored, the call to getAll will not include the new connection unless the event handler awaits the ConnectionManager#onConnect finishing in its entirety. But that potentially also runs into issues re issue 1.

cc @wemeetagain

@vasco-santos
Copy link
Member Author

Added this to the #655 Epic issue for 0.30

@jacobheun jacobheun added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked labels Jun 18, 2020
@vasco-santos
Copy link
Member Author

This should be part of #744 and will be moved to 0.31

@wemeetagain
Copy link
Member

ConnectionManager has since been refactored. Closing as stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

3 participants