[x] stream: don't emit finish after destroy()

[ronag]

When destroying, the stream will be potentially incomplete and therefore it doesn't make sense to emit finish.

Also simplify and inline related conditions.

After destroy() (in general) we should really only be emitting 'close' and possibly 'error'.

As we do in fs the user should call end() if they want to gracefully close/destroy a stream (#15407).

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

NOTE TO SELF: do minor refactoring and cleanup after merge

[ronag]

When reviewing please look at each commit, otherwise it's difficult to fully follow the changes.

[mcollina]

LGTM

[ronag]

Unchecking test. Seems there are some failures I missed. Probably need to update tests to consider the new behavior.

[ronag]

Rebased and test fixed.

[nodejs-github-bot]

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

[ronag]

@mcollina: This required more significant changes to pass all test. Mainly in relation to fs, _destroy and autoClose.

[lpinca]

I would prefer to wait for @addaleax's review as it seems this partially reverts d3f02d0.

[ronag]

@lpinca: How? clearBuffer checks for state.destroyed. https://github.com/nodejs/node/blob/1107841e76dbf884245811216f2246ba1efda7f1/lib/_stream_writable.js#L490

[lpinca]

Yes but I think it goes against the original intent of d3f02d0? At least this is what it seems to me by looking at the changes made to test/parallel/test-stream-write-destroy.js.

The proposed change to not emit 'finish' after destroy seems reasonable to me. I'm just not sure if we did already change that to actually emit 'finish' after destroy. Sorry for the bad wording. Hope it makes sense :)

[ronag]

@lpinca: Hm, I see. Yea, let's wait for @addaleax.

[addaleax]

Yeah, I see that there could be arguments made for either behaviour. I’m not sure why we wouldn’t emit finished when the writable side is actually finished (as opposed to what the PR description indicates?), but either way, I agree with this being semver-major.

[ronag]

why we wouldn’t emit finished when the writable side is actually finished

Because it's unexpected?

e.g.

foo.on('continue', () => {
  // more followup side effects...
})

s.on('finish', () => {
  foo.emit( 'continue')
});

foo.on('cancelled', () => {
  foo.destroy()
  foo.on('continue', mustNotCall())
});

As a user I wouldn't want finish to be emitted if I destroy the stream, I would expect destroy is aborting any following side effects as well. I would not expect to have to remove my finish listener.

[addaleax]

I don’t really see why it’s unexpected – yes, you need to be aware of whether the stream was destroyed, but in the 'finished' handler you should only be doing things that you want to do when the stream actually finished, and so I don’t see why we would skip that when the streams is destroyed.

In the end, it’s not that important to me – you can go ahead with this, and as long as it’s landed in a major release, I don’t think this is a huge issue.

[jasnell]

Hmm... I'm not convinced just yet. If destroy() is called with an Error, then I can definitely see the argument on not emitting finish, but when destroy() is called without an error, I can see still cases where finish may be necessary... there may be many stream implementations out in the wild that make that assumption. One thing we have never defined are consistent abort semantics across all streams. Perhaps that's where we should actually start? That is, define a standard abort pattern that can be optionally enabled for all streams through an option. Once a stream implementation opts in to the standard abort, destroy() before finish()/end() would emit abort and not emit finish or end.

I'd be much happier with that kind of approach than this semver-major change.

[ronag]

defined are consistent abort semantics across all streams

Yep, I asked something similar to be added for the TSC agenda today/tomorrow (#29197).

If destroy() is called with an Error, then I can definitely see the argument on not emitting finish, but when destroy() is called without an error, I can see still cases where finish may be necessary...

Currently we do not have any well defined semantics to achieve both at the same time... Would probably have to add more state to the streams or maybe we could use errorEmitted. Unsure.

My opinion is that 'finish' after destroy() is not bad per se, but I find it unexpected.

My assumption so far based on the rest of the code and also the documentation has been that after destroy() only 'error' and/or 'close' is/are emitted.

If we compare with rxjs observables or other stream similar library. If I dispose an observable, it will not call my complete function, even if the observable would have finished in the same tick. Same thing with cancellable promises.

I'm really fine either way (although I prefer avoiding unexpected behaviours even if per se technically correct) as long it is documented and that we strive towards consistency.

[ronag]

@Trott: As it is related, should we attach this to the same item as #29197 on the TSC agenda, or does that make the scope too big?

[Trott]

@Trott: As it is related, should we attach this to the same item as #29197 on the TSC agenda, or does that make the scope too big?

Honestly, I doubt the TSC is going to make a decision on any of this tomorrow--the conversation will be started, sure, but I wouldn't expect immediate resolution. So given that, lots of scope and context is fine. I'll add a note and a link here to the TSC agenda, but I predict most TSC members won't actually follow along until after the meeting.

[mcollina]

@jasnell I think that .destroy() is an abrupt operation, and likely there is data loss. 'finish' indicates a clean close where all internal buffers have been flushed out.

I'm +1 in adding a on('destroy') event ('abort' is used in http).

@ronag can you make sure we do not emit 'end' in the same circumstances (and in this PR, if changes need to be made)?

[jasnell]

Yep, I agree that finish after destroy is not really ideal, but there's lots of streams code out there that could likely break on this change. As I said, the best approach here is to better define a standardized pattern and make it opt-in for now. We can transition and flip it to opt-out later

[mcollina]

Agreed @jasnell

See #29205 (comment)

[ronag]

So should I add some kind of option flag? Any name suggestions? strict: true?

[ronag]

If we add strict: true we could use that for other strange compat related behaviour such as emitClose: false.

[ronag]

@Trott: I think this needs a WIP label while we figure out what we want.

[ronag]

I don't know what to do here... Closing until someone can provide some consensus on what to do.