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

fs: allow overring fs for streams #29083

Closed
wants to merge 9 commits into from
Closed

Conversation

@ronag
Copy link
Member

ronag commented Aug 11, 2019

This would allow modules such as graceful-fs to gracefully implement alternative fs methods for fs streams.

Would also make it possible for user land modules to implement alternative file systems (e.g. WebDAV, HTTP, SMB etc...) while re-using the existing streams implementation.

Refs: #29050

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 ronag mentioned this pull request Aug 11, 2019
4 of 4 tasks complete
@ronag ronag force-pushed the nxtedition:fs-stream-fs branch 2 times, most recently from e7563b2 to 4846e3f Aug 11, 2019
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 13, 2019

We have been moving in the opposite direction for many apis... Making them more difficult or impossible to monkey patch for both maintenance and security reasons. I'm not sure this pr is the right approach. We should revisit or strategy around this and build better hook points that are safer and supportable.

Copy link
Member

jasnell left a comment

Making the -1 explicit until this can be discussed further

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Aug 13, 2019

@jasnell: I get your point but I think I disagree. This is more about composition than encapsulation. We are allowing the user to pass a public API interface. For me this is no different than letting users create their on read and write streams using Readable and Writable.

e.g. graceful-fs, rather than rolling their own implementation they chose to monkey patch. Which is something we should discourage, but still we need to provide reasonable alternatives.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Aug 13, 2019

I'll fix @addaleax comments once there is consensus.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Aug 13, 2019

@jasnell I don’t think it qualifies as monkey-patching if we provide a public API for overriding methods. In my opinion this is exactly the kind of approach we should take in order to avoid monkey-patching.

/cc @nodejs/tsc

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Aug 13, 2019

I’m not convinced that there is so much code in WriteStream or ReadStream that can’t easily be duplicated in a module, if one desired so. I’m unsure if this would solve the graceful-fs problems or not.

@coreyfarrell

This comment has been minimized.

Copy link
Member

coreyfarrell commented Aug 13, 2019

Right now graceful-fs creates new ES5 object constructors which create objects that extend fs.ReadStream and fs.WriteStream with an overridden open method to enable retry on ENFILE/EMFILE. Even this little change means that the overridden open method can (does) diverge from the node.js method. The native WrtieStream in node.js 5.5.0+ supports an autoClose: false option to prevent stream.destroy() from being called when fs.open has an error. graceful-fs currently performs an unconditional stream.destroy() upon error from fs.open in WriteStream.prototype.open. I think completely re-implementing the fs streams code in graceful-fs would lead to more chances of undocumented differences from native fs. Obviously graceful-fs is meant to be different from native fs, but only in the documented ways.

This change would allow graceful-fs to skip re-implementing the stream open methods if run under node.js 13+, it would be able to just inject the fs option to the constructor instead.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 13, 2019

As opposed to overriding like this or monkeypatching, what I'd prefer to see is the improved ability for developers to create their own alternative implementations using supported lower level APIs... Something more along the lines of...

class MyAlternativeWriter extends fs.FileWriteBase {
  open() { /* ... */ }
  close() { /* ... */ }
  /* ... */
}

While the differences in the approaches may be subtle, this would allow anyone inspecting the object and seeing the MyAlternativeWriter object name to know that they are working with something that operates differently from fs.FileWriter.

lib/internal/fs/streams.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the nxtedition:fs-stream-fs branch 3 times, most recently from aa36717 to 752a5b2 Aug 14, 2019
@ronag ronag force-pushed the nxtedition:fs-stream-fs branch from 752a5b2 to 12c3893 Aug 14, 2019
Copy link
Member

addaleax left a comment

Ftr, I liked the previous approach (passing an fs object), but it doesn’t make a huge difference anyway.

doc/api/fs.md Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the nxtedition:fs-stream-fs branch from 12c3893 to 0869dd0 Aug 14, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Aug 14, 2019

Ftr, I liked the previous approach (passing an fs object), but it doesn’t make a huge difference anyway.

I'm fine with either...

package-lock.json Outdated Show resolved Hide resolved
@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Aug 14, 2019

@jasnell I don't think this is monkey-patching, it is introducing an API so that monkey-patching wouldn't be used. That API would be documented and its usage would be visible. Also, we could just forbid this call with policies or harden method later, security-wise.

In general, I am in favour (if this would help to minimize monkey-patching), but I am not sure about the API yet and I don't think that open/read/close is enough here? But that's not a blocker.

@ronag ronag force-pushed the nxtedition:fs-stream-fs branch from 0869dd0 to 8e47c7e Aug 14, 2019
@coreyfarrell

This comment has been minimized.

Copy link
Member

coreyfarrell commented Aug 14, 2019

Ftr, I liked the previous approach (passing an fs object), but it doesn’t make a huge difference anyway.

I do agree with @jasnell that having the stream internally store references to the individual methods is best so the stream ignores any monkey-patching done after construction but I think it would be simpler for the public API to just accept a single fs option. I'm not stuck on this one way or the other just sharing my preference.

With the fs option you could still test with the following:

fs.createReadStream(fn, {
  fs: {
    open: common.mustCall(fs.open),
    read: common.mustCallAtLeast(fs.read, 1),
    close: common.mustCall(fs.close)
  }
});
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 12, 2019

@Trott: This looks ready to land?

lib/internal/fs/streams.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 13, 2019

Sorry, I made a git mistake with the last one. Can we start another CI?

@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 13, 2019

@ronag Linter is still failing…

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 13, 2019

How do I run the linter without having to rebuild the whole thing?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 14, 2019

How do I run the linter without having to rebuild the whole thing?

If it's just JavaScript linting, make lint-js (or vcbuild lint-js on Windows) shouldn't require a full rebuild, I don't think.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Dec 18, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 18, 2019

(Whoever lands this should edit the commit first line for typos since those end up displayed prominently in the change log files.)

@nodejs-github-bot

This comment has been minimized.

Trott added a commit that referenced this pull request Dec 18, 2019
Allow overriding open, write, and close when using createReadStream()
and createWriteStream().

PR-URL: #29083
Refs: #29050
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 18, 2019

Landed in 2b9847c.

@Trott Trott closed this Dec 18, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
Allow overriding open, write, and close when using createReadStream()
and createWriteStream().

PR-URL: #29083
Refs: #29050
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/REPLACEME
description: The `fs` options allow overriding the used `fs`
implementation.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 7, 2020

Member

Late nit: it would be ideal to keep the versioning order the next time.

This comment has been minimized.

Copy link
@ronag

ronag Jan 7, 2020

Author Member

versioning order?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 7, 2020

Member

The changes entry should be at the top but it's at the bottom of the changes.

This comment has been minimized.

Copy link
@ronag

ronag Jan 7, 2020

Author Member

Ah, my mistake. I though it was the other way around.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 7, 2020

Member

There is no clear rule for it so far. So it's different each time and it's just good to keep it consistent in the changes itself. We might want to define an order at some point.

BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.