-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Node 6 fs.realpath behavior changes #7175
Comments
@isaacs Is this related to npm/npm#5082 fix being blocked? |
Introduced in #3594 |
@thealphanerd citgm tests glob, right? @isaacs Did glob have tests for it previously? |
@Fishrock123 ... the primary issue with glob is two fold: (a) fs.realpath now throws errors when it previously did not and (b) when those errors are thrown is platform specific. Previously, if I use glob with realpath on a circular symlink, it would recurse until a name too long error would occur, which is being caught and intentionally used by glob. This error is essentially the signal that glob should stop recursing and just returns the result up to that point. On certain operating systems (e.g. OSX), the libuv realpath implementation now throws ELOOP errors before the other error is thrown. Since glob does not ignore the ELOOP errors, the app dies. On Linux, the eloop and name too long errors throw at the same depth so the problem is not encountered because the ignored error comes first and the loop inside glob exits before the eloop can occur. On OSX, however, the eloop happens first. The glob code was written to assume that fs.realpath would never throw an errors of it's own (there's no defensive error handling logic around the call to realpath). |
I think glob's assumptions here are ok in terms of errors, fwiw. Edit: As long as you don't pass it obviously invalid parameters |
As a note, the However it does not note this specifically, and it does not mean that there isn't still a problem or bug here. |
Note, I'm not saying that glob's assumptions around error handling are right or wrong, just describing the situation. Previously fs.realpath did not throw, and now it does throw based on platform specific differences. |
It is important to note that other
There is almost no |
@Fishrock123 Yes, glob's tests fail on Node 6. There's a PR from @thealphanerd which makes the test pass, but there are problems with the fix. It can't really be hacked around in node-glob without a very subtle and significant breaking change in node-glob that'll require people to update their code. Imo, citgm should have prevented this change from happening in the first place. It is a bug in the process that it got out into a node release. That underlying process bug should be fixed, along with this technical bug. @jhamhader Prior to node 6, one could use |
Was this behavior documented? Generally speaking, all fs APIs are thing wrappers around their POSIX counterparts, and very thin wrappers at that. IMHO the general expectation is that they work just the man page says. In this line, I consider the current implementation works as expected. FTR, the work done here was to make fs.realpath faster, and keeping the JS fallback was met with resistance back then. We waited for Windows XP support to go away in order to land it, because uv_fs_realpath doesn't work there. The way I see it the only feasible solution here is to catch the error at the application level and handle it.
I understand that a documentation change is acceptable to you then? |
Does this mean that other
Perhaps what we need here need a separate method for just that, either in |
Adequately documenting breaking changes in a platform is the bare minimum requirement. Saying that something "raises new errors", without specifying what those errors are, when they are raised, or that they are platform-specific, is inadequate. It is not a "fix" though, and I'll be disappointed if that's the resolution.
Time and typing. It looks like I'm going to have to fall back to the pre-node6 fs.realpath implementation anyway, I'd just prefer that it be in the platform rather than in my userland code, because I care a great deal about the stability of the Node.js platform and the community built on top of it. Stability is the point of a platform; it's what a platform is for, and breaking stability imposes a high cost. Stuff that was built on that platform falls over. Do not be flippant about that cost. Every time something like this comes up, it imposes many person-hours of work, and reduces confidence in Node.js. Ignoring that cost is user-hostile, especially if the justification is "Well, it was wrong of the user to rely on this thing not breaking anyway". I'm not saying that breaking changes are never justified, but they are expensive. As a platform, Node has a responsibility to make sure the benefit is valuable enough to be worth it. What is the point of making The net impact of stuff like this is that module authors feel the need to scramble and do a bunch of busy-work to catch up to the ways that core has broken their programs. It causes resentment and reduces trust. I'm guessing that this is not the intent of the TSC or CTC, based on what I've been told every time something like this comes up and I complain about it. Citgm is a good step in the right direction. But there needs to be a clearer understanding of trade-offs much earlier in the design process. I don't see that happening. I see my code breaking, and then post-hoc justifications about why it was the right thing to do, based on values that are not clearly articulated and not shared by me or my users. |
I get the impression that you think this was done somewhat deliberately. It was not. The sequence of events was more like: "hey! let's make realpath faster!" "cool, are you up for a libuv patch?" "sure! here you go! and bonus nachos, the Node part as well!" "yikes, that won't work on Windows XP, let's wait until we drop it" "okidoki" The fact that OSX behaves differently with regards to ELOOP was not observed back then, AFAIK. The discussion about keeping the current fs.realpath was not in this context but in the context of supporting Windows XP. The consensus was that having 2 implementations was not a good thing so we waited until Node 6, when XP support was dropped and then the whole thing landed, same code for all platforms. Turns out CITGM was not in full swing (IIRC) so it didn't catch the node-glob problem, so here we are. Now, solutions. From your example above it looks like returning the unchanged path if realpath fails is what you need? Or is there any other case in which it used to behave different? |
I don't think it was done deliberately. I think it wasn't deliberately avoided, and so far I've seen a lot of resistance to what seems like a pretty straightforward and compelling argument to revisit the "two implementations" approach. I also think that the process needs to treat this kind of breakage as a mistake, and correct to avoid the same mistake in the future, rather than treating it as expected and acceptable. I don't need it to return the unchanged path, I need it to return the real path successfully, like it used to. See the test case in the OP in this thread. Falling back to the JS impl is a pretty obvious solution. This issue was raised with node-glob a week prior to node 6 being released, but node 6 was released anyway. isaacs/node-glob#258 Fwiw, this exists now, so whatever https://www.npmjs.com/package/fs.realpath |
I think the fact that we broke a module that got 1.5 million downloads yesterday is a huge problem. Is a revert off the table at this point? |
Not my call, but here is my 2 cents: It would probably be a good idea to keep using |
What's keeping us from falling back to a JS impl for this one case if realpath does not work? |
I have upgrade NodeOS from Node.js 4.x to 6.x and now I'm getting the next (probably related) error: fs.js:1568
return binding.realpath(pathModule._makeLong(path), options.encoding);
^
Error: ENOENT: no such file or directory, realpath '/usr/bin/env'
at Error (native)
at Object.realpathSync (fs.js:1568:18)
at Function.Module._findPath (module.js:167:25)
at Function.Module._resolveFilename (module.js:438:25)
at Function.Module._load (module.js:388:25)
at Module.runMain (module.js:575:10)
at run (node.js:348:7)
at startup (node.js:140:9)
at node.js:463:3 When hardcoding the shebang to fs.js:1568
return binding.realpath(pathModule._makeLong(path), options.encoding);
^
Error: ENOENT: no such file or directory, realpath '/sbin/init'
at Error (native)
at Object.realpathSync (fs.js:1568:18)
at Function.Module._findPath (module.js:167:25)
at Function.Module._resolveFilename (module.js:438:25)
at Function.Module._load (module.js:388:25)
at Module.runMain (module.js:575:10)
at run (node.js:348:7)
at startup (node.js:140:9)
at node.js:463:3 Seems |
@piranna Is /usr/bin/env a binary? Is /bin/node a symlink to /sbin/init? Do you have rights to /sbin? |
On NodeOS, /usr/bin/env is a symlink to /bin/env, and /sbin/init is a |
Hum. Can you strace that? fs.realpath uses realpath(3), which does work with symlinks, so this shouldn't happen. |
How can I be able to do that?
|
@piranna |
General note: I checked the OSX libc and it bails out with ELOOP if it finds more than MAXSYMLINKS symlinks. This value is alas defined as 32 in a kernel header, so we cannot change it even at compile time. Glibc OTOH will return a minimum of 40. Now, question: given that the entire OSX has this limit, have we run into it IRL or just in synthetic benchmarks? |
@piranna strace -f -y the_program.
I'll try to check it on the desktop (NodeOS don't have strace...).
Also, what libc are you using in NodeOS?
musl
|
@saghul I'm currently running into this issue in a project at work. Mocha's watched (based on chokidar) scans the whole project tree which contains a few circular symlinks in our case and causes the watcher to bail out. |
@saghul What I'm doing is splitting the path down and resolving that. then taking the remaining path, attaching it and resolving that. Seems to work just fine. |
Also note that the v4 implementation isn't without it's warts. Take the following:
On Linux that should fail with
|
You mean resolving 32 segments at a time and concatenating results? Sounds like a good idea! |
Quick update: I've tried (without success) to get Windows to work properly. No luck. One clear case where our new realpath implementation fails is with ramdisks like the ones made with ImDisk. For context, we use GetFinalPathNameByHandle with If there is no way around this, I don't see other solution but to revert to the old code. I'll keep digging. |
realpath(3) would fail if the symbolic link depth was too deep. If ELOOP is encountered then resolve the path in parts until the entire thing is resolved. This excludes if the number of symbolic links is too deep, or if they are recursive. Fixes: nodejs#7175
This reverts parts of nodejs@b488b19 restoring javascript implementation of realpath and realpathSync. Fixes: nodejs#7175 Fixes: nodejs#6861 Fixes: nodejs#7294 Fixes: nodejs#7192 Fixes: nodejs#7044 Fixes: nodejs#6624 Fixes: nodejs#6978
What version will include this patch? Next release on 6.X? |
there's a new 6.x version coming out next week but it's not clear yet if this will be included in that. /cc @cjihrig @evanlucas |
It's in the commit list at #8070. |
npm run build error in my existing project i am getting this error
debug log:
how can i fix the problem? |
Previously, looping symbolic links and long path names were handled by fs.realpath without issue. The changelog says that fs.realpath "can throw new errors", but fails to identify that it also is less powerful (and also does not identify what those new errors are).
Seems like it'd be possible to maintain backwards compatibility if Node could fall back to the slower but more resilient implementation when
ELOOP
orENAMETOOLONG
are raised.If this is not desirable, then the changelog should be updated to reflect the reduction in functionality.
I have no strong opinion about whether or not Node is capable of resolving the realpath of long looping symbolic links, but I would very much like a thing to point to when people complain about glob being broken. Thanks.
The text was updated successfully, but these errors were encountered: