[x] stream: eof & pipeline compat

[ronag]

Some broken streams might emit 'close' before 'end' or 'finish'. However, if they have actually been ended or finished, this should not result in a premature close error.

This fixes #29699 while still maintaining what could be considered correct behaviour.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

ping @mafintosh

[ronag]

Is this still relevant? Been stale for a while...

Happy new year!

[ronag]

rebased to fix conflicts

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28066/

[ronag]

CI

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28074/

[ronag]

ping @mafintosh

[ronag]

rebased to fix conflict

[ronag]

Turns out this actually had a little bit of an issue, the stream might still be in a pending state after destroyed so checking that is not enough.

[mafintosh]

This is still quite complex, but tried to do another pass at it. Can give an slight 👍. As mentioned in my comment I think it's worth investing in making a happy-path for when a stream is autoDestroy: true since that's much easier to reason about.

[ronag]

@mafintosh

but tried to do another pass at it. Can give an slight 👍.

Thx!

I think it's worth investing in making a happy-path for when a stream is autoDestroy: true since that's much easier to reason about.

I'm not quite sure I understand what you mean by this? Something like the following?

  if (autoDestroy && emitClose) {
    if (opts.error !== false) stream.on('error', onerror);

    function onclose() {
      callback.call(stream);
    }

    if (closed) {
      process.nextTick(callback.bind(stream));
    } else {
      stream.on('close', onclose);
    }

    return () => {
      stream.removeListener('close', onclose);
      stream.removeListener('error', onerror);
    };
  }

  // Rest is compat and edge cases...

[ronag]

My general feeling is that landing this safely is uncertain due to the complexity of the changes and uncertainty of reviewers.

There are actually several changes here that do not need to be directly related and could be split up.

I'm closing this in favour of smaller on more specific PR's. Please let me know if there are any objections to this.