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

[v10.x] fs: stop lazy loading stream constructors #21776

Closed
wants to merge 1 commit into from
Closed

[v10.x] fs: stop lazy loading stream constructors #21776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 7 additions & 53 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const {
} = errors.codes;

const { FSReqWrap, statValues } = binding;
const { ReadStream, WriteStream } = require('internal/fs/streams');
const internalFS = require('internal/fs/utils');
const { getPathFromURL } = require('internal/url');
const internalUtil = require('internal/util');
Expand Down Expand Up @@ -91,13 +92,6 @@ let fs;
let promises;
let watchers;
let ReadFileContext;
let ReadStream;
let WriteStream;

// These have to be separate because of how graceful-fs happens to do it's
// monkeypatching.
let FileReadStream;
let FileWriteStream;

const isWindows = process.platform === 'win32';

Expand Down Expand Up @@ -1697,20 +1691,11 @@ function copyFileSync(src, dest, flags) {
handleErrorFromBinding(ctx);
}

function lazyLoadStreams() {
if (!ReadStream) {
({ ReadStream, WriteStream } = require('internal/fs/streams'));
[ FileReadStream, FileWriteStream ] = [ ReadStream, WriteStream ];
}
}

function createReadStream(path, options) {
lazyLoadStreams();
return new ReadStream(path, options);
}

function createWriteStream(path, options) {
lazyLoadStreams();
return new WriteStream(path, options);
}

Expand Down Expand Up @@ -1793,43 +1778,12 @@ module.exports = fs = {
writeSync,
Stats,

get ReadStream() {
Copy link
Member

Choose a reason for hiding this comment

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

An alternative fix is adding setters here to allow users to override the lazy-loaded variables so we can still avoid loading the internal/fs/streams module when requiring fs.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's how it's implemented now. It doesn't work with jest-mock because accessors are ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, I see that there are setters...so this looks more like a bug of Jest?

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, maybe. But this was still kind of a sever major change and I'm not sure it's worth to do it.

lazyLoadStreams();
return ReadStream;
},

set ReadStream(val) {
ReadStream = val;
},

get WriteStream() {
lazyLoadStreams();
return WriteStream;
},

set WriteStream(val) {
WriteStream = val;
},

// Legacy names... these have to be separate because of how graceful-fs
// (and possibly other) modules monkey patch the values.
get FileReadStream() {
lazyLoadStreams();
return FileReadStream;
},

set FileReadStream(val) {
FileReadStream = val;
},

get FileWriteStream() {
lazyLoadStreams();
return FileWriteStream;
},

set FileWriteStream(val) {
FileWriteStream = val;
},
// Stream constructors
ReadStream,
WriteStream,
// Legacy names...
FileReadStream: ReadStream,
FileWriteStream: WriteStream,

// For tests
_toUnixTimestamp: toUnixTimestamp
Expand Down
18 changes: 12 additions & 6 deletions lib/internal/fs/streams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE
} = require('internal/errors').codes;
const fs = require('fs');
const { Buffer } = require('buffer');
const {
copyObject,
Expand All @@ -18,6 +17,13 @@ const { Readable, Writable } = require('stream');
const { getPathFromURL } = require('internal/url');
const util = require('util');

let fs;
function lazyFs() {
if (fs === undefined)
fs = require('fs');
return fs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to just return require('fs') since require already has a cache?

Copy link
Member

Choose a reason for hiding this comment

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

The require cache is slightly more expensive so we generally use this pattern through core to keep it at cheap as possible. It's likely unnecessary in most cases, but it's a consistent pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, cool. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell we might want to benchmark the influence of that again. It would actually be so clean if we'd just use require directly.

}

const kMinPoolSpace = 128;

let pool;
Expand Down Expand Up @@ -92,7 +98,7 @@ function ReadStream(path, options) {
util.inherits(ReadStream, Readable);

ReadStream.prototype.open = function() {
fs.open(this.path, this.flags, this.mode, (er, fd) => {
lazyFs().open(this.path, this.flags, this.mode, (er, fd) => {
if (er) {
if (this.autoClose) {
this.destroy();
Expand Down Expand Up @@ -142,7 +148,7 @@ ReadStream.prototype._read = function(n) {
return this.push(null);

// the actual read.
fs.read(this.fd, pool, pool.used, toRead, this.pos, (er, bytesRead) => {
lazyFs().read(this.fd, pool, pool.used, toRead, this.pos, (er, bytesRead) => {
if (er) {
if (this.autoClose) {
this.destroy();
Expand Down Expand Up @@ -177,7 +183,7 @@ ReadStream.prototype._destroy = function(err, cb) {
};

function closeFsStream(stream, cb, err) {
fs.close(stream.fd, (er) => {
lazyFs().close(stream.fd, (er) => {
er = er || err;
cb(er);
stream.closed = true;
Expand Down Expand Up @@ -242,7 +248,7 @@ WriteStream.prototype._final = function(callback) {
};

WriteStream.prototype.open = function() {
fs.open(this.path, this.flags, this.mode, (er, fd) => {
lazyFs().open(this.path, this.flags, this.mode, (er, fd) => {
if (er) {
if (this.autoClose) {
this.destroy();
Expand Down Expand Up @@ -270,7 +276,7 @@ WriteStream.prototype._write = function(data, encoding, cb) {
});
}

fs.write(this.fd, data, 0, data.length, this.pos, (er, bytes) => {
lazyFs().write(this.fd, data, 0, data.length, this.pos, (er, bytes) => {
if (er) {
if (this.autoClose) {
this.destroy();
Expand Down