Response read timeout

[sullman]

Describe the bug

• Node.js version: 14.14.0
• OS & version: Ubuntu 20.04

We've noticed that the timeout isn't applied very consistently when using streams and piping the stream to something slow. It's not particularly obvious if the timeout is meant to account for this.

Actual behavior

Depending on the backpressure of the stream being piped to, the stream returned by got.stream() may end, error, or hang.

Expected behavior

I expected the timeout to trigger consistently rather than depending on the backpressure.

Code to reproduce

const http = require('http');
const { finished, Readable, Writable } = require('stream');
const got = require('got');

function createReadStream(size) {
  return new Readable({
    read(n) {
      const chunkSize = Math.min(size, n);
      size -= chunkSize;
      if (chunkSize) {
        setImmediate(() => {
          this.push(Buffer.alloc(chunkSize, 'a'));
        });
      } else {
        this.push(null);
      }
    }
  });
}

function createBlackHole() {
  return new Writable({
    write() {}
  });
}

const server = http.createServer();

server.on('request', (request, response) => {
  const size = parseInt(request.url.slice(1)) || Infinity;

  response.statusCode = 200;
  response.setHeader('Content-Type', 'application/octet-stream');
  createReadStream(size).pipe(response);
});

async function makeRequest(size) {
  const timeout = 2000;
  const stream = got.stream(`http://localhost:3000/${size}`, { timeout });

  console.log(`\nMaking request for stream of ${size} bytes...`);

  return new Promise((resolve, reject) => {
    const timer = setTimeout(() => {
      console.warn('Stream never ended or errored!');
      resolve();
    }, timeout + 1000);

    const cleanup = finished(stream, err => {
      cleanup();
      clearTimeout(timer);

      if (err) {
        console.log(`Stream errored: ${err.message}`);
      } else {
        console.log('Stream ended');
      }

      resolve();
    });

    stream.pipe(createBlackHole());
  });
}

async function runTests() {
  // At a small number of bytes, the response emits end and the request emits
  // close, and the stream will be finished. No timeouts.
  await makeRequest(1024);

  // At a number of bytes slightly bigger than 16kb, the request will emit
  // close which will cause got to clear the request timeout. The response
  // doesn't emit end and the stream will never be finished. Our fallback timer
  // will expire.
  await makeRequest(1024 * 20);

  // At a number of bytes larger than 32kb, the request will not emit close and
  // the response will not emit end. Got's request timeout will fire and destroy
  // the request.
  await makeRequest(1024 * 40);
}

server.listen(3000, err => {
  if (err) {
    return console.error(err);
  }

  runTests().then(() => {
    process.exit();
  });
});

Output:

Making request for stream of 1024 bytes...
Stream ended

Making request for stream of 20480 bytes...
Stream never ended or errored!

Making request for stream of 40960 bytes...
Stream errored: Timeout awaiting 'request' for 2000ms

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

---

I'm willing to try to open a PR to address this, but I want to be sure that this isn't considered expected behavior since it has as much to do with the stream being piped to as it does got itself. While that's true, I think the expectation when passing timeout is that the request can't hang indefinitely.

[szmarczak]

This is actually not a bug. In the second example Got has actually successfully received the entire response body, but you haven't read it from the stream (missing write(chunk, encoding, callback) { callback(); }). It doesn't hang. If you server.close() then the process just exits as expected.

The last example actually times out, because Got doesn't read from the socket as it waits for your acknowledgement.

[szmarczak]

The proper solution is to attach a timeout to the stream of createBlackHole(), simply call blackHole.destroy(new Error('processing timed out')) on timeout.

[szmarczak]

If you consider this a feature request (do let me know) then I'll reopen and wait for @sindresorhus' opinion on this :)

[sullman]

Sure, I'm happy to consider it a feature request instead. :] Our team internally has been debating whether we consider it a bug since it has to do with the consumption of the stream. Ultimately it just felt strange because of the inconsistency. Without having this handled by got's timeout, since we have multiple places we make streaming requests we'll probably end up wrapping those calls to add our own timeout logic.

One possibility that was on my mind was adding an additional timeout type. Since request is clearly defined as ending when the response's end event fires, maybe it would make sense to have a stream timeout that wouldn't clear until the stream had ended?

[szmarczak]

@sindresorhus What about a new timeout option: read. It would be a special one, as it wouldn't be affected by the global timeout (timeout.request) because it would be too breaking. Simply if the response hasn't been consumed after X seconds, destroy it with an error.

[szmarczak]

/cc @Giotino What do you think? ^^^^

[sindresorhus]

@szmarczak Sounds good to me.