Skip to content

Commit

Permalink
fix: gracefully handle remote stream closure during ping (#2822)
Browse files Browse the repository at this point in the history
Fixes #2770. This issue happens because the ping protocol does not handle the case where the remote peer closes the stream and with the introduction of `connectionMonitor` this became an issue. It opens a stream for the ping protocol, sends a ping, gets the response then closes the stream. The local peer throws an exception and triggers a yamux reset.

---------

Co-authored-by: privacyguard <[email protected]>
  • Loading branch information
achingbrain and privacyguard authored Nov 15, 2024
1 parent eee97c7 commit 4db0645
Showing 1 changed file with 23 additions and 2 deletions.
25 changes: 23 additions & 2 deletions packages/protocol-ping/src/ping.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { randomBytes } from '@libp2p/crypto'
import { ProtocolError, TimeoutError } from '@libp2p/interface'
import { ProtocolError, TimeoutError, setMaxListeners } from '@libp2p/interface'
import { byteStream } from 'it-byte-stream'
import { equals as uint8ArrayEquals } from 'uint8arrays/equals'
import { PROTOCOL_PREFIX, PROTOCOL_NAME, PING_LENGTH, PROTOCOL_VERSION, TIMEOUT, MAX_INBOUND_STREAMS, MAX_OUTBOUND_STREAMS } from './constants.js'
Expand Down Expand Up @@ -60,10 +60,12 @@ export class PingService implements Startable, PingServiceInterface {
const { stream } = data
const start = Date.now()
const bytes = byteStream(stream)
let pinged = false

Promise.resolve().then(async () => {
while (true) {
const signal = AbortSignal.timeout(this.timeout)
setMaxListeners(Infinity, signal)
signal.addEventListener('abort', () => {
stream?.abort(new TimeoutError('ping timeout'))
})
Expand All @@ -74,15 +76,34 @@ export class PingService implements Startable, PingServiceInterface {
await bytes.write(buf, {
signal
})

pinged = true
}
})
.catch(err => {
this.log.error('incoming ping from %p failed with error', data.connection.remotePeer, err)
// ignore the error if we've processed at least one ping, the remote
// closed the stream and we handled or are handling the close cleanly
if (pinged && err.name === 'UnexpectedEOFError' && stream.readStatus !== 'ready') {
return
}

this.log.error('incoming ping from %p failed with error - %e', data.connection.remotePeer, err)
stream?.abort(err)
})
.finally(() => {
const ms = Date.now() - start
this.log('incoming ping from %p complete in %dms', data.connection.remotePeer, ms)

const signal = AbortSignal.timeout(this.timeout)
setMaxListeners(Infinity, signal)

stream.close({
signal
})
.catch(err => {
this.log.error('error closing ping stream from %p - %e', data.connection.remotePeer, err)
stream?.abort(err)
})
})
}

Expand Down

0 comments on commit 4db0645

Please sign in to comment.