Body stream error is not caught by Got

[ohana54]

Describe the bug

• Node.js version: 12.14.1
• OS & version: MacOS 10.13.4

When using streams to upload a file, an error thrown from the stream read (for example, passing an inexistent file path) is not being caught by got.

This looks similar to #614

Actual behavior

The code in the example throws an error that is not caught by got itself.
Console output:

events.js:200
      throw er; // Unhandled 'error' event
      ^

Error: ENOENT: no such file or directory, open './no-file.txt'
Emitted 'error' event on ReadStream instance at:
    at internal/fs/streams.js:120:12
    at FSReqCallback.oncomplete (fs.js:146:23) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: './no-file.txt'
}

Expected behavior

The error should be caught and "caught error" should be logged to the console.

Code to reproduce

const got = require('got')
const fs = require('fs')

got
  .put('http://a.com/', {
    body: fs.createReadStream('./file-that-does-not-exist.txt'),
  })
  .catch(() => console.log('caught error'))

PR with failing test: #1047

Checklist

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

[szmarczak]

That's because we use stream.pipeline and it causes all streams to throw:

• the Got stream itself,
• the body.

I wouldn't consider this a bug, although it's quite annoying behavior.

@sindresorhus Should this be configurable through an option? For example catchBodyErrors.

[ohana54]

@szmarczak I agree it can be considered as not a bug (I had second thoughts when I opened the issue), although it's [possibly naively] expected that the library will catch any related exceptions, like it already does very nicely.
Also, handling it on the user side is rather ugly :)

Thank you for considering a solution for this, I really appreciate it.

[sindresorhus]

Got is the one consuming the fs.createReadStream('./file-that-does-not-exist.txt') stream. It's Got's responsibility to catch the body error and forward it to the promise. I don't think we need an option for that.

[sindresorhus]

That's because we use stream.pipeline and it causes all streams to throw:

We should open an issue on Node.js about this. That's definitely not the wanted (or useful) behavior. I assumed it would forward the error to the last stream or the callback to stream.pipeline if there's one.

[szmarczak]

I assumed it would forward the error to the last stream or the callback to stream.pipeline if there's one.

You assumed correctly. stream.pipeline works just fine. I just misguessed it :'(

It's just that there's no error handler for options.body before attaching it to pipeline. Will send a fix ASAP.

[szmarczak]

It's just that it throws the error before Got has the time to attach a handler...

[szmarczak]

Fixed :)

[ohana54]

Awesome, thanks!!

[sindresorhus]

And released: https://github.com/sindresorhus/got/releases/tag/v10.5.1