caolan / highland

High-level streams library for Node.js and the browser
https://caolan.github.io/highland
Apache License 2.0
3.43k stars 147 forks source link

concat stream not emitting "end" event #653

Open benjamine opened 6 years ago

benjamine commented 6 years ago

I might be missing something but I'm trying a simple stream concat of 2 streams, and noticed that while each stream emits the "end" event, the concat result doesn't.

Reproduction Steps:

var highland = require("highland");
(function() {
s1 = highland(['1', '2', '3']), s2 = highland(['4', '5', '6']), s3 = s1.concat(s2);
const log = s => process.stdout.write(` [${s}] `);
s1.on('end', function() { log('s1 ended') });
s2.on('end', function() { log('s2 ended') });
s3.on('end', function() { log('s3 ended') });
s3.map(n => `[${n}]`).pipe(process.stdout);
})();

Outputs: [1][2][3] [s1 ended] [4][5][6] [s2 ended] undefined Expected: [s3 ended] to print to console too.

ps: I noticed that if I add another transform at the end ( s4 = s3.map(....); s4.on('end', ...)), that stream does emit "end".

vqvu commented 6 years ago

This sounds like a bug. The "undefined" part of the output too.

I'm travelling right now, but I'll try to take a look in a few days.

Does using

stream.observe().done(...)

work as a workaround for the 'end' event? Using events directly isn't really recommended, since they're not on a well-tested code path.

On Sun, Jul 8, 2018, 6:35 PM Benjamín Eidelman notifications@github.com wrote:

I might be missing something but I'm trying a simple stream concat of 2 streams, and noticed that while each stream emits the "end" event, the concat result doesn't.

Reproduction Steps:

var highland = require("highland"); (function() { s1 = highland(['1', '2', '3']), s2 = highland(['4', '5', '6']), s3 = s1.concat(s2);const log = s => process.stdout.write([${s}]);s1.on('end', function() { log('s1 ended') });s2.on('end', function() { log('s2 ended') });s3.on('end', function() { log('s3 ended') });s3.map(n => [${n}]).pipe(process.stdout); })();

Outputs: [1][2][3] [s1 ended] [4][5][6] [s2 ended] undefined Expected: [s3 ended] to print to console too.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/caolan/highland/issues/653, or mute the thread https://github.com/notifications/unsubscribe-auth/AGIyZTj0ZhJB1KKfLC1EDySHwGsJEHB_ks5uErNHgaJpZM4VG9nJ .

benjamine commented 6 years ago

thanks!

just tried:

var highland = require("highland");
(function() {
s1 = highland(['1', '2', '3']), s2 = highland(['4', '5', '6']), s3 = s1.concat(s2);
const log = s => process.stdout.write(` [${s}] `);
s1.observe().done(function() { log('s1 done') });
s2.observe().done(function() { log('s2 done') });
s3.observe().done(function() { log('s3 done') });
s3.map(n => `[${n}]`).pipe(process.stdout);
})();

but same results

vqvu commented 6 years ago

Took a look at this on my phone while waiting at the airport. The reason why observe doesn't work is this todo: https://github.com/caolan/highland/blob/2.x/lib/index.js#L1367

The reason why on('end') doesn't work is because of an different but related bug where we don't copy the EventEmitter handlers. I suspect the 'data' and 'error' events don't work either when combined with stream redirection.

On Sun, Jul 8, 2018, 8:20 PM Benjamín Eidelman notifications@github.com wrote:

thanks!

just tried:

var highland = require("highland"); (function() { s1 = highland(['1', '2', '3']), s2 = highland(['4', '5', '6']), s3 = s1.concat(s2);const log = s => process.stdout.write([${s}]);s1.observe().done(function() { log('s1 done') });s2.observe().done(function() { log('s2 done') });s3.observe().done(function() { log('s3 done') });s3.map(n => [${n}]).pipe(process.stdout); })();

but same results

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/caolan/highland/issues/653#issuecomment-403348953, or mute the thread https://github.com/notifications/unsubscribe-auth/AGIyZWNg0N89VI3BtrXdt_t1GVBgdkH9ks5uEswRgaJpZM4VG9nJ .