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

copySync, moveSync: remove extra step in reading loop #553

Merged

Conversation

kdziamura
Copy link
Contributor

By some reasons on our environment fs.readSync from node 8.x throws error when try to read data from position which more than size of file. Here is the test script with logs in comments:
https://gist.github.com/wontem/da867f66324e1cbe712c717b5fefb944

Since we start using node 8.x and fs-extra 5.x - the issue is disappeared (fs-extra uses copyFileSync from node) but with node 8.x and fs-extra 3.0.1 the issue is reproducible.
While investigating the problem I've noticed that the method copySync from fs-extra 3.0.1 uses extra step in its read loop. To be more safe I want to suggest remove this step from fallback logic for copySync and for moveSync in fs-extra 5.x

@kdziamura kdziamura changed the title Remove extra step in reading loop copySync, moveSync: remove extra step in reading loop Feb 26, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 85.565% when pulling 7e2aba0 on wontem:fs-readSync-throws-error into a0d44c1 on jprichardson:master.

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 26, 2018

I'm not understanding exactly why these changes are needed and what this fixes?

let pos = 0

while (bytesRead > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RyanZim bytesRead will be greater than zero right after the file is copied.
At this moment pos === srcStat.size and the next line will call fs.readSync with this pos as argument for position.
On my environment node throws error "ENOENT" if try to read file from position which greater than or equal to its size.

Copy link
Collaborator

@JPeer264 JPeer264 left a comment

Choose a reason for hiding this comment

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

LGTM

@jprichardson
Copy link
Owner

Nice work. Is it possible to get a test in place for at least one of these cases? Thanks.

@kdziamura
Copy link
Contributor Author

@jprichardson Cases I've mentioned are just for environment on our project and they already covered with existed tests. This MR is just for removing extra step in loop.

@jprichardson jprichardson merged commit b7aa7e3 into jprichardson:master Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants