stream: pipeline error callback

[ktfth]

Included some tests to pipeline error with createTransformStream

[aduh95]

/cc @nodejs/streams

[ronag]

Is this supposed to be a failing test? Also you probably need to spawn a child process for testing stdout.

[mcollina]

cc @nodejs/streams

[ktfth]

Is this supposed to be a failing test? Also you probably need to spawn a child process for testing stdout.

Do you have an example to do that task?

[ktfth]

I mading some tests before write a fix for the case

[ktfth]

I'm trying do understand how the reference occur with ret and when to apply without a variable

[ktfth]

@ronag and @mcollina something strange appear, the test do not fail for a case who i have created. Have some suggestions for that case in specific?

[ktfth]

I working on an improvement of this solution

[rluvaton]

I think you should add in your tests a comment with a reference for the issue your trying to reproduce, like in:

https://github.com/nodejs/node/blob/08ef0ae998df5b7bb99e23ec9b82b27854589495/test/pseudo-tty/test-tty-stdin-end.js#L5

[rluvaton]

Also, as I wrote here, the issue persist with async iterators...

[ktfth]

This looks wrong to me. What are we trying to solve? Does the transform stream test fail?

For me too, we are trying to solve this: https://github.com/nodejs/node/issues/39447 Resumed the callback is not called because of destroyer, i'm looking for a new solution for that.

The test not fail on this case.

[ronag]

But this seems more like a problem with stdout than pipeline? Is the problem reproducible without stdout?

[ktfth]

But this seems more like a problem with stdout than pipeline? Is the problem reproducible without stdout?

Yes can be reproducible without process.stdout

[ktfth]

Also, as I wrote here, the issue persist with async iterators...

Working on that case

[ronag]

But this seems more like a problem with stdout than pipeline? Is the problem reproducible without stdout?

Yes can be reproducible without process.stdout

Do you have an example?

[ktfth]

But this seems more like a problem with stdout than pipeline? Is the problem reproducible without stdout?

Yes can be reproducible without process.stdout

Do you have an example?

'use strict';
const {
  Transform,
  Writable,
  pipeline,
} = require('stream');
const assert = require('assert');

function createTransformStream(tf, context) {
  return new Transform({
    readableObjectMode: true,
    writableObjectMode: true,

    transform(chunk, encoding, done) {
      tf(chunk, context, done);
    }
  });
}

const write = new Writable({
  write(data, enc, cb) {
    cb();
  }
});

const ts = createTransformStream((chunk, _, done) => {
  return done(new Error('Artificial error'));
});

pipeline(ts, write, (err) => {
  assert.ok(err, 'should have an error');
  console.log(err);
});

ts.write('test');

The case above help?

[ronag]

'use strict';
const {
  Transform,
  Writable,
  pipeline,
} = require('stream');
const assert = require('assert');

function createTransformStream(tf, context) {
  return new Transform({
    readableObjectMode: true,
    writableObjectMode: true,

    transform(chunk, encoding, done) {
      tf(chunk, context, done);
    }
  });
}

const write = new Writable({
  write(data, enc, cb) {
    cb();
  }
});

const ts = createTransformStream((chunk, _, done) => {
  return done(new Error('Artificial error'));
});

pipeline(ts, write, (err) => {
  assert.ok(err, 'should have an error');
  console.log(err);
});

ts.write('test');

That works fine on Node v16.3.0? What version does it fail on?

[ktfth]

'use strict';
const {
  Transform,
  Writable,
  pipeline,
} = require('stream');
const assert = require('assert');

function createTransformStream(tf, context) {
  return new Transform({
    readableObjectMode: true,
    writableObjectMode: true,

    transform(chunk, encoding, done) {
      tf(chunk, context, done);
    }
  });
}

const write = new Writable({
  write(data, enc, cb) {
    cb();
  }
});

const ts = createTransformStream((chunk, _, done) => {
  return done(new Error('Artificial error'));
});

pipeline(ts, write, (err) => {
  assert.ok(err, 'should have an error');
  console.log(err);
});

ts.write('test');

That works fine on Node v16.3.0? What version does it fail on?

With this example work fine on both versions mentioned here and on the issue. https://github.com/nodejs/node/issues/39447

[ronag]

With this example work fine on both versions mentioned here and on the issue. #39447

Yes? So what are we trying to solve here? Isn't it stdout?

[ktfth]

With this example work fine on both versions mentioned here and on the issue. #39447

Yes? So what are we trying to solve here? Isn't it stdout?

Makes sense, can you help understand better the problem with stdout because we have something to count here but is totally a draft of a real better solution. And thank you for your time, you it is a strong connection

[ktfth]

With this example work fine on both versions mentioned here and on the issue. #39447

Yes? So what are we trying to solve here? Isn't it stdout?

On the refactor i have founded some indications to process on destroyer the correctly behavior, looks good to me but need some review

[mcollina]

I think this branch contains some unwanted changes, as it shows all unrelated stuff in github. Could you please rebase your changes on top of master?

[ktfth]

I think this branch contains some unwanted changes, as it shows all unrelated stuff in github. Could you please rebase your changes on top of master?

I can do that

[ktfth]

I think this branch contains some unwanted changes, as it shows all unrelated stuff in github. Could you please rebase your changes on top of master?

One test fail local, for test-tty-color-support comes with reabase... what we can do?

[ktfth]

@mcollina all done after checks

[rluvaton]

I think I found the problem, the problem is that the process.stdout closed which caused the console.log to never output the data (because it use the process.stdout underneath) but when changed to process.stderr.write in the pipeline cb you see that it does get called (there is an output)

This is because the destroyer see that the process.stdout has a destroy function, (unrelated, the process.stdout also have an dummyDestroy assigned to his _destroy property )

WDYT @mcollina @ronag @ktfth ?

[ktfth]

I think I found the problem, the problem is that the process.stdout closed which caused the console.log to never output the data (because it use the process.stdout underneath) but when changed to process.stderr.write in the pipeline cb you see that it does get called (there is an output)

This is because the destroyer see that the process.stdout has a destroy function, (unrelated, the process.stdout also have an dummyDestroy assigned to his _destroy property )

WDYT @mcollina @ronag @ktfth ?

Good work, waiting the others to get more information about

[mcollina]

If you look at https://github.com/nodejs/node/blob/954217adda03457115e426e9126dcad845877f74/lib/internal/bootstrap/switches/is_main_thread.js#L101-L114, you will see that we are not emitting 'close' if _writableState.emitClose is not set. I think we might want to revisit that if check and/or set emitClose in the relevant place.

[rluvaton]

I'm afraid some change here gonna break some CLI applications.

how can you make changes with confidence when you can break some library? I'm afraid of every change

[rluvaton]

If you look at https://github.com/nodejs/node/blob/954217adda03457115e426e9126dcad845877f74/lib/internal/bootstrap/switches/is_main_thread.js#L101-L114, you will see that we are not emitting 'close' if _writableState.emitClose is not set. I think we might want to revisit that if check and/or set emitClose in the relevant place.

process.stdout.destroy is not the same function as process.stdout._destroy, so changing that probably won't fix it as the destroy function is called and not the _destroy function

When the _destroy function called I think it's all good

Q: why is there destroy and _destroy?