Skip to content

Commit

Permalink
fix: close circuit-relay streams on connection failure (#1758)
Browse files Browse the repository at this point in the history
If a dial to a relayed address fails, e.g. with status `NO_RESERVATION`, the newly created `/libp2p/circuit/relay/0.2.0/hop` stream is **never closed**. This can be particularly bad if the connection manager keeps auto-dialing the address, since the streams just accumulate until they hit the max per-protocol stream limit (64). I believe this is the ultimate issue behind [#1695](#1695).

---------

Co-authored-by: achingbrain <alex@achingbrain.net>
  • Loading branch information
joeltg and achingbrain committed May 18, 2023
1 parent e93d258 commit 1af7808
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/circuit-relay/transport/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,10 @@ class CircuitRelayTransport implements Transport {
disconnectOnFailure = true
}

let stream: Stream | undefined

try {
const stream = await relayConnection.newStream([RELAY_V2_HOP_CODEC])
stream = await relayConnection.newStream([RELAY_V2_HOP_CODEC])

return await this.connectV2({
stream,
Expand All @@ -197,6 +199,11 @@ class CircuitRelayTransport implements Transport {
})
} catch (err: any) {
log.error(`Circuit relay dial to destination ${destinationPeer.toString()} via relay ${relayPeer.toString()} failed`, err)

if (stream != null) {
stream.abort(err)
}

disconnectOnFailure && await relayConnection.close()
throw err
}
Expand Down
24 changes: 24 additions & 0 deletions test/circuit-relay/relay.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,30 @@ describe('circuit-relay', () => {
expect(conns).to.have.nested.property('[0].stat.status', 'OPEN')
})

it('dialer should close hop stream on hop failure', async () => {
await local.dial(relay1.getMultiaddrs()[0])

// dial the remote through the relay
const relayedMultiaddr = relay1.getMultiaddrs()[0].encapsulate(`/p2p-circuit/p2p/${remote.peerId.toString()}`)

await expect(local.dial(relayedMultiaddr))
.to.eventually.be.rejected()
.and.to.have.property('message').that.matches(/NO_RESERVATION/)

// we should still be connected to the relay
const conns = local.getConnections(relay1.peerId)
expect(conns).to.have.lengthOf(1)
expect(conns).to.have.nested.property('[0].stat.status', 'OPEN')

// we should not have any streams with the hop codec
const streams = local.getConnections(relay1.peerId)
.map(conn => conn.streams)
.flat()
.filter(stream => stream.stat.protocol === RELAY_V2_HOP_CODEC)

expect(streams).to.be.empty()
})

it('destination peer should stay connected to an already connected relay on hop failure', async () => {
await local.dial(relay1.getMultiaddrs()[0])
await usingAsRelay(local, relay1)
Expand Down

0 comments on commit 1af7808

Please sign in to comment.