Revert "stream: make finished call the callback if the stream is closed"

[mcollina]

Fixes #29699.

This reverts commit b03845b.

Checklist

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

[mcollina]

cc @ronag

[mcollina]

@BethGriggs this would need to be pulled in v13 and/or I would remove b03845b from there asap anyway.

[nodejs-github-bot]

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

[mcollina]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2012/

[ronag]

I'm a big -1 on this.

Instead of reverting the whole thing, please at least let me make a PR that addresses the specific behavioural change referenced in #29699.

Including:

• Documentation for the behaviour.
• A big note in the code.
• Added test for expected compat.
• Workaround for relevant places (if any) in core that uses finished e.g. async iterators.

I can have this done by the end of the day.

Also, I would ask us to continue the discussion in #29699 for finding a long term sustainable strategy for this and similar issues. This indirectly impacts pending work and PR's I have that could end up in a similar situation.

With this I would be -0.

[benjamingr]

I would prefer it if we picked the original one out before the release.

[jasnell]

So long as the changes do not go out in a release and @ronag and @mafintosh can collaborate on a revised acceptable solution, then I think we can afford to hold off reverting this immediately. I think we should time box that however, and the change should definitely be pulled out of the 13.0.0 working branch for now while the fix is being worked on.

[ronag]

I believe I have found an elegant solution which fixes #29699 and possibly make everyone happy (fingers crossed). #29724

[Trott]

@mcollina Unfortunately, there's a merge conflict to be resolved.

@jasnell Do you have a proposal for what should be the time-box deadline? I suppose if @BethGriggs has an opinion, that's probably the opinion we should pay the most attention to.

[BethGriggs]

I'd prefer not picking the commit out of the v13.x branch and release - it has already gone into the first RC, and would need to be forced out of a protected branch.

I can pull in this PR or #29724 into the v13.x release once either of those has landed (subject to no TSC objections as per https://github.com/nodejs/node/blob/master/doc/releases.md#release-branch). My plan is to update the v13.x-proposal each Tuesday up until 15th October - after which I'd like to avoid pulling any additional commits into the release.

[mcollina]

Then I think we should land this asap and then reapply the PR with the fix if we can land it in time.

[jasnell]

I'm happy with whichever approach @BethGriggs and @mcollina are

[Trott]

Am I correct that the two options are either rebase this and land it soon, or we can land #29724? (And in either event, the fix needs to get into the 13.x release.) If we know which one we're going with, we can add it to the 13.x milestone.

[mcollina]

Given that we are running out of time, I propose we land this as soon as possible (tomorrow) as a precaution. That commit should not reach v13 as-is and we should likely do a an RC release without it.

[mafintosh]

Yes, I'm +1 on reverting at this stage as well. This is all super tricky stuff and we need to understand the changes better before trying to land a fix.

[Trott]

Given that we are running out of time, I propose we land this as soon as possible (tomorrow) as a precaution. That commit should not reach v13 as-is and we should likely do a an RC release without it.

Yes, I'm +1 on reverting at this stage as well. This is all super tricky stuff and we need to understand the changes better before trying to land a fix.

Based on this, I've added it to the 13.x milestone.

@mcollina This needs a rebase.

[Trott]

Rubber-stamp LGTM if CI passes and CITGM doesn't show anything surprising and alarming.

[Trott]

@nodejs/tsc This is labeled semver-major so will need a second TSC review.

[ronag]

Also needs to revert ce62e96 to avoid the conflict.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25930/ ✔️

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2043/

This time, without a typo in the ref so it might actually run the tests.

[mcollina]

CITGM seems ok to me, landing.

[mcollina]

Landed in 7682874...9f873b3

[mcollina]

@BethGriggs would you mind adding this to v13?

[BethGriggs]

@mcollina I've pulled it into #29504 and I'm currently building rc-2