use of graceful-fs causes fs.stat object to not be an instance of fs.Stats on 0.11.13

[sam-github]

nodejs/node-v0.x-archive#7203 introduced a change where fs.Stats is defined in lib/fs.js, and passed to the binding with FSInitialize().

graceful-fs reevaluates the fs source code, causing a new instantiation of a source-identical fs.Stats function to be passed to FSInitialize.

After requireing graceful-fs, the stat object returned by fs.stat and friends will no longer be an instance of require('fs').Stats, but it will be an instance of require('graceful-fs').Stats.

This breaks the node API contract (http://nodejs.org/api/fs.html#fs_class_fs_stats) and causes chaos, generally, see expressjs/express#2351

As far as I can tell, the nastiness of https://github.com/isaacs/node-graceful-fs/blob/master/fs.js is unnecessary.

Why not replace fs.js with module.exports = util._extend({}, require('fs')) ?

Reproduction:

var fs = require('fs');
var g = require('graceful-fs');

var stat = fs.statSync(__filename);
console.log('is fs.Stats?', stat instanceof fs.Stats);
console.log('is g.Stats?', stat instanceof g.Stats);

[dougwilson]

git blame points to the change starting from 08471b2 if it helps

[dougwilson]

And the reason your eval is altering global state in node 0.11.13 is because in your eval, the line https://github.com/joyent/node/blob/v0.11.13/lib/fs.js#L153 is run, which modifies the global binding.

[isaacs]

Hm, interesting.

I suppose we could just extend the fs module, but I'd want to test it pretty thoroughly before publishing.

[isaacs]

Would it be possible to just add a guard in that function in node-core so that it doesn't call multiple times?

[dougwilson]

but I'd want to test it pretty thoroughly before publishing

yes. i don't think there's any hurry here :) in fact, the dirtiest solution is to do the following, lol:

var lib = process.binding('natives').fs.replace(/\bbinding\.FSInitialize\b/, '(function(){})')
var src = pre + lib + post

Would it be possible to just add a guard in that function in node-core so that it doesn't call multiple times?

i.m.o. the only reason it's even like that is because of some weird circular dependency crap. but yes, i would think it should only call once; i don't see any reason why you'd want to call it multiple times. i believe the only reason it is even in js-land is because v8 will optimize the ops into less code than writing the same thing using the c++ api (this is according to the talk about that commit).

[isaacs]

Can you please see if 34e473a fixes the issue?

In your node program, you can type npm install isaacs/node-graceful-fs#34e473a915b6d10379151ce3b8ce591549b35c8a to pull in the change.

[dougwilson]

Yes, it does fix it for my limited testing.

I'm going to ping the following people here who were actually encountering the issue (and so I assume make more use of graceful-fs), hopefully for some feedback on if the change has noticeable effects:
@sam-github @mikebarnhardt @bnoordhuis @cgole @raymondfeng

[sam-github]

@isaacs

Would it be possible to just add a guard in that function in node-core so that it doesn't call multiple times?

You probably noticed, but a guard fixes fs, but breaks graceful-fs by reversing the problem: it causes the stat object to be an instance of fs.Stats, but not to be an instance of gfs.Stats.

The change LGTM, but I've only minimized reproductions.

[cgole]

LGTM.

[sam-github]

So, works for the apps we noticed it with at strongloop (@cgole @raymondfeng @bnoordhuis and me).

@mikebarnhard?

[elado]

Breaks etag, send, serve-static, browser-sync.

pillarjs/send#63

[xmlking]

seams like it is working with Node v0.11.14 without this issue.

[dougwilson]

If you are talking about etag module working, it's because I added a workaround, though it is slow.

[xmlking]

@dougwilson you are right , in my case browse-sync -> serve-static -> send -> etag

[isaacs]

@elado Are you saying that this gfs change fixes pillarjs/send#63, or that it's still broken? (That is, are you +1ing this change, or -1ing it?)

[isaacs]

Published as 3.0.2, since it seems to be a strictly good change that does not affect API at all.

[elado]

@isaacs it was a +1. Thanks.

[dougwilson]

Hi @isaacs , I just wanted to chime in and thank you for coming up with the patch and eventually releasing it. I hope that it doesn't cause any new issues due to some un-forseen issue that it creates. In the end, because of the was the fs module has been changed in 0.11, it helps graceful-fs fulfill the "no global monkey patch" promise :)