Skip to content

Commit

Permalink
fix: drop connection when stream ends unexpectedly
Browse files Browse the repository at this point in the history
Pull streams pass true in the error position when the sream ends.
In https://github.com/multiformats/js-multistream-select/blob/5b19358b91850b528b3f93babd60d63ddcf56a99/src/select.js#L18-L21
...we're getting lots of instances of pull-length-prefixed stream
erroring early with `true` and it's passed back up to the dialer
in https://github.com/libp2p/js-libp2p-switch/blob/fef2d11850379a4720bb9c736236a81a067dc901/src/dial.js#L238-L241

The `_createMuxedConnection` contains an assumption that any error
that occurs when trying `_attemptMuxerUpgrade` is ok, and keeps the
relveant baseConnecton in the cache. If the pull-stream has ended
unexpectedly then keeping the connection arround starts causing
the "already piped" errors when we try and use the it later.

This PR adds a guard to avoid putting the connection back into the
cache if the stream has ended.

There is related work in an old PR to add a check for exactly this issue in
pull-length-prefixed dignifiedquire/pull-length-prefixed#8
...but it's still open, so this PR adds a check for true in
the error position at the site where the "already piped" errors
were appearing. Once the PR on pull-length-prefixed is merged this
check can be removed. It's not ideal to have it in this code as it
is far removed from the source, but it fixes the issue for now.

Arguably anywhere that `msDialer.handle` is called should do the
same check, but we're not seeing this error occur anywhere else so
to keep this PR small, I've left it as the minimal changeset to
fix the issue.

Of note, we had to add '/dns4/ws-star.discovery.libp2p.io/tcp/443/wss/p2p-websocket-star'
to the swarm config to trigger the "already piped" errors. There
is a minimal test app here https://github.com/tableflip/js-ipfs-already-piped-error

Manual testing shows ~50 streams fail in the first 2 mins of
running a node, and then things stabalise with ~90 active muxed
connections after that.

Fixes libp2p#235
Fixes ipfs/js-ipfs#1366
See dignifiedquire/pull-length-prefixed#8

License: MIT
Signed-off-by: Oli Evans <[email protected]>
  • Loading branch information
olizilla authored and Jacob Heun committed May 31, 2018
1 parent fef2d11 commit cb5245a
Showing 1 changed file with 12 additions and 5 deletions.
17 changes: 12 additions & 5 deletions src/dial.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,14 @@ class Dialer {
}

connection.setPeerInfo(this.peerInfo)

waterfall([
(cb) => {
this._attemptMuxerUpgrade(connection, b58Id, cb)
this._attemptMuxerUpgrade(connection, b58Id, (err, muxer) => {
// The underlying stream closed unexpectedly, so drop the connection.
// Fixes https://github.com/libp2p/js-libp2p-switch/issues/235
if (err && err.message === 'Unexpected end of input from reader.') {
log('Connection dropped for %s', b58Id)
return callback(null, null)
}
], (err, muxer) => {

if (err && !this.protocol) {
this.switch.conns[b58Id] = connection
return callback(null, null)
Expand Down Expand Up @@ -237,6 +239,11 @@ class Dialer {
const msDialer = new multistream.Dialer()
msDialer.handle(connection, (err) => {
if (err) {
// Repackage errors from pull-streams ending unexpectedly.
// Needed until https://github.com/dignifiedquire/pull-length-prefixed/pull/8 is merged.
if (err === true) {
return callback(new Error('Unexpected end of input from reader.'))
}
return callback(new Error('multistream not supported'))
}

Expand Down

0 comments on commit cb5245a

Please sign in to comment.