Skip to content
This repository was archived by the owner on Aug 23, 2019. It is now read-only.

Commit cb5245a

Browse files
olizillaJacob Heun
authored and
Jacob Heun
committed
fix: drop connection when stream ends unexpectedly
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 #235 Fixes ipfs/js-ipfs#1366 See dignifiedquire/pull-length-prefixed#8 License: MIT Signed-off-by: Oli Evans <[email protected]>
1 parent fef2d11 commit cb5245a

File tree

1 file changed

+12
-5
lines changed

1 file changed

+12
-5
lines changed

src/dial.js

+12-5
Original file line numberDiff line numberDiff line change
@@ -159,12 +159,14 @@ class Dialer {
159159
}
160160

161161
connection.setPeerInfo(this.peerInfo)
162-
163-
waterfall([
164-
(cb) => {
165-
this._attemptMuxerUpgrade(connection, b58Id, cb)
162+
this._attemptMuxerUpgrade(connection, b58Id, (err, muxer) => {
163+
// The underlying stream closed unexpectedly, so drop the connection.
164+
// Fixes https://github.com/libp2p/js-libp2p-switch/issues/235
165+
if (err && err.message === 'Unexpected end of input from reader.') {
166+
log('Connection dropped for %s', b58Id)
167+
return callback(null, null)
166168
}
167-
], (err, muxer) => {
169+
168170
if (err && !this.protocol) {
169171
this.switch.conns[b58Id] = connection
170172
return callback(null, null)
@@ -237,6 +239,11 @@ class Dialer {
237239
const msDialer = new multistream.Dialer()
238240
msDialer.handle(connection, (err) => {
239241
if (err) {
242+
// Repackage errors from pull-streams ending unexpectedly.
243+
// Needed until https://github.com/dignifiedquire/pull-length-prefixed/pull/8 is merged.
244+
if (err === true) {
245+
return callback(new Error('Unexpected end of input from reader.'))
246+
}
240247
return callback(new Error('multistream not supported'))
241248
}
242249

0 commit comments

Comments
 (0)