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: support special files in promises.readFile #21497

Closed
wants to merge 3 commits into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jun 24, 2018

A size of 0 may indicate that the file is a Linux special file, which may still have content when read(). Instead of returning early, use the same approach as fs.readFile():

if (this.size === 0) {
buffer = this.buffer = Buffer.allocUnsafeSlow(kReadFileBufferLength);
offset = 0;
length = kReadFileBufferLength;
} else {
buffer = this.buffer;
offset = this.pos;
length = Math.min(kReadFileBufferLength, this.size - this.pos);
}
const req = new FSReqWrap();
req.oncomplete = readFileAfterRead;
req.context = this;
read(this.fd, buffer, offset, length, -1, req);

Additionally, only the return code of 0 indicates end of file, and we should not use bytesRead !== chunkSize as a indication of EOF. Per POSIX:

The value returned may be less than nbyte if the number of bytes left in the file is less than nbyte, if the read() request was interrupted by a signal, or if the file is a pipe or FIFO or special file and has fewer than nbyte bytes immediately available for reading. For example, a read() from a file associated with a terminal may return one typed line of data.

Refs: http://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html
Refs: https://groups.google.com/forum/#!topic/nodejs-dev/rxZ_RoH1Gn0
Fixes: #21331

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@TimothyGu TimothyGu requested a review from jasnell June 24, 2018 05:18
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jun 24, 2018
if (size > kMaxLength)
throw new ERR_FS_FILE_TOO_LARGE(size);

const chunks = [];
const chunkSize = Math.min(size, 16384);
let totalRead = 0;
const chunkSize = size === 0 ? 16384 : Math.min(size, 16384);
Copy link
Member

Choose a reason for hiding this comment

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

might wanna factor that number out into a constant

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@TimothyGu
Copy link
Member Author

@@ -125,6 +125,10 @@ async function writeFileHandle(filehandle, data, options) {
} while (remaining > 0);
}

// Note: This is different from kReadFileBufferLength used for non-promisified
// fs.readFile.
const kReadFileMaxChunkSize = 16384;
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, is there context for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, it was there when I found it so I kept it that way. I just thought it was interesting to note.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we'd have to ask @jasnell about it.

@TimothyGu
Copy link
Member Author

TimothyGu commented Jul 4, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15726/ (linuxone passes after re-run)

TimothyGu added a commit that referenced this pull request Jul 4, 2018
PR-URL: #21497
Fixes: #21331
Refs: http://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html
Refs: https://groups.google.com/forum/#!topic/nodejs-dev/rxZ_RoH1Gn0
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@TimothyGu
Copy link
Member Author

Landed in 5057dd4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants