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

[gjunkie]

Left just a couple of small, non-blocking comments.

[gjunkie]

What would happen here?

[ktfth]

In some cases the shift function are not encountered

[ronag]

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

[rluvaton]

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

node/test/pseudo-tty/test-tty-stdin-end.js

Line 5 in 08ef0ae

// https://github.com/nodejs/node/issues/1068

[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: #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. #39447

[rluvaton]

Maybe it's just me but what's the point of this? It seems very confusing to check the length of the array that you're assigning to... 😬

[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

[ktfth]

@ronag and @mcollina updates to something we can discuss and get into a solution

[ktfth]

@ronag and @mcollina I added the solution for the issue, but is just to show where this appear. Can both of you guide me for the correctly solution based on this evidence?

[mcollina]

Is https://github.com/nodejs/node/pull/39533/files#diff-f9aa56489c061a70f83b602c8a32200b4f092a23b6eb8769f978ac1adb322a46R342 still needed?

[ktfth]

Is https://github.com/nodejs/node/pull/39533/files#diff-f9aa56489c061a70f83b602c8a32200b4f092a23b6eb8769f978ac1adb322a46R342 still needed?

Yes, because of the callback behavior

[mcollina]

Is https://github.com/nodejs/node/pull/39533/files#diff-f9aa56489c061a70f83b602c8a32200b4f092a23b6eb8769f978ac1adb322a46R342 still needed?

Yes, because of the callback behavior

I removed it and it seems all tests are passing.

[ktfth]

Is https://github.com/nodejs/node/pull/39533/files#diff-f9aa56489c061a70f83b602c8a32200b4f092a23b6eb8769f978ac1adb322a46R342 still needed?

Yes, because of the callback behavior

I removed it and it seems all tests are passing.

There is an issue with that, you know how force test check the callback?

[ktfth]

I'm not founded an example

[mcollina]

Please look at #39447 (comment). On a closer look this does not seem a bug.