Skip to content
This repository has been archived by the owner on Aug 23, 2019. It is now read-only.

feat: add basic dial queue to avoid many connections to peer #310

Merged
merged 12 commits into from
Mar 20, 2019

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Mar 14, 2019

See the commit details, added here for ease:

BREAKING CHANGE: This adds a very basic dial queue peer peer. This will prevent multiple, simultaneous dial requests to the same peer from creating multiple connections. The requests will be queued per peer, and will leverage the same connection when possible. The breaking change here is that .dial, will no longer return a connection. js-libp2p, circuit relay, and kad-dht, which use .dial were not using the returned connection. So while this is a breaking change it should not break the existing libp2p stack. If custom applications are leveraging the returned connection, they will need to convert to only using the connection returned via the callback.

Required by libp2p/js-libp2p#336

BREAKING CHANGE: This adds a very basic dial queue peer peer.
This will prevent multiple, simultaneous dial requests to the same
peer from creating multiple connections. The requests will be queued
per peer, and will leverage the same connection when possible.
The breaking change here is that `.dial`, will no longer return a
connection. js-libp2p, circuit relay, and kad-dht, which use `.dial`
were not using the returned connection. So while this is a breaking change
it should not break the existing libp2p stack. If custom applications
are leveraging the returned connection, they will need to convert to only
using the connection returned via the callback.
@ghost ghost assigned jacobheun Mar 14, 2019
@ghost ghost added the in progress label Mar 14, 2019
@jacobheun jacobheun changed the title feat: add basic dial queue to avoid many connections to peer [WIP] feat: add basic dial queue to avoid many connections to peer Mar 14, 2019
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Overall looks good! Awesome work Jacob!

src/dialer/index.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
@jacobheun jacobheun marked this pull request as ready for review March 18, 2019 16:57
@jacobheun jacobheun requested a review from alanshaw March 18, 2019 16:57
@jacobheun jacobheun changed the title [WIP] feat: add basic dial queue to avoid many connections to peer feat: add basic dial queue to avoid many connections to peer Mar 18, 2019
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

This looks good 👍 - most of my comments are around code organisation so pretty much all suggestions really.

src/connection/handler.js Show resolved Hide resolved
@@ -366,8 +379,6 @@ class ConnectionFSM extends BaseConnection {
const conn = observeConnection(null, key, _conn, this.switch.observer)

this.muxer = this.switch.muxers[key].dialer(conn)
// this.switch.muxedConns[this.theirB58Id] = this
this.switch.connection.add(this)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this no longer added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now controlled by the dialer queue.

src/dialer/queue.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/connection/index.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/dialer/queue.js Outdated Show resolved Hide resolved
src/get-peer-info.js Show resolved Hide resolved
@jacobheun jacobheun requested a review from alanshaw March 20, 2019 14:03
@jacobheun
Copy link
Contributor Author

Aside from the _switch param changes, the rest of the feedback should be implemented.

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM

@jacobheun jacobheun merged commit 6a94d9a into master Mar 20, 2019
@ghost ghost removed the in progress label Mar 20, 2019
@jacobheun jacobheun deleted the feat/dial-queue branch March 27, 2019 11:26
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.

3 participants