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

[x] fs: refactor to fully use Stream api #29048

Closed
wants to merge 1 commit into from
Closed

[x] fs: refactor to fully use Stream api #29048

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 8, 2019 •
edited
Loading

This refactors the fs streams to fully/properly use the existing stream API's and helpers.

No test modified. All fs tests pass.

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

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. labels Aug 8, 2019
@ronag ronag changed the title stream: refactor to use Stream api fs: refactor to use Stream api Aug 8, 2019
@ronag ronag force-pushed the fs-stream-refactor branch 11 times, most recently from 8ea1c0e to 4a10df8 Compare August 8, 2019 16:24
ronag
ronag commented Aug 8, 2019
View reviewed changes
lib/_stream_writable.js Outdated Show resolved Hide resolved
lib/internal/streams/destroy.js Outdated Show resolved Hide resolved
@ronag ronag changed the title fs: refactor to use Stream api fs: refactor to fully use Stream api Aug 8, 2019
@ronag ronag force-pushed the fs-stream-refactor branch 7 times, most recently from 5a163e6 to 8135078 Compare August 8, 2019 17:00
@ronag ronag mentioned this pull request Aug 8, 2019
Closed
4 tasks
@ronag ronag force-pushed the fs-stream-refactor branch 6 times, most recently from 7394b4c to 0487efa Compare August 8, 2019 22:25
@ronag
Copy link
Member Author

ronag commented Aug 12, 2019

As @Slayer95 points out this might be breaking without #29058

@ronag
Copy link
Member Author

ronag commented Aug 12, 2019

@Trott: I think this needs a blocked label.

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Aug 12, 2019
@coreyfarrell
Copy link
Member

Since #29058 is tagged semver-major and is needed for this shouldn't this also be semver-major? That issue aside this will break anything which extends ReadStream with an overridden open method as the override function will be unable to set this[kPending] = false.

@ronag ronag force-pushed the fs-stream-refactor branch from a44f414 to fa10d0d Compare August 14, 2019 16:15
@ronag
Copy link
Member Author

ronag commented Aug 14, 2019 •
edited
Loading

@coreyfarrell: Fixed in way that should maintain compat.

@ronag
Copy link
Member Author

ronag commented Aug 14, 2019

Another problem though (not directly related to this PR) is that errorOrDestroy is internal and not available for stream implementors. @mcollina is that something we need to expose? e.g. in the context of graceful-fs and overriding open() "correctly".

@ronag ronag force-pushed the fs-stream-refactor branch 4 times, most recently from 6632caf to 0a9a759 Compare August 14, 2019 16:20
coreyfarrell
coreyfarrell reviewed Aug 14, 2019
View reviewed changes
lib/internal/fs/streams.js Outdated
this.on('open', () => {
// Do this in listener to maintain compat with e.g.
// graceful-fs.
this[kPending] = false;
Copy link
Member

Choose a reason for hiding this comment

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

this[kPending] = false is not needed before errorOrDestroy()?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I don't think so

Copy link
Member Author

Choose a reason for hiding this comment

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

it exist to check whether 'open' has been emitted or not

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I was wrong. It's needed before. I'm not quite sure what's the best way here...

@ronag ronag force-pushed the fs-stream-refactor branch from 0a9a759 to 0a97fca Compare August 14, 2019 17:21
@ronag
Copy link
Member Author

ronag commented Aug 14, 2019 •
edited
Loading

This will break graceful-fs. See @coreyfarrell's comments regarding kPending. I need some guidance on how to resolve that...

@coreyfarrell
Copy link
Member

This will break graceful-fs. See @coreyfarrell's comments regarding kPending. I need some guidance on how to resolve that...

If this PR will be a semver-major then I'm not sure it needs to be resolved, assuming #29083 or similar is merged so graceful-fs can avoid overriding the stream open methods in node.js 13+. I think it's acceptable for node.js 13 to break graceful-fs 4.0.0 through 4.2.2, assuming we can publish a new 4.x release to fix compatibility without breaking functionality for node.js < 13. In this case I would say that #29050 should be reverted to the original patch which does not allow the open method to be overridden, maybe even initialize ReadStream.prototype.open as a read-only property so anything which tries overriding throws?

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 15, 2019
mcollina
mcollina requested changes Aug 15, 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 with the changes to fs. However I'm not ok with the changes to the stream destroy methods here. Are those already part of #29058?

@mcollina
Copy link
Member

This is semver-major.

@ronag
Copy link
Member Author

ronag commented Aug 15, 2019

I'm ok with the changes to fs. However I'm not ok with the changes to the stream destroy methods here. Are those already part of #29058?

Fair enough. They were required to make this correct. Yes, those changes are already part of #29058. This could land before, but is probably better if it lands after.

@ronag ronag force-pushed the fs-stream-refactor branch 2 times, most recently from d1376d9 to e35a498 Compare August 17, 2019 04:46
@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott no longer blocked

@ronag ronag force-pushed the fs-stream-refactor branch from e35a498 to e1ba8c6 Compare August 17, 2019 04:47
@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Aug 17, 2019
This was referenced Aug 25, 2019
Closed
Closed
@ronag
Copy link
Member Author

ronag commented Aug 28, 2019

closing for now

@ronag ronag closed this Aug 28, 2019
@ronag ronag changed the title fs: refactor to fully use Stream api [x] fs: refactor to fully use Stream api Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Slayer95 Slayer95 Slayer95 left review comments

@coreyfarrell coreyfarrell coreyfarrell left review comments

@mcollina mcollina mcollina requested changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ronag @coreyfarrell @mcollina @Slayer95 @Trott @nodejs-github-bot