When `got.stream.post` fails `pipeline` callback is called without error

[prvak]

Describe the bug

• Node.js version: v12.18.4, v14.15.3
• OS & version: Linux Mint 20
• Got version: 11.8.1

When using got.stream.post with pipeline the pipeline callback is called without error even though the request itself fails with 500 status code.

Actual behavior

The pipeline callback is called without error. Script below outputs:

pipe success
output error ...

Expected behavior

The pipeline callback should be called with an error. Script below should output:

pipe error ...
output error ...

Code to reproduce

Similar to the example from documentation: https://github.com/sindresorhus/got#streams

Save as got-test.js and run node got-test.js (it is using this file contents as input).

const got = require('got');
const { pipeline } = require('stream');
const fs = require('fs');

const input = fs.createReadStream('./got-test.js');
const output = got.stream.post('https://httpstat.us/500');

pipeline(
  input,
  output,
  (error) => {
    if (error) {
      console.log('pipe error', error);
    } else {
      console.log('pipe success');
    }
  }
);

output.on('error', error => console.log('output error', error));

Checklist

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

[Giotino]

https://github.com/sindresorhus/got/issues/1514#issuecomment-719413307

[prvak]

@Giotino Thank you for the link.

What is a bit misleading is that got.post throws on HTTP error while got.post.stream with pipeline does not, unless you explicitly read the response body. I cannot imagine a situation where I would want to use just

    await pipeline(
        fs.createReadStream('index.html'),
        got.stream.post('https://sindresorhus.com')
    );

Without the new stream.PassThrough() that code completely ignores any HTTP errors.

After going through the discussion in https://github.com/sindresorhus/got/issues/1481 I see that this is an expected behavior. @aalexgabi already made all the points I would have made. Mainly

I think most people assume that Got will throw an error if a 4xx or 5xx status code is sent by the server. Doing the request by passing the body or by using a stream should not change that behavior. The stream is just a solution to avoid keeping a lot of data in memory. It should provide the same level of reliability.

and

There should be a clear warning in the example saying that got won't detect any HTTP response error, nor any connection drop during the transmission of the response.

Please, add new stream.PassThrough() to the example in the documentation. It it were there it would have saved me a lot of frustration :-)

[Giotino]

@Giotino Thank you for the link.

I'm sorry that I just sent you the link without writing anything more, but I was a little busy and I started feeling that lately I answered that same question a lot.

What is a bit misleading is that got.post throws on HTTP error while got.post.stream with pipeline does not, unless you explicitly read the response body.

I would suggest that "Stream API are for those who know what they're doing". With that I don't mean you don't know what you're doing, but if you use a stream you have to "consume" the "output" of it if you care about what you're receiving from the server.

Please, add new stream.PassThrough() to the example in the documentation. It it were there it would have saved me a lot of frustration :-)

I think I already suggested to add an example (or a note) about that behavior but I think it never made to the documentation because of "Stream API are for those who know what they're doing".

If you need to send a file I suggest you to do as follows

await got.post('https://sindresorhus.com', {
  body: fs.createReadStream('index.html'),
  retry: 0
});

[prvak]

I was a little busy and I started feeling that lately I answered that same question a lot.

I understand. It is my fault that I failed to find the other issue by myself. Sorry for that.

If you need to answer the same question so much, isn't it because the behavior is not obvious from the documentation? :-)

Anyways, thank you for the tip with passing the stream as body. It should work for my use case.

[Giotino]

If you need to answer the same question so much, isn't it because the behavior is not obvious from the documentation? :-)

My bad, I missed a comment from @szmarczak in #1481

At least we can modify the pipeline example as something like the following

Yep, that sounds good. Feel free to send a PR.

I think I should be able to make the modification to the example soon, thank you for your report.

[Giotino]

1566