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

bug: connection manager seems to have no awareness of pending connections #1898

Closed
SgtPooki opened this issue Jul 25, 2023 · 2 comments
Closed

Comments

@SgtPooki
Copy link
Member

async openConnection (peerIdOrMultiaddr: PeerId | Multiaddr | Multiaddr[], options: OpenConnectionOptions = {}): Promise<Connection> {
if (!this.isStarted()) {
throw new CodeError('Not started', codes.ERR_NODE_NOT_STARTED)
}
options.signal?.throwIfAborted()
const { peerId } = getPeerAddress(peerIdOrMultiaddr)
if (peerId != null) {
log('dial %p', peerId)
const existingConnections = this.getConnections(peerId)
if (existingConnections.length > 0) {
log('had an existing connection to %p', peerId)
return existingConnections[0]
}
}
const connection = await this.dialQueue.dial(peerIdOrMultiaddr, {
...options,
priority: options.priority ?? DEFAULT_DIAL_PRIORITY
})
let peerConnections = this.connections.get(connection.remotePeer)
if (peerConnections == null) {
peerConnections = []
this.connections.set(connection.remotePeer, peerConnections)
}
// we get notified of connections via the Upgrader emitting "connection"
// events, double check we aren't already tracking this connection before
// storing it
let trackedConnection = false
for (const conn of peerConnections) {
if (conn.id === connection.id) {
trackedConnection = true
}
}
if (!trackedConnection) {
peerConnections.push(connection)
}
return connection
}

WebTransport dial attempts take a while, and doing some testing with https://github.com/SgtPooki/helia-playground/blob/152e52846c6b3a83d0c13690030a99e3e80c3f70/src/webtransport.ts#L324-L327C9 showed that auto-dialer kept trying to dial the same address, even though the webTransport connection to the exact same URL was already attempted and is currently pending.

connection-manager should be aware of all pending outgoing dial attempts, and not be re-attempting multiaddrs that have pending dials already in flight.

related to #744 (comment)

@SgtPooki
Copy link
Member Author

maybe

if (this.queue.hasJob(peer.id)) {
log.trace('not autodialing %p because they are already being autodialed')
return false
}
isn't doing what it's supposed to?

@maschad
Copy link
Member

maschad commented Aug 28, 2023

Closed by #1943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants