fs.copy() incorrect async behavior

[malinges]

fs.copy() seems to have a incorrect async behavior. In some circumstances, the callback is called somewhat randomly: it may be called once, but also twice, three times, or even never.

Here is a basic working example of this issue: https://gist.github.com/malinges/c9d48ffaf8fa940e4df3

This behavior showed up while I was using grunt-docco, which wraps docco, which itself uses fs-extra. I noticed some files weren't copied properly with fs.copy(), in a purely random fashion (files were either correctly written, empty, or missing).
See the original issue here: DavidSouther/grunt-docco#40.

Let me know if you need more info.

[DavidSouther]

Might also be ncp, I'll rerun @malinges gist against that.

[DavidSouther]

Using https://gist.github.com/DavidSouther/77db8d2ce83c98521082 this is definitely in ncp.

% cd ~/devel/tmp/grunt_docco_issue_40_ncp/77db8d2ce83c98521082
% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }
done: { '0': null }

% node copytest1.js
done: { '0': null }
done: { '0': null }

% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }

% node copytest1.js
done: { '0': null }
done: { '0': null }

% node copytest2.js
done: { '0': null }

% node copytest2.js

% node copytest2.js

% node copytest2.js

% node copytest2.js

%

I opened this in AvianFlu/ncp#71

[robogeek]

I'm seeing this behavior as well in AkashaCMS.

[adam-lynch]

+10!

Must be in ncp because it still happens if I use ncp instead.

Is there any way to workaround this? What can I do if the callback is called but the file is not copied (fs.existsSync(dest) returns false)? @DavidSouther do you have any workaround?

[DavidSouther]

@adam-lynch I've been doing more explorations; this is an issue only when the target directory is not empty.

[DavidSouther]

See AvianFlu/ncp#75

[jprichardson]

Ugghhh... this is nasty. I'm considering adding a file walker into fs-extra and building a new copy on top of that...

[robogeek]

I've rewritten my code in AkashaCMS to use glob to gather the list of files, then copy them one by one, making directories as needed.

See: https://github.com/robogeek/akashacms/blob/master/lib/globcopy.js

I wrote it to be pretty generic and of course there could be some options given - such as whether to duplicate file ownership or permissions.

For reference, it's called from the copyAssets function in https://github.com/robogeek/akashacms/blob/master/index.js

Would that code be of any use to you?

[jprichardson]

Yeah, it should be a nice reference, thank you.

The plan moving forward is for me to include a file walker and then build copy() on top of that. Ultimately, mkdirs and remove() will be as well, but that's further on down the line. Glob functionality would be awesome as well, but that's even further on down the line.

I've started by including the tests from ncp as seen here: https://github.com/jprichardson/node-fs-extra/tree/master/test/ncp as the more test coverage we have, the better. If anyone is willing, it'd be helpful to know why the "skipped" (https://github.com/jprichardson/node-fs-extra/blob/master/test/ncp/ncp.js#L175 and https://github.com/jprichardson/node-fs-extra/blob/master/test/ncp/ncp.js#L184) are failing.

[adam-lynch]

@jprichardson do you have a rough ETA on your rewrite? Sounds like it would take awhile

[jprichardson]

@jprichardson do you have a rough ETA on your rewrite? Sounds like it would take awhile

Unfortunately, no. I think it will take awhile. The first step is to build a walker, probably a hybrid of https://github.com/daaku/nodejs-walker and https://github.com/thlorenz/readdirp

[robogeek]

FYI - https://www.npmjs.com/package/globfs

[jprichardson]

OK, I've forked ncp into fs-extra. Will be looking into this next...

[jprichardson]

This has been fixed. Unfortunately, I don't have an automated test to reproduce it. You can verify that this fix does indeed work by doing the following:

Clone the gist from @DavidSouther above. Create a bash script with the following:

name it: runtest

#!/usr/bin/env bash

node copytest1.js
echo ""

then in bash run:

while ./runtest; do :; done

You'll see a bunch of clustered:

done: { '0': null }
done: { '0': null }

The behavior that we want is:

done: { '0': null }

done: { '0': null }

done: { '0': null }

done: { '0': null }

basically indicating that a callback was only called once.

Ok, so now that verifies that [email protected] is broken. Now to substitute fs-extra by modifiying:

copytest1.js:

from:

require("ncp").copy("node_modules/docco/resources/parallel/", "parallel", function(err, res) { console.log("done:", arguments); });

to

require("fs-extra").copy("node_modules/docco/resources/parallel/", "parallel", function(err, res) { console.log("done:", arguments); });

Checkout git commit a9b98ee

then run:

while ./runtest; do :; done

you'll see the same behavior as ncp.

Finally checkout this latest commit: 35345a3

then run:

while ./runtest; do :; done

You'll see the expected behavior.

I'd love it if anyone could submit a test for this. I've been staring at this code for too long today. Glad it's finally fixed though :)

[overlookmotel]

Has this not been fixed in ncp? Seems like a serious bug and something the maintainers would want to do something about...

But nice one @jprichardson for fixing it in fs-extra. Can your fix also patch ncp?

[jprichardson]

Has this not been fixed in ncp?

Sadly it hasn't.

Seems like a serious bug and something the maintainers would want to do something about...

Totally agree.

Can your fix also patch ncp?

Yes it's just simply: 35345a3. But the ncp maintainers have shown that they're no long interested in maintaining ncp and that's why I felt forced to bring the ncp code into this module.

[overlookmotel]

What a ball-ache! Seems like ncp is depended on by a lot of other modules.

[overlookmotel]

Did you try issuing a PR on ncp?

[jprichardson]

Did you try issuing a PR on ncp?

No, because they've been ignoring all of them within the last two months. So I figured there wasn't much of a point.

[overlookmotel]

Fair enough! It's a shame. Least they could do is put something in the README saying the module is no longer maintained.

To my eyes, ncp's coding style is pretty convoluted, so I'm not entirely surprised this kind of bug has crept in.

[sdnetwork]

For me the problem appear in ncp when options.modified have been added
AvianFlu/ncp@85cd50a

so it appears since the ncp 1.0.0 version, if i have understand the problem.
but in my project i use the 0.5.0 version, and today i have the same behaviour some file are empty, not copied.

i can't reproduce but it is just to ask if you are sure that fix this problem :

I noticed some files weren't copied properly with fs.copy(), in a purely random fashion (files were either correctly written, empty, or missing).

[jprichardson]

@sdnetwork does it work with [email protected]?