Repository navigation
[meta] realpath issues in v6 #7726
Description
Activity
/cc @nodejs/collaborators
Are the proposed fixes that are currently open workarounds or proper solutions? Also, could you link to them just like you did the issues so we can have a look into them?
Why was this tagged
ctc-agendaagain?@ronkorving they are linked on the issues themselves AFAIK.
I have a fix for the linux ELOOP issue. It makes sense to me that we keep that implementation around. Performance is noticeably better (been taking advantage of this recently in an app that's getting the realpath of > 50,000 files as quickly as possible).
Reacted by Sindre Sorhus@trevnorris Unfortunately, that's not enough. It's unacceptable IMHO that Windows people can't even run a hello world file if it's on one of those weird devices or network shares. I'm not opposed to some
_realpathmethod somewhere, for those who want it though.Reacted by Sindre Sorhus@saghul What would you think of implementing #7559 in more general way that would cover all of the Windows problems listed above? It is currently based on bailing out to the original JS implementation when encountering an error in the libuv-based
realpath(currently, only upon certain kinds of errors, but it’s definitely written in a way that would make it easy to add more conditions).That way forward has come up there and in #7175; it should work consistently and retain the performance gains in the most common (by far) cases.
Thanks for this @saghul, it's helpful to have a roll-up like this and I'm very much inclined to view it in the same way as you at this stage. Without satisfactory resolution for Windows users we're stuck in a corner, regardless of progress papering over
ELOOP.I see two alternatives here:
- Revert entirely (I'd actually +1 the re-addition of
cache, it shouldn't have been removed without a deprecation cycle in the first place, especially when it was replaced with an argument of the same type!) - Revert for Windows and apply @trevnorris' fixes for everything else. Or something like that, perhaps as per win, fs: fix realpath behavior on substed drives #7559 as @addaleax says above.
What is the libuv stance on its new realpath implementation? Given that this issue is an admission that it's essentially broken (for now), is it going to stay around? Does it make sense for libuv to continue shipping something that is known broken for Windows?
- Revert entirely (I'd actually +1 the re-addition of
Given that this issue is an admission that it's essentially broken (for now), is it going to stay around?
Or maybe the other way around: Would you accept a port of whatever solution ultimately ends up in Node to libuv? It’s something I’d definitely be interested in if you think it makes sense.
What is the libuv stance on its new realpath implementation? Given that this issue is an admission that it's essentially broken (for now), is it going to stay around? Does it make sense for libuv to continue shipping something that is known broken for Windows?
Given our versioning scheme, we can't remove it. Now, I also wouldn't consider it broken (on Unix): macOS and some BSDs have this systemwide 32 symlinks limit and nobody complains about it except the Node ecosystem, so one could argue Node was abusing this. And I would agree with that. This can be fixed, however, by taking the realpath(3) implementation in OpenSSH-portable (for example) and removing the limit. On Linux we could even copy the musl implementation, which is really simple and elegant.
But! Windows.
Or maybe the other way around: Would you accept a port of whatever solution ultimately ends up in Node to libuv? It’s something I’d definitely be interested in if you think it makes sense.
Certainly.
On Windows there is no realpath(3), so we are trying to emulate it, albeit without much success. It's possible that a way to solve all this is found later on, in which case we could go back to libuv's implementation, but know what to test for. That's partially the reason why I suggest we keep the current API, so swapping the implementation is possible. Also, those who know they are not affected by any of these shortcomings could use some "node-uvrealpath" addon and monkeypatch fs.realpath for their purposes.
Revert entirely (I'd actually +1 the re-addition of cache, it shouldn't have been removed without a deprecation cycle in the first place, especially when it was replaced with an argument of the same type!)
The cache allowed for some very bizarre and unexpected path substitutions, IIRC that was the reason for its removal. If nobody misses the cache now, I argue we are ok without it. Since the JS version needs a cache, however, we will have to have one, but not as part of the API, IMHO.
29 remaining items
We document the differences and leave it at that. Fallbacks get messy.
On Tuesday, July 26, 2016, Myles Borins notifications@github.com wrote:
@jasnell https://github.com/jasnell is there a clear way that we can
signal to individuals all the problems associated with this implementation?
Should we implement the fallback to the original implementation on failure?—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7726 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eTNGjKpKyB8kOmP82FPWPg_q2cJtks5qZkFSgaJpZM4JMKMc
.The key challenge with that is that there may be developers who have since come to depend on the new impl. I'd much rather not drop it entirely.
There is nothing on the new implementation that can be depended upon. Unless being broken is such thing. I say this as someone who has followed this since the start (I reviewed the libuv PR) and came to the conslusion that we currently have no other reasonable choice.
Exposing both is not problematic if we have a clear deprecation path.
There is no clear path. Exposing something with the intention to deprecate it sounds like a waste of time IMHO.
Reacted by Anna Henningsen@thealphanerd please let's not have 2 implementations each broken in a different way, it's just a recipe for dissaster.
I was hoping the last CTC call would have suggested some clear course of action, but it seems we are all as confused as we originally were.
There is no alternative to reverting which fixes all the problems without adding back the original implementation, it's the only viable way forward at this point. We can revisit later. Git archeology suggests the only reason for replacing the implementation was performance. If users could live with it until Node v6 they can certainly live with it a bit longer.
Reacted by Myles Borins, Anna Henningsen and Alexis CampaillaI agree that reverting is necessary. The new implementation simply has caused too many breakages, some of which are not just acceptable behavior changes but rather nasty bugs.
Fallbacks get messy, and do double the surface area for bugs and other behavioral differences. It doesn't sound like a desirable approach IMO.
On Windows, the list of bugs is so bad that we have no choice but reverting to the JS implementation. If you think that the Unix libuv implementation is in a better state, we could consider a Windows-specific revert to JS, and keep the libuv implementation for Unix. Note that this doesn't increase the bug surface area with respect to the current implementation, because the libuv implementation is different on Unix vs Windows. It does however leave room for inconsistencies between platforms, so my preference would be to do a full revert to JS on all platforms, and consider ways to improve performance later on.
Reacted by Saúl Ibarra Corretgé@orangemocha Agreed, I reached the same conclusion. When I mentioned the bug surface I meant from Node's perspective, considering what libuv internally does an implementation detail. At any rate, it's good to see we are on the same page.
Here's a PR to revert: #7899
There is one additional consideration for reverting the realpath implementation in that the new implementation allows an
encodingoption to be passed to get the path back as aBufferrather than a string. While it's unlikely that has extensive use by this point in time, it's still technically a major change to pull that back out.Btw, something that would probably be incredibly helpful to have after the revert are regression tests for all the issues we’ve been encountering… /cc @nodejs/platform-windows
Reacted by Rich TrottGiven that the ultimate purpose of
realpath()ing module paths is to avoid loading the same module twice, I wonder if there is a completely different solution to this; what about instead of using the path as the cache key, use the [std_dev, st_ino] pair?Thanks @saghul and everyone for addressing this.
Overview
There are a number of problems related to the realpath change in #3594. This issue aims to serve a single entry point to those, with the hope that we have a clear picture and are able to address them one way or another.
Please: Do not report new issues here, this is a collection of the currently existing ones.
Current state
Bad. Some of these issues have PRs addressing a specific issue, but none fix all of them.
Original reasoning
Following the issue trail I can only see performance as the argument for creating
uv_fs_realpathand using it in node:Way forward(?)
After scratching my head for a while I have no other suggestion than to revert back to the JS implementation. Since we changed the API to remove the
cacheargument, which was needed for performance, I suggest we keep the cache private, thus avoiding yet another API change.I'll update the libuv documentation so nobody tries to bring this up again in Node v103.x, or at least not without validating it solves all the aforementioned problems.
Suggestions / alternatives are more than welcome, but at this point it's all or nothing, we either have a plan for fixing all the problems or we revert, IMHO.