Main callback gets potentially called before all data is written

AvianFlu / ncp

Asynchronous recursive file copying with Node.js.

MIT License

681 stars 103 forks source link

Main callback gets potentially called before all data is written #31

Closed ladislavhorky closed 10 years ago

ladislavhorky commented 11 years ago

Hello,

I see the callback gets called after

readStream.once('end', cb);

which potentially leads to the situation when callback gets called before all writes are complete. Shouldn't this:

writeStream.once('finish', cb);

be used instead?

The potential problem may never occur with some sane file-system and stream implementation but it still comes as a bit dangerous to me.

I have seen the first construction (wrong in my opinion) used in many projects, so if I am wrong go on and educate me please.

AvianFlu commented 11 years ago

This was written using node 0.8 ReadStream semantics. What you're saying is probably true for 0.10 streams, which I've made no moves to add support for yet.

I'm happy to take a pull request for this, provided steps are taken to ensure 0.8 compatibility.

ladislavhorky commented 11 years ago

Thanks for explanation, I'll happily do a pull request covering this and probably some (0.10-ish ?) error checking on read and write streams* as soon as I get some time to test the functionality and compatibility.

now it seems that if space runs out during the copy (or some other r/w error occurs), the user won't know about it.

2013/9/3 Charlie McConnell notifications@github.com

This was written using node 0.8 ReadStream semantics. What you're saying is probably true for 0.10 streams, which I've made no moves to add support for yet.

I'm happy to take a pull request for this, provided steps are taken to ensure 0.8 compatibility.

— Reply to this email directly or view it on GitHubhttps://github.com/AvianFlu/ncp/issues/31#issuecomment-23725120 .

AvianFlu commented 10 years ago

This should be fixed by #32