-
Notifications
You must be signed in to change notification settings - Fork 281
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
Stumped on error with requiring ../encodings #118
Comments
I just encountered this in an expressjs application after upgrading from node 4.2 to 6.6.0. After upgrading node, I removed my node_modules folder and ran a fresh 'npm install'. First startup seemed healthy until posting a form to the server where I saw this issue. As mentioned by @leanderlee restarting the service seemed to fix it. |
Still cannot reproduce it :( Tried the upgrade method you described and it worked without any errors.. Let me know if you're able to consistently reproduce it. |
Issue is due to changing the current working directory of node.js process after process starts and then you attempt to perform method using I think I found the solution. Try changing:
to
https://github.com/ashtuchkin/iconv-lite/blob/master/lib/index.js#L61 |
I can reliably recreate this error in our test environment, we deploy node and mode_module updates packaged into an rpm, and in our latest version, our code is crashing on body parsing due to its use of this library. restarting our node service fixes this issue, it only happens right after we deploy the rpm. |
@dbussert Did you see my comment? |
Guys I need your help here. I don't have an environment to reproduce it, so I'm counting on you to isolate the exact requirements for this to happen. I'm also not fond of changing that line to |
This isn't something which requires you setup a reproducible environment to isolate or fix the bug. It seems to be simple. Looking at https://github.com/ashtuchkin/iconv-lite/blob/master/lib/index.js#L61 , we can see that we have a "lazy require" statement. In other words, we have a require statement which isn't being executed until In the case I hit ( a production service serving millions of requests ), the Node.js process was being Is there a reason that Also, you could probably replicate the bug using https://www.npmjs.com/package/chroot library. |
Thanks for the extended explanation, @Marak.
It looks to me that generally, it's not safe to do a chroot of a node process, because node's module caching has unexpected results for dynamic requires. Are you sure other libraries/modules don't use it? It's a perfectly valid mode of loading modules in node.js, as far as I know.
Yes, it's an explicit optimization of startup time. Encodings tree is large and can take several hundred ms to load. I don't want to impose this tax on all projects indirectly requiring iconv-lite because they might not use (or even know about) it at all. I still don't know why it works the second time for @dbussert - module caching should not depend on that. |
Lazy loading of Node.js modules is not a best practice and can cause several issues. You should understand that Regardless, I understand the need for optimization on start time. It's a trade-off you have to make. Based on what I see here, the majority of users will never experience this require failing issue and will benefit from the optimized start time. HOWEVER, some users are experiencing this issue and I'm telling you exactly why it's happening. You should either figure out a fix, or tell us you will not fix it. I already proposed the solution we used in production. What none of us need here is reasons why you don't think this is broken. Cheers. |
Dear @Marak, I'm yet to find evidence that others are experiencing the same issue as you are. All the other people in this thread have a situation where a restart "fixes" the problem, which should not happen with a chroot. Also I haven't seen any mention of chroot other than from you. So, I'm still gathering information about that case and would prefer you stop interfering and going personal about it. There's no need to preach about best practices, tell me what I should do, or talk like you represent all the other people in this thread. As for your particular problem - no, I won't fix it for you. The core reason you have this problem is you built a system based on a subtly incorrect assumption that the node module system works correctly after a chroot. This subtle incorrectness showed itself when you started using iconv-lite, but it has potential to show for other modules as well. You cannot assume that other modules won't use a valid system interface because you think it's "not a best practice". I'm glad you found a workaround for your particular system, but 1) it's not guaranteed to work for other people, and 2) you'll need to find similar workarounds for other libraries/modules. I would've recommended to stop relying on the assumption above and try to fix your system in a more general way, like using a separate process to do the chroot and only start your original node process after that, but I don't have enough context about your system. To be clear, I'm not being stubborn about making the change you suggested. I'm not doing that because:
|
Have you considered what is going to happen if someone is running a webserver with 1000s of requests per second and then I hit this problem ( of not being able to find encodings file ), found the solution, and posted it here. You want to lazy require a huge file with a relative path, go for it. Every-time anyone hits any environment issue with |
The lazy loading also seems to be causing an issue in other libraries... jestjs/jest#2605 |
I'm experiencing the same issue when executing a script within my sandboxed test environment (Jest) which uses When I'm not lazy-loading The initial reason for lazy-loading this module was because of speed issues, which were fixed in PR #26. I ran this script twice (both using lazy-loading and without) and got the following results: With lazy-loading:
Without lazy-loading (changed
The results seem to be exactly the same. I'm not sure if something changed within Node itself, or if I'm doing something wrong, but if this those stats are correct there would be no reason to lazy-load the encodings anyway. |
Still experiencing this problem. I am running inside a Docker environment. |
Running |
2019/12 same problem here |
Facing this issue with Electron and the asar package format, used the patch-package package to disable lazy-loading, and everything is now working. |
@octalmage there are several different problems discussed in this issue, could you provide more info about your setup to help me understand what's happening? Thanks! |
I've been reviewing this thread again since it popped up in my feed. Re-reading my old posts I can see the language and tone I was using wasn't the best for collaboration. If you'd like to get this issue closed I believe all the information needed is here posted above with my comments and other user's comments. I would consider removing the lazy loading entirely or make it a configurable option which is disabled by default. |
This issue happens whenever I try to POST JSON data to the server. It's in chroot jail, but removing that does not fix it. Nor does reinstalling node_modules or restarting the server. Express goes to decode the payload and fails to lazy load the encoding module. This issue has been open for four years - time to actually fix it? |
It's worth mentioning that I use node
Rolling back to |
Thanks for your report! Could you add a bit more details into your setup?
Are you running your code from a mounted folder? Do you unmount it yourself
after it starts? Could you check if it's being unmounted by docker itself?
The exception only occurs if node process doesn't have access to
node_modules some time after starting, so I'm trying to understand what
exactly happens.
I don't see anything relevant in docker release notes
https://docs.docker.com/docker-for-mac/release-notes/#docker-desktop-community-302
Alex
…On Mon, Dec 21, 2020, 11:32 Paul Gain ***@***.***> wrote:
It's worth mentioning that I use node 12.18.0, express 4.16.2 and
body-parser 1.18.3 all in docker, everything was fine until the 18th
December 2020 when upgrading docker desktop from 3.01 to 3.0.2 I started
seeing this error:
api_1 | Error: Cannot find module '../encodings'
api_1 | Require stack:
api_1 | - /usr/src/app/node_modules/iconv-lite/lib/index.js
api_1 | - /usr/src/app/node_modules/raw-body/index.js
api_1 | - /usr/src/app/node_modules/body-parser/lib/read.js
api_1 | - /usr/src/app/node_modules/body-parser/lib/types/json.js
api_1 | - /usr/src/app/node_modules/body-parser/index.js
api_1 | - /usr/src/app/node_modules/express/lib/express.js
api_1 | - /usr/src/app/node_modules/express/index.js
api_1 | - /usr/src/app/server.js
...
Rolling back to 3.0.1 fixed the issue but means I'm unable to update
docker for now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#118 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEZKHNCWZ6L76S2TQ2SLNLSV52A7ANCNFSM4CBCI3HA>
.
|
@ashtuchkin I encountered the exact same problem you mentioned in your initial error. I am running a Node server on a Xenial VM on GCP and after running my app.js and making a POST request to send an email, it makes use of iconv-lite at some point and throws |
@MeanManMachineMan can you provide more details about how you're deploying your app.js? Do you bundle it somehow? I'm trying to understand whether the encodings/ directory disappears after you're starting the app or it just never getting shipped to the VM. As a stop-gap, you can always manually require the module from your exception in your app, so that it's in the cache instead of loading on-demand. |
@ashtuchkin Yes certainly. I don't bundle my app.js since it's not a very big file and it just needs to act as a server for a few features. I simply serve it with |
You mentioned it works if you run
|
Ah I can see why the confusion has risen haha. I meant to say @dbussert had said that reinstalling it works. So after I have deployed the application through my pipeline (which runs |
yep that's what I'm trying to get to - how is your pipeline transferring the contents of node_modules to the vm so that it apparently loses some of the files (which you add back by doing npm install)? Is this some GCP-provided thing you're using or is it your own script? Also could you maybe attach result of As for |
My pipeline isn't "transferring" any contents because in my repository I don't have any
|
@ashtuchkin I actually figured out why I was getting that error dump. When I logged into the VM in my |
Thanks for digging through it @MeanManMachineMan! It's becoming much clearer now. Basically node_modules directory was being cleaned up by a different task while the test was running, right? That would definitely result in this error. Let me know if you need anything else. |
@ashtuchkin Yes, you are correct! I do have another question through, why didn't |
When you run app.js, first thing it does is it loads all the required modules in memory. For most apps it means that you can delete node_modules directory after your process has started and it will work just fine. Iconv-lite is an outlier in this regard as it can take from 20 to 150ms to load all the character tables (especially chinese, japanese encodings). This translates directly to startup time of any app that uses e.g. express framework. As encoding conversion is not actually needed for 99.99% apps, it makes sense to lazy-load character tables so that you pay this latency penalty only if you actually need them. If at that point node_modules folder is not present, then that error happens. |
Aha that clears it up pretty much. Thanks for taking out your time to help me @ashtuchkin! |
I was able to reproduce this issue with following setup and none of the workarounds worked for me. Minimal reproduction,
This particular setup somehow triggers the issue. Running standalone express works fine. Did not have enough time to trace it to particular package and I only found an issue reported here to update. UPDATE:
Since I thought supertest maybe the culprit here, I removed it and started standalone express in jest |
works for me 😉 |
Just ran into this issue when running a node 20.7.0 docker container with a simple express app in it... I read this whole thread and am I actually correct in assuming there is no solution other than 'just do another npm install, or restart your node process'? |
I've been stumped by this error a few times, and it's really hard to reproduce because it only happens once on a new install. So in a desperate attempt to glean more information, I'm posting this here to see if someone with a bit more knowledge can help point me in the right direction.
This error seems to be something with iconv-lite failing to require
../encodings
but it does exist. What's even more strange is that if you restart the server it works fine thereafter.Here is the stacktrace of the error:
The text was updated successfully, but these errors were encountered: