Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 19, 2019

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 19, 2019
@ronag ronag force-pushed the stream-finish-after-destroy branch from 7673bdd to d18961a Compare August 19, 2019 15:53
@ronag
Copy link
Member Author

ronag commented Aug 19, 2019

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

@ronag ronag force-pushed the stream-finish-after-destroy branch from d18961a to 11c1620 Compare August 19, 2019 15:54
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 20, 2019
mcollina
mcollina previously approved these changes Aug 20, 2019
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina requested a review from a team August 20, 2019 08:21
@ronag
Copy link
Member Author

ronag commented Aug 20, 2019

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

When destroying the stream will be potentially incomplete and
therefore it doesn't make sense to emit finish.
@ronag ronag force-pushed the stream-finish-after-destroy branch 2 times, most recently from d4602d4 to b8f013a Compare August 20, 2019 16:13
@ronag
Copy link
Member Author

ronag commented Aug 20, 2019

Rebased and test fixed.

@ronag ronag force-pushed the stream-finish-after-destroy branch 2 times, most recently from bc067c0 to b8f013a Compare August 20, 2019 16:17
@nodejs-github-bot
Copy link
Collaborator

@ronag ronag force-pushed the stream-finish-after-destroy branch from b8f013a to 1107841 Compare August 20, 2019 18:19
@ronag
Copy link
Member Author

ronag commented Aug 20, 2019

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

@lpinca
Copy link
Member

lpinca commented Aug 20, 2019

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

@ronag
Copy link
Member Author

ronag commented Aug 20, 2019

@ronag ronag force-pushed the stream-finish-after-destroy branch from 1107841 to c7a82fb Compare August 20, 2019 19:21
@lpinca
Copy link
Member

lpinca commented Aug 20, 2019

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
Copy link
Member Author

ronag commented Aug 20, 2019

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

@addaleax
Copy link
Member

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
Copy link
Member Author

ronag commented Aug 20, 2019

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
Copy link
Member

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.

@ronag ronag force-pushed the stream-finish-after-destroy branch from c7a82fb to 73048fa Compare August 20, 2019 20:00
@ronag ronag force-pushed the stream-finish-after-destroy branch from 73048fa to 8add970 Compare August 20, 2019 20:01
@jasnell
Copy link
Member

jasnell commented Aug 20, 2019

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
Copy link
Member Author

ronag commented Aug 20, 2019

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
Copy link
Member Author

ronag commented Aug 20, 2019

@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
Copy link
Member

Trott commented Aug 21, 2019

@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
Copy link
Member

@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
Copy link
Member

jasnell commented Aug 21, 2019

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
Copy link
Member

Agreed @jasnell

@ronag
Copy link
Member Author

ronag commented Aug 21, 2019

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

@ronag
Copy link
Member Author

ronag commented Aug 21, 2019

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

@ronag
Copy link
Member Author

ronag commented Aug 23, 2019

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

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Aug 23, 2019
@ronag ronag mentioned this pull request Aug 24, 2019
4 tasks
@ronag
Copy link
Member Author

ronag commented Aug 26, 2019

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

@ronag ronag closed this Aug 26, 2019
@ronag ronag changed the title stream: don't emit finish after destroy() [x] stream: don't emit finish after destroy() Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants