Repository navigation
fs.readFile does not partition reads - threadpool exhaustion #17047
Description
Activity
Hey @davisjam thanks for stopping by and making a contribution!
I recommend making a PR if you have a patch already since it's more easy to discuss concrete code changes.
I would love to see some benchmarks detailing the performance characteristics of
readFilenow vs. how it would look like.If you'd like assistance setting up the PR or benchmarks please let us know.
In particular - results for https://github.com/nodejs/node/blob/master/benchmark/fs/readfile.js with and without the patch would be interesting - it might be a good opportunity to add more benchmarks to
readFileanyway.@benjamingr Thanks for the pointer. I'll add some benchmarks to my PR and open it soon.
Reacted by Benjamin Gruenbaum@davisjam great, if you're stuck or not sure how to proceed please feel free to leave a comment and we'll do our best to help you. I'll also ping the libuv and fs teams on the actual PR.
Thanks again and good luck :)
- added a commit that references this issue
on Feb 1, 2018 - added a commit that references this issue
on May 8, 2018
Version: Everything
Platform: All
Subsystem: FS
Description
The node master, and all previous releases AFAIK, implement
fs.readFileas a call to stat, followed by a request to read the entire file based on the size reported by stat.Why is this bad?
The effect is to place on the libuv threadpool a "giant" read request, occupying the libuv thread until it completes. While readFile certainly requires buffering the entire file contents, it can partition the read into smaller buffers (as is done on other read paths) to avoid threadpool squatting.
If the file is relatively large or stored on a slow medium, reading the entire file in one shot seems particularly harmful, and seems to present a possible DoS vector.
Downsides to partitioning?
I don't think partitioning the read like this raises any additional risk of read-write races on the FS. If the application is concurrently readFile'ing and modifying the file, it will already see funny behavior. Though libuv uses
preadvwhere available, this doesn't guarantee read atomicity in the presence of concurrent writes.Related
It might be that writeFile has similar behavior. I didn't check.
PR
I have a patch that partitions readFile. I'm happy to submit a PR if there's interest.