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

fs.readFile of directory on IBM i returns "Unknown system error -64" instead of "EISDIR" #25433

Closed
kadler opened this issue Jan 10, 2019 · 8 comments
Labels
aix Issues and PRs related to the AIX platform. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@kadler
Copy link

kadler commented Jan 10, 2019

  • Version: v10.15.0
  • Platform: OS400 wernstrom 2 7 0010000ACBAA Os
  • Subsystem: fs

Since upgrade of libuv to 1.23.2 in #23336, fs.readFile on a directory on the IBM i platform returns "Unknown system error -64" instead of EISDIR. (64 is EOPNOTSUPP on IBM i/PASE). Reproducing the problem is really easy:

node -e "try { require('fs').readFileSync('.') } catch(err) { console.log(err.code) }"

On Linux and macOS, this returns EISDIR, while on IBM i this now returns "Unknown system error -64".

This was changed in libuv/libuv#2025 to allow AIX to read a directory, since it's supported there (as in many BSDs and other OSes) and remove the performance penalty of doing a stat call for each read. Since the PASE environment where Node.js runs on IBM i is an AIX-like environment, this affected our platform as well, but unlike AIX, PASE does not support reading from directories.

The net result of this, is that global npm install are broken, due to gentle-fs assuming that reading from a directory will return EISDIR or ENOTASHIM: https://github.com/npm/gentle-fs/blob/latest/lib/rm.js#L245-L248

So the question is what should happen here? I think at a minimum, err.code should be "EOPNOTSUPP" instead of "Unknown system error -64", but that still wouldn't actually fix the gentle-fs issue. libuv was deliberately changed due to the performance of the stat on each read, while gentle-fs has already done a stat and didn't bother to check if it was a directory first. gentle-fs could add a stat.isDirectory() check prior to calling readCmdShim, but I don't know enough about Windows to know if a cmd shim can be a directory. The other option is to standardize on the EISDIR behavior and convert the EOPNOTSUPP on IBM i to EISDIR, but is then the question is whether that should happen in libuv or in Node.

@sam-github
Copy link
Contributor

@nodejs/platform-aix

@gireeshpunathil just a check, you are in the above group?

@sam-github
Copy link
Contributor

If the only cause of EOPNOTSUPP is that the fd refers to a directory, mapping it to EISDIR is obvious, but what if the fd is something else other than a directory that does not support read()? In this case, EISDIR would be wrong.

Perhaps if read() returns EOPNOTSUPP, it can fallback to the fstat() and return EISDIR if it was a dir, and just pass back ENOTSUPP otherwise? This would move the usually unnecessary fstat from the normal code path, to the failure path.

@kadler
Copy link
Author

kadler commented Jan 10, 2019

@sam-github There are various ways to get EOPNOTSUPP, not just for a attempting to read a directory so a simple mapping is not sufficient. The fallback to the fstat() as you proposed was what I was thinking as well, I just don't know if it's more appropriate to put that in Node or libuv.

@sam-github
Copy link
Contributor

It should be in libuv. I suggest opening a PR there.

@ChALkeR ChALkeR added fs Issues and PRs related to the fs subsystem / file system. aix Issues and PRs related to the AIX platform. labels Jan 13, 2019
@bnoordhuis
Copy link
Member

@kadler Are you working on this? If not, I could revert libuv/libuv#2025 but with s/_AIX/__PASE__/.

@bnoordhuis bnoordhuis added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Jan 15, 2019
@kadler
Copy link
Author

kadler commented Jan 15, 2019

Yes, working on it, though I've been tied up with other things at the moment.

@kadler
Copy link
Author

kadler commented Jan 15, 2019

I've opened a libuv PR here: libuv/libuv#2148

cjihrig pushed a commit to libuv/libuv that referenced this issue Jan 17, 2019
On IBM i PASE, EOPNOTSUPP is returned when reading a directory instead
of EISDIR, like (seemingly) every other platform which doesn't support
reading directories. To ensure compatibility with software expecting
EISDIR, we map the EOPNOTSUPP to EISDIR when the fd passed in was a
directory.

This is a partial revert of 25a3894, but scoped to PASE and the fstat
call is moved to the end so it's out of the fast path.

Refs: nodejs/node#25433
Fixes: #2147
PR-URL: #2148
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@richardlau
Copy link
Member

This was fixed in libuv 1.25.0.

njlr pushed a commit to buckaroo-pm/libuv that referenced this issue Apr 5, 2019
On IBM i PASE, EOPNOTSUPP is returned when reading a directory instead
of EISDIR, like (seemingly) every other platform which doesn't support
reading directories. To ensure compatibility with software expecting
EISDIR, we map the EOPNOTSUPP to EISDIR when the fd passed in was a
directory.

This is a partial revert of 25a3894, but scoped to PASE and the fstat
call is moved to the end so it's out of the fast path.

Refs: nodejs/node#25433
Fixes: libuv#2147
PR-URL: libuv#2148
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aix Issues and PRs related to the AIX platform. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

No branches or pull requests

5 participants