Stream does not report errors when status code is different of 2xx

[yenyen]

Describe the bug

• Node.js version: v12.16.3
• OS & version: Windows 10 Pro 1909 (and Ubuntu WSL 18.04)

When got.stream is used and the server return an error code, got does not report the error. This makes very difficult to know if the request was a success (and more importantly, it lets you think that your request was a success.) This is not consistent with other methods.

Expected behavior

got.stream should trigger an "error" event when a server responses with a 4xx or 5xx status code

Code to reproduce

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

// console log => 'ERROR'
got.post('https://www.google.com', { body: 'hello world' }).then(() => console.log('OK')).catch(error => { console.log('ERROR') });

// console log => 'OK'
pipeline(
    Readable.from('hello world'),
    got.stream.post('https://www.google.com'),
    err => {
        if (err) {
            console.log('ERROR')
        } else {
            console.log('OK')
        }
    }
);

[szmarczak]

It does error. It's just that you do not handle it. Try adding a PassThrough stream to the pipeline.

[yenyen]

Thank you for your quick response. Indeed, it is working if we add a PassThrough step to the pipeline.

I think, this should be added to your documentation (we almost lost critical data because of this...)

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

const pipeline = promisify(stream.pipeline);

(async () => {
    await pipeline(
        got.stream('https://sindresorhus.com'),
        fs.createWriteStream('index.html')
    );

    // For POST, PUT, and PATCH methods `got.stream` returns a `stream.Writable`
    await pipeline(
        fs.createReadStream('index.html'),
        got.stream.post('https://sindresorhus.com'),
        new stream.PassThrough()
    );
})();

[szmarczak]

I think, this should be added to your documentation (we almost lost critical data because of this...)

Maybe... But the stream.pipeline docs states that it adds a hanging error handler, so you should be aware of what you're using :)

stream.pipeline() leaves dangling event listeners on the streams after the callback has been invoked. In the case of reuse of streams after failure, this can cause event listener leaks and swallowed errors.

[szmarczak]

I made a Node.js issue about it some while ago, and they stated that it's "correct behavior to fix broken streams". Unfortunately they're not aware that they broke more things than they fixed.

[yenyen]

OK, thank you to have taken your time to response.

I think the behavior of pipeline is dangerous and I am sure that I am not the only person who did not expected to work like this. Maybe a little note on your REAME.md to warn that the requests errors are not catched (and to add a PassThrough() step to be sure it does) would be great.