Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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