[x] fs: refactor to fully use Stream api

[ronag]

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

[ronag]

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

[ronag]

@Trott: I think this needs a blocked label.

[coreyfarrell]

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]

@coreyfarrell: Fixed in way that should maintain compat.

[ronag]

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".

[coreyfarrell]

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

[ronag]

no, I don't think so

[ronag]

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

[ronag]

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

[ronag]

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

[coreyfarrell]

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]

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]

This is semver-major.

[ronag]

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]

@Trott no longer blocked

[ronag]

closing for now