Skip to content

fix(libp2p): default abortConnectionOnPingFailure to false #3463

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.

Sign up for GitHub

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

Open
marshalleq wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −5
Open

fix(libp2p): default abortConnectionOnPingFailure to false #3463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions packages/libp2p/src/connection-monitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,14 @@ const PROTOCOL_VERSION = '1.0.0'
const PROTOCOL_NAME = 'ping'
const PROTOCOL_PREFIX = 'ipfs'
const PING_LENGTH = 32
const DEFAULT_ABORT_CONNECTION_ON_PING_FAILURE = true
// Probe failures are not reliable evidence a connection is dead under real
// WAN conditions — trans-continental RTT, event-loop contention, and transient
// network jitter routinely cause a single 10s ping exchange to fail. Yamux
// keepalive detects genuinely dead connections at the muxer layer without
// tearing down the TCP socket, so aborting on a single probe failure is
// measurable self-harm at non-trivial scale. Applications that want the
// aggressive behaviour can still opt in by setting this to true.
const DEFAULT_ABORT_CONNECTION_ON_PING_FAILURE = false

export interface ConnectionMonitorInit {
/**
Expand All @@ -38,9 +45,12 @@ export interface ConnectionMonitorInit {
pingTimeout?: Omit<AdaptiveTimeoutInit, 'metricsName' | 'metrics'>

/**
* If true, any connection that fails the ping will be aborted
* If true, any connection that fails the ping will be aborted. Defaults to
* false because a single 10s probe failure is not reliable evidence that
* the connection is dead under real WAN conditions. Set to true to restore
* the previous aggressive behaviour.
*
* @default true
* @default false
*/
abortConnectionOnPingFailure?: boolean

Expand Down
25 changes: 23 additions & 2 deletions packages/libp2p/test/connection-monitor/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ describe('connection monitor', () => {
pingInterval: 50,
pingTimeout: {
maxTimeout: 50
}
},
abortConnectionOnPingFailure: true
})

await start(monitor)
Expand All @@ -133,7 +134,8 @@ describe('connection monitor', () => {

it('should abort a connection that fails', async () => {
monitor = new ConnectionMonitor(components, {
pingInterval: 10
pingInterval: 10,
abortConnectionOnPingFailure: true
})

await start(monitor)
Expand All @@ -150,6 +152,25 @@ describe('connection monitor', () => {
expect(connection.abort).to.have.property('called', true)
})

it('should not abort a connection that fails by default', async () => {
monitor = new ConnectionMonitor(components, {
pingInterval: 10
})

await start(monitor)

const connection = stubInterface<Connection>()
connection.newStream.withArgs('/ipfs/ping/1.0.0').callsFake(async (protocols, opts) => {
throw new ConnectionClosedError('Connection closed')
})

components.connectionManager.getConnections.returns([connection])

await delay(500)

expect(connection.abort).to.have.property('called', false)
})

it('should not abort a connection that fails when abortConnectionOnPingFailure is false', async () => {
monitor = new ConnectionMonitor(components, {
pingInterval: 10,
Expand Down