Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

got.stream() swallows errors when used to upload small files with stream.pipeline() #1026

Closed
aalexgabi opened this issue Jan 17, 2020 · 0 comments · Fixed by #1051
Closed
Labels
bug Something does not work as it should ✭ help wanted ✭

Comments

@aalexgabi
Copy link

aalexgabi commented Jan 17, 2020

Describe the bug

  • Node.js version: v13.5.0
  • OS & version: Linux pc 5.3.18-1-MANJARO Stream support #1 SMP PREEMPT Wed Dec 18 18:34:35 UTC 2019 x86_64 GNU/Linux

got.stream() stream emits finish before error in case of ECONNREFUSED. The finish event should be used to signal that all data has been written/sent once stream.end() has been called (https://nodejs.org/api/stream.html#stream_event_finish).

Actual behavior

stream.pipeline() promise is resolved.

Expected behavior

stream.pipeline() should be rejected with ECONNREFUSED;

Code to reproduce

This code

const fs = require('fs');
const util = require('util');
const stream = require('stream');

const pipeline = util.promisify(stream.pipeline);
const got = require('got');


async function run() {
    await pipeline(
        // Read small file
        fs.createReadStream('/proc/version'),
        // Generates ECONNREFUSED
        got.stream.put('http://localhost:7777')
    )
}

run().then(() => {
    console.log('done');
}).catch((err) => {
    console.log('err', err);
})

Prints:

done

It seems that this is because stream.pipeline() considers the data handled if the finish event is received. To isolate the issue I wrote another small test:

const got = require('got');

const u = got.stream.put('http://localhost:7777')
u.on('error', (err) => {
    console.log('err', new Date(), err);
});
u.on('end', () => {
    console.log('end', new Date());
});
u.on('close', () => {
    console.log('close', new Date());
});
u.on('finish', () => {
    console.log('finish', new Date());
});

const writeSomeMore = u.write('test');
console.log('writeSomeMore', writeSomeMore);
u.end();

Which prints:

writeSomeMore true
finish 2020-01-17T13:50:48.061Z
err 2020-01-17T13:50:48.071Z GotError: connect ECONNREFUSED 127.0.0.1:7777
    at onError (.../node_modules/got/dist/source/request-as-event-emitter.js:137:29)
    at handleRequest (.../node_modules/got/dist/source/request-as-event-emitter.js:170:17)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1136:16) {
  name: 'RequestError',
  code: 'ECONNREFUSED'
}

Note that if I read /dev/urandom (a bigger file) instead of /proc/version the stream.pipeline() promise is rejected.

Checklist

  • [ x] I have read the documentation.
  • [ x] I have tried my code with the latest version of Node.js and Got.
@aalexgabi aalexgabi changed the title got.stream() swallows errors when used with pipeline got.stream() swallows errors when used to upload small files with pipeline Jan 17, 2020
@aalexgabi aalexgabi changed the title got.stream() swallows errors when used to upload small files with pipeline got.stream() swallows errors when used to upload small files with stream.pipeline() Jan 17, 2020
@szmarczak szmarczak added bug Something does not work as it should ✭ help wanted ✭ labels Jan 17, 2020
@szmarczak szmarczak mentioned this issue Feb 6, 2020
18 tasks
szmarczak added a commit to szmarczak/got that referenced this issue Feb 29, 2020
szmarczak added a commit to szmarczak/got that referenced this issue Mar 3, 2020
szmarczak added a commit to szmarczak/got that referenced this issue Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants