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

Fix fs.Stats binding #42

Closed
wants to merge 2 commits into from
Closed

Conversation

etiktin
Copy link
Contributor

@etiktin etiktin commented Jun 6, 2015

This commit fixes issue #35

@etiktin
Copy link
Contributor Author

etiktin commented Jun 6, 2015

Prior to node v0.11.13, fs.Stats was set to a constructor implemented in the C++ binding.
In newer versions fs.Stats is defined in js and a reference to the constructor is passed to the binding (see: nodejs/node-v0.x-archive@e9ce8fc).
When we initialize gfs we eval the fs source code and therefor redefine Stats and pass the new reference to the binding. So fs.Stats is no longer the constructor used by the binding which results in errors like:
fs.statSync(path) instanceof fs.Stats === false

It wasn't an issue prior to the 3.0.4 release because before that version gfs was created by cloning fs and replacing/extending just what was needed (monkey patching) instead of using eval.

To fix it, straight after the eval, we reset the binding to use fs.Stats and set gfs.Stats to fs.Stats.

It's also possible to implement it by manipulating the fs source before passing it to the vm (e.g. replace fs.Stats = with fs.Stats = module.exports.Stats || and pass fs.Stats to fn through module.exports), but it's arguably harder to follow/maintain.

@isaacs, what do you think?

@etiktin etiktin changed the title Fix Stats binding Fix fs.Stats binding Jun 6, 2015
@isaacs
Copy link
Owner

isaacs commented Jun 6, 2015

Monkey-patching the core fs global is a huge problem. That's why it was switched.

Evaluating the internal code is also raising a lot of issues.

#41
nodejs/node#1898

The solution isn't obvious, and I'd like to figure out how we defer EMFILE errors effectively (without falling over with streams etc or falling out of sync with node's fs) before diving too deeply into this relatively minor edge case.

@isaacs
Copy link
Owner

isaacs commented Sep 27, 2016

Fixed already on latest releases.

@isaacs isaacs closed this Sep 27, 2016
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.

2 participants