file_system: group promisify, avoid relying on in-advance fs.exists checks #112
Conversation
ChALkeR
force-pushed
the
fix-fs-race
branch
3 times, most recently
from
631687c to
aa11bfe
Compare
|
Travis failure in https://travis-ci.org/tensorflow/tfjs-node/jobs/398469237 looks unrelated (seed |
ChALkeR
force-pushed
the
fix-fs-race
branch
3 times, most recently
from
46c846a to
806442d
Compare
|
Review status: src/io/file_system.ts, line 31 at r1 (raw file):
Let's call this Comments from Reviewable |
complete! 1 of 1 LGTMs obtainedNo need to call promisify() per every method call, we can do that once instead and save the result.
This avoids possible race conditions, e.g. when more than one parallel async .save() operation is called (from a single or several processes) sharing a common prefix which does not yet exist. Instead, createOrVerifyDirectory() now just tries creating the dir, and if that fails -- checks _why_ that failed, if it is anything other than the directory already existing -- throws an error, if it's the dir already existing -- skips that and continues. Refs: https://nodejs.org/api/fs.html#fs_fs_exists_path_callback
Instead, use fs.stat and fs.readFile directly and let them throw if the file in question does not exist, and format the error in a way how we want using a small helper function.
|
@nkreeger Updated. That added one line though ;-). |
|
Ah, also, it was This PR changes that to a common variant (without the dot). Tests are happy both ways. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Review status: src/io/file_system.ts, line 121 at r2 (raw file):
I can see this getting confusing on the command line - do we need a name? If so, maybe something like 'Path from load' or 'loading model'. src/io/file_system.ts, line 139 at r2 (raw file):
Ditto here. Comments from Reviewable |
|
@nkreeger I am not sure that I understood. Current ones in this PR are: Did you mean that it's unclear that the error comes from loading a model, and there should be I did not understand |
Again several commits here with their own descriptions.
This removes
fs.existsusage and does some minor cleanup.My intention was to remove the in-advance checks that might result in a race.
This change is