From 25b6b51bf2210c1dfd05459d4e74dd605d47b6a1 Mon Sep 17 00:00:00 2001
From: nlf <quitlahok@gmail.com>
Date: Thu, 24 Mar 2022 08:25:21 -0700
Subject: [PATCH] fix: do not setup the response timeout if the stream has
 already ended

this change ensures we will not trigger a timeout caused by a slow
stream pipeline if the stream has already ended. this subtly shifts the
meaning of the timeout from "the response has been fully consumed" to
"the socket behind the response has finished" which i feel maintains the
spirit of the timeout while also not needlessly throwing errors when
we're not using the socket any more.
---
 lib/body.js  |  6 ++++--
 test/body.js | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/lib/body.js b/lib/body.js
index 2b8e3bd..c7ffa5b 100644
--- a/lib/body.js
+++ b/lib/body.js
@@ -127,8 +127,10 @@ class Body {
       : this.size ? new MinipassSized({ size: this.size })
       : new Minipass()
 
-    // allow timeout on slow response body
-    const resTimeout = this.timeout ? setTimeout(() => {
+    // allow timeout on slow response body, but only if the stream is still writable. this
+    // makes the timeout center on the socket stream from lib/index.js rather than the
+    // intermediary minipass stream we create to receive the data
+    const resTimeout = this.timeout && stream.writable ? setTimeout(() => {
       stream.emit('error', new FetchError(
         `Response timeout while trying to fetch ${
           this.url} (over ${this.timeout}ms)`, 'body-timeout'))
diff --git a/test/body.js b/test/body.js
index 7108b5f..1d280e9 100644
--- a/test/body.js
+++ b/test/body.js
@@ -176,6 +176,33 @@ t.test('stream body too slow', async t => {
   })
 })
 
+t.test('no timeout if stream ends before we even start consuming', async t => {
+  // this test mimics how lib/index.js writes data into the intermediary minipass stream
+
+  // the SlowMinipass class delays the result from concat() mimicking a slow pipe downstream
+  class SlowMinipass extends Minipass {
+    async concat () {
+      // 10 millisecond delay before resolving
+      await new Promise((resolve) => setTimeout(resolve, 10))
+      return super.concat()
+    }
+  }
+  const networkStream = new Minipass()
+  const wrappedStream = new SlowMinipass()
+
+  networkStream.on('data', (chunk) => wrappedStream.write(chunk))
+  networkStream.on('end', () => wrappedStream.end())
+
+  for (let i = 0; i < 10; ++i) {
+    networkStream.write('some data')
+  }
+  networkStream.end()
+
+  // timeout of 1ms, must be lower than the 10ms used in SlowMinipass to trigger the bug
+  const b = new Body(wrappedStream, { timeout: 1 })
+  await t.resolves(b.text(), 'some data')
+})
+
 t.test('random toString-ing thing body', async t => {
   const b = new Body({ toString () {
     return 'a=1'