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

Calling createReadStream() for uploaded file crashes in NodeJS 13 #3508

Closed
lynxtaa opened this issue Nov 14, 2019 · 53 comments
Closed

Calling createReadStream() for uploaded file crashes in NodeJS 13 #3508

lynxtaa opened this issue Nov 14, 2019 · 53 comments

Comments

@lynxtaa
Copy link

lynxtaa commented Nov 14, 2019

Running in NodeJS 13.1.0:

const { createReadStream, filename } = await file // uploaded file
const stream = createReadStream()

Result:

RangeError: Maximum call stack size exceeded
        at _openReadFs (internal/fs/streams.js:1:1)        
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
        at _openReadFs (internal/fs/streams.js:123:12)     
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
        at _openReadFs (internal/fs/streams.js:123:12)     
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
        at _openReadFs (internal/fs/streams.js:123:12)     
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
        at _openReadFs (internal/fs/streams.js:123:12)     
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
        at _openReadFs (internal/fs/streams.js:123:12)     
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
        at _openReadFs (internal/fs/streams.js:123:12)     
        at ReadStream.<anonymous> (internal/fs/streams.js:116:3)
        at ReadStream.deprecated [as open] (internal/util.js:70:15)
        at ReadStream.open (D:\db-server\node_modules\fs-capacitor\lib\index.js:90:11)
@biomazi
Copy link

biomazi commented Nov 19, 2019

I am having the sam issue, but only when app is in production.
@lynxtaa did you find any workaround?

@lynxtaa
Copy link
Author

lynxtaa commented Nov 19, 2019

@biomazi Well, I decided to use Node 12 until this issue is fixed

@parktheredcar
Copy link

This seems to be a problem with an older version of fs-capacitor

According to jaydenseric/graphql-upload#170 the fix is checked in to master already and awaiting publish soon

until then, merging this into package.json will work. The preinstall script is only needed if using npm, as yarn supports dependencies natively

  "resolutions": {
    "fs-capacitor": "5.0.0"
  },
  "scripts": {
    "preinstall": "npx npm-force-resolutions"
  }

@Slessi
Copy link

Slessi commented Dec 3, 2019

graphql-upload 9.0.0 is out with node 13 support

@lynxtaa
Copy link
Author

lynxtaa commented Jan 27, 2020

graphql-upload 10.0.0 is out

@YCMitch
Copy link

YCMitch commented Feb 3, 2020

Adding this here in case it helps someone else.

After trying everything listed above I couldn't get it to work (even on Node 12??). Anyways I discovered I had graphql-upload in my node_modules, but apollo-server-core had its own and was using that one. Updating THAT one (just by FTP) has fixed it.

Far from a perfect solution, but ¯_(ツ)_/¯

@artonio
Copy link

artonio commented Feb 21, 2020

This seems like a pretty serious bug. I'm surprised that it is still open..

@safead
Copy link

safead commented Mar 5, 2020

Same issue here, any updates?

@polakmark
Copy link

@safead For me it showed up in production build inside docker containers, downgrading node to the last LTS helped FROM node:12.16.1

@BernatWozzo
Copy link

I downgraded to Node 12 and worked.

@cincarnato
Copy link

Same issue here

Vultraz added a commit to Vultraz/keystone-5 that referenced this issue Mar 29, 2020
Fixes keystonejs#2101.

Currently, apollo-server-express depends on apollo-server-core which depends on graphql-upload ^8.0.2.
graphql-upload 9.0.0 updated its own fs-capacitor dependency to a newer version that supports Node 13,
but apollo-server-core hasn't updated its own graphql-upload dependency yet. Solve this by just forcing
a compatible version of graphql-upload (currently the latest 10.0.0) to be used.
@navdeepsingh
Copy link

Yeah downgraded the node version, make it work. But not a right solution.

@ghost
Copy link

ghost commented Apr 7, 2020

Having the same issue. However check this workaround instead of downgrading: MichalLytek/type-graphql#37 (comment) more specifically for people using type-graphql.

@MWalid
Copy link

MWalid commented Apr 29, 2020

Same here, downgrading to 12.

@ChanManChan
Copy link

Annotation 2020-05-14 171750
Working fine with this Node version.

@ojaswa1942
Copy link

Isn't it about time to fix this? Node v14 is out with release status LTS, and here we are still finding support for v13 😕

@lynxtaa
Copy link
Author

lynxtaa commented May 27, 2020

Having the same issue. However check this workaround instead of downgrading: MichalLytek/type-graphql#37 (comment) more specifically for people using type-graphql.

Thanks!

Half a year passed since I opened this issue and no response from maintainers. I consider switching to https://github.com/mcollina/fastify-gql which is significantly faster , supports subscriptions and have built-in support for loaders

@danomatic
Copy link

I'm moving to express-graphql since this is a blocker for me and it appears to be unresolved.

@ElusiveByte
Copy link

I am having the sam issue, but only when app is in production.
@lynxtaa did you find any workaround?

Same here, only in production

@abenhamdine
Copy link

abenhamdine commented Jun 19, 2020

This bug is really a blocker for this module and should be considered top priority for maintainers.

A workaround is to force resolution for module graphql-upload to a more recent version :

in package.json

 "resolutions": {
    "graphql-upload": "11.0.0"
  },

be aware that resolutions property is currently only handled by yarn package manager, not by npm

with npm, you have to preinstall an aditionnal module to force resolutions :

"scripts": {
    "preinstall": "npx npm-force-resolutions",

of cours, check that your CI support preinstall scripts (our don't)

@glasser
Copy link
Member

glasser commented Feb 11, 2021

We are never going to upgrade graphql-upload in Apollo Server v2, and we are going to remove the built-in integration from Apollo Server v3. That's not to say that we don't like graphql-upload: we just think folks should be able to get every new improvement @jaydenseric puts out without needing to tightly couple it to the Apollo Server release cycle. Folks who want to use the newer version of graphql-upload now should follow @abernix 's suggestion in #3508 (comment) to disable the built-in integration and install the graphql-upload version of your choice.

I recognize that there may be some integrations (eg Lambda) where there's no easy place to plug in the graphql-upload processRequest function. If that's the case, we can fix that by adding an appropriate hook to the integration that would allow you to insert processRequest (or any handler of your choice!) at the right place, without tightly coupling the implementation to graphql-upload.

@JesusKreist
Copy link

None of the solutions worked for me. The resolutions fix and the graphql manual update didn't work as I'm using apollo-server-lambda. Downgrading my node version to 12 just so I can get apollo working does not seem like a reasonable thing to do. I guess I'm on an island here.

@dylvaz
Copy link

dylvaz commented Feb 19, 2021

Downgrading to node 12 removes the error for me but doesn't seem like the correct fix...

@glasser
Copy link
Member

glasser commented Feb 23, 2021

Yes, as mentioned, I don't think there's currently a great solution for integrating a non-built-in use of graphql-upload with apollo-server-lambda.

AS3 is going to remove the built-in graphql-upload. I think it would be great if, before AS3, we had a way to use graphql-upload with apollo-server-lambda that isn't the built-in integration! That probably involves looking at how apollo-server-lambda works, seeing where the graphql-upload integration gets slotted in, and adding a more generic integration point that doesn't have to be graphql-upload-specific.

I'm the new lead maintainer of AS but I have to admit — I don't use lambda myself, and we don't use it much internally at Apollo. My philosophy is that while apollo-server-lambda is quite important (it's the second most downloaded AS flavor, though the gap between it and Express is huge), it's best for improvements to apollo-server-lambda to be driven by folks who actually use it themselves in practice rather than by folks like me who are mostly learning Lambda on the fly.

So what I'd love to see happen is a PR from somebody in the community that allows you to slot in the newest release of graphql-upload directly into apollo-server-lambda without tying the implementations directly to each other. Then we can get that into a v2 minor release, and so when v3 comes out without the built-in graphql-upload integration it doesn't actually remove any functionality.

Taking a quick glance at apollo-server-lambda/ApolloServer.ts, it looks like it defines a fileUploadHandler function takes in a next function and closes over (and mutates!) event. So it seems like we could add a new option to new ApolloServer that's like lambdaPreGraphQLMiddleware: (event: APIGatewayProxyEvent, next: Function) => something which if provided gets called before fileUploadHandler. And then there would hopefully be a simple way to plug graphql-upload into that lambdaMiddleware function (while also passing uploads: false).

The function also closes over response but looks like that's just an elaborate way of being able to trigger graphql-upload's cleanup. So probably there should be a second lambdaPostGraphQLMiddleware hook where you can invoke response.end() rather than that being part of the first middleware.

I'd love to review a PR that's something like this! Of course I'm not a Lambda expert so there might be some other much more natural way of implementing this. But basically: let's come up with some non-upload-specific hooks that let you use upstream graphql-upload with apollo-server-lambda, and let's get that into AS2!

@glasser
Copy link
Member

glasser commented Feb 23, 2021

I've filed #4951 to track this.

@nuggy875
Copy link

A fix in my case was to force a version pf graphql-upload:
in your package.json add this:

  "resolutions": {
    "graphql-upload": "^11.0.0"
  },

If you are using yarn, that should work. npm users should add this to their "scripts" in package.json:
"preinstall": "npx npm-force-resolutions"
Then run npm install. You do need a lockfile for it to work.

Thanks 👍
Shocking that they still haven't fixed this officially

Actually, this doesn't fix the error in my case..

Any case like me?
Any other solutions rather than downgrading to Node v12 ?

My Node version is v.14.9.0

@glasser
Copy link
Member

glasser commented Feb 26, 2021

@nuggy875 What web server are you trying to use Apollo Server and graphql-upload with?

@nuggy875
Copy link

@nuggy875 What web server are you trying to use Apollo Server and graphql-upload with?

I'm using them for GraphQL Server..
Actually, I finally set my Node version to v12 ;)

@edw19
Copy link

edw19 commented Feb 27, 2021

I have the same problem with node 14x

@glasser
Copy link
Member

glasser commented Mar 1, 2021

@nuggy875 I mean, which apollo-server-* package are you using? Is this Lambda?

@Mian-Anees
Copy link

facing same issue with node 14x

@abenhamdine
Copy link

abenhamdine commented Mar 17, 2021

I'm gonna roll this one into #4865 too. I hope to make some rapid progress on this soon.

Actually I don't understand why this issue is closed, since #4922 uses now a fork of graphql-upload 8, which does not support either nodejs >=13 (nodejs 13 support has begun with version 9, see https://github.com/jaydenseric/graphql-upload/releases/tag/v9.0.0)

Correct me if I'm wrong, but if I understand correctly, this repo is still not compatible with nodejs >=13 and thus this issue is still pending.

@glasser
Copy link
Member

glasser commented Mar 17, 2021

@abenhamdine This is essentially closed as WONTFIX.

Upgrading the version of graphql-upload that Apollo Server 2 relies on in order to support Node 14 would require taking other backwards-incompatible changes from graphql-upload, which we aren't comfortable doing within the single major version of AS2.

On the other hand, the plan for AS3 is not to upgrade graphql-upload but to disentangle it from AS entirely and encourage folks to use graphql-upload directly; the role of AS3 here will be to provide whatever hooks are necessary for users to add arbitrary versions of graphql-upload to their app rather than to bundle a specific version.

I encourage you to use graphql-upload directly in your app (pass uploads: false to new ApolloServer and just use graphql-upload as described in its docs).

My guess is that this will work fine with, say, apollo-server-express, but that this will be challenging for apollo-server-lambda because there's no "middleware" there to let you inject the call to graphql-upload. So somebody interested in getting that working should come up with a good hook or two to add to apollo-server-lambda in order to integrate graphql-upload.

Note that I just landed a PR to rewrite apollo-server-lambda to be an async function (and to inline the old graphqlLambda function) which I think will make it a lot easier to add integration points, as the old code structure was quite hard to follow.

@abenhamdine
Copy link

Thanks a lot @glasser for this detailled explanation.
We plan to implement direct use of graphql-upload as you explained, thx.

However I see that all the issues related the the incompatibility with node 13 are closed with a link to the version 2.21 of appolo-server-express.

I fail to see how version 2.21 is related to this very problem : version 2.21 allows as of now to use graphql@15, but is still incompatible with node 14 (as you have confirmed above).

It was not clear for me what the current situation was (because I don't know what you have modified in your fork of graphql-upload version 8) and I suspect many users of this module are confused as well.
I think the most important part here is "This is essentially closed as WONTFIX" and should be mentionned in the readme, because newcomers with this library or users migrating from node <13 are likely to run into the same issue as well.

And thx again for all your work on this library 👍

@glasser
Copy link
Member

glasser commented Mar 17, 2021

That's a good point. So far to learn about this you'll have to search for my comments on random GitHub issues. #5039 should be a good start.

jahjedtieson added a commit to Smithsonian/dpo-packrat that referenced this issue Apr 30, 2021
* Switch from using build-in Apollo Server "Upload" functionality to making explicit use of graphql-upload.  This has been done to work around a defect handling uploads on NodeJS v14.  Uploads have been working ok in containers and in our staging environment, but they were not working when running the server directly on Windows.  Now, this works too.  Note that Apollo Server currently uses graphql-upload -- the same version that we're using!  But we are disabling Apollo Server's default use, and replacing it with our own, explicit express middleware, per apollographql/apollo-server#3508.
ichrist97 referenced this issue in ichrist97/msp_backend Nov 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.