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.

Sign up for GitHub

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

Jump to bottom

stream: async iterator destroy compat #29176

Closed
wants to merge 1 commit into from
Closed

stream: async iterator destroy compat #29176

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 17, 2019

destroy with callback can achieved using destroy

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

ronag commented Aug 17, 2019

@mcollina is there a reason why it's not done like this instead?

mcollina
mcollina requested changes Aug 17, 2019
View reviewed changes
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.

I’m ok in changing the async iterator. I’m -1 in changing destroy().
The callback in destroy predates adding finished to node core.

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019 •
edited
Loading

How about we keep the destroy callback but consider it deprecated? So we don’t use it in core. A comment maybe?

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019 •
edited
Loading

The biggest issue I have with this is that it's undocumented and monkey patched destroy will break in unexpected ways:

  • async iterator return.
  • monkey patching destroy on fs streams.

There are a few destroy in core that does not provide the cb functionality.

  • IncomingMessage
  • OutgoingMessage
  • ClientRequest
  • Worker (not a stream)
  • Agent (not a stream)

An alternative is to make it documented and part of the public API? Also, I wouldn't mind fixing the once that are streams.

@mcollina
Copy link
Member

The callback property was added because it was needed somewhere in core. Maybe that has changed somehow, but I would rather wait before calling it deprecated.

Fixing the stream iterator thing is important anyway, good spot.

@ronag ronag force-pushed the stream-destroy-cb branch 5 times, most recently from 81f38d2 to f168368 Compare August 17, 2019 10:53
@mcollina
Copy link
Member

Can you add a test with an overridden destroy with no callback?

@mcollina
Copy link
Member

You might want to change the name of the PR and the commit message.

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

Added test

@ronag ronag changed the title stream: remove non public arg from public destroy API stream: async iterator destroy compat Aug 17, 2019
@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

Messages fixed

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019 •
edited
Loading

The callback property was added because it was needed somewhere in core

Yep. I can see it now. Classes that inherit from streams use it sometimes. But it doesn't ever seem to be used outside of the implementation (except for the case with this PR).

We might want to consider refactoring that and calling the method something else hidden behind a symbol.

@ronag ronag mentioned this pull request Aug 17, 2019
Closed
5 tasks
@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

Created a follow up issue #29180

@ronag ronag force-pushed the stream-destroy-cb branch 7 times, most recently from e08ccc1 to 71c4984 Compare August 17, 2019 12:05
mcollina
mcollina reviewed Aug 17, 2019
View reviewed changes
lib/internal/streams/async_iterator.js Outdated Show resolved Hide resolved
lib/internal/streams/async_iterator.js Outdated
const stream = this[kStream];

// TODO(ronag): Remove this check once finished() handles
// already destroyed streams.
Copy link
Member

Choose a reason for hiding this comment

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

Isn’t that part of another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, put this as blocked and wait for that first?

Copy link
Member Author

@ronag ronag Aug 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

it's not exactly the same, #28748 does not check readableEnded which will break this for streams that do not emit 'close' after 'end'. Maybe #28748 should be updated? I'm not sure... I prefer to do this in steps...

@ronag
stream: async iterator destroy compat
98fcb56 
async iterator should not depend on internal API for better compat
with streamlike objects.
mcollina
mcollina approved these changes Aug 22, 2019
View reviewed changes
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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2019
mcollina pushed a commit that referenced this pull request Aug 23, 2019
@ronag @mcollina
stream: async iterator destroy compat
4e188b3 
async iterator should not depend on internal API for better compat
with streamlike objects.

PR-URL: #29176
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member

Landed in 4e188b3

@mcollina mcollina closed this Aug 23, 2019
@ronag ronag mentioned this pull request Aug 26, 2019
Closed
4 tasks
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@ronag @BridgeAR
stream: async iterator destroy compat
e939a87 
async iterator should not depend on internal API for better compat
with streamlike objects.

PR-URL: #29176
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Merged
@gordonmleigh gordonmleigh mentioned this pull request Oct 13, 2019
Closed
@ronag ronag mentioned this pull request Jun 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

@addaleax addaleax Awaiting requested review from addaleax

@benjamingr benjamingr Awaiting requested review from benjamingr

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ronag @mcollina @nodejs-github-bot @jasnell @Trott