From cb5245a99d979fdc322be658a4e77cfcee76fd9f Mon Sep 17 00:00:00 2001
From: Oli Evans <oli@tableflip.io>
Date: Wed, 30 May 2018 23:17:18 +0100
Subject: [PATCH] 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 https://github.com/dignifiedquire/pull-length-prefixed/pull/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 https://github.com/ipfs/js-ipfs/issues/1366
See https://github.com/dignifiedquire/pull-length-prefixed/pull/8

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
---
 src/dial.js | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/dial.js b/src/dial.js
index 116c628847..0082f7bff1 100644
--- a/src/dial.js
+++ b/src/dial.js
@@ -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)
@@ -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'))
       }