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

Apollo Server hangs during upload file with graphql-upload #352

Closed
NicoSan20 opened this issue Dec 15, 2022 · 41 comments
Closed

Apollo Server hangs during upload file with graphql-upload #352

NicoSan20 opened this issue Dec 15, 2022 · 41 comments

Comments

@NicoSan20
Copy link

Hello, I am writing to you because I am having issue when I'm using graphql-upload. Let me explain :

I have successfully implemented graphql-upload with the following documentation Enabling file uploads in Apollo Server

As you can see I use Apollo Server (3.5). Here are also the other technologies used in backend :

  • node (16.18.1)
  • npm (8.19.2)
  • graphql-upload (13.0.0)

And from the frontend, I use apollo-upload-client (17.0.0).

I didn't really have a problem with setting it up, but it's when using it that it goes bad. The issue occurs when I upload a file. During the upload, my apollo server is no longer accessible and the frontend no longer works.

What is strange is that the sending is done correctly (without error) but you have to wait between 3 to 6 minutes for a simple 3MB image (and during this time, my application is no longer available).

I specify that the server works locally on my PC and that it is not saturated and also I am the only one to use it, so no memory or cpu overload.

After some research, I find that the problem occurs in the resolver, when doing the stream.pipe(out). To make it simple, I took the code provided in the documentation mentioned above :

    singleUpload: async (parent, { file }) => {
      const { createReadStream, filename, mimetype, encoding } = await file;
      const stream = createReadStream();
      const out = require('fs').createWriteStream('local-file-output.txt');
      stream.pipe(out);
      await finished(out);
      return { filename, mimetype, encoding };
    },

In my final code I was also thinking of using promisify and waiting for file upload streams, but I'm not sure which is the best way.

Do you know why I have these slownesses? Why is my server hangs? What's going on? Please, I really need your help on this issue. (sorry for my english, it's not my mother tongue)

@jaydenseric
Copy link
Owner

Hello :)

You are using graphql-upload v13, but the current version is v16.0.2. There is a chance your issue has been fixed in those 3 major versions; perhaps try updating it? I can't support very old versions of this package.

@daunJung-dev
Copy link

daunJung-dev commented Dec 15, 2022

Hi :) @NicoSan20

I had struggled same issue like yours and I have solved it by some actions.
this is caused by version issue of node.
Did you see the log from process when you upload file?
image

the warning from the process is the main problem of it.

In Node.js docs, you can check ReadStream and WriteStream are separated from core of node.js and it is belong to fs.
but, in graphql-upload, we use the createReadStream causing error.

Individual Solution

I just solved it, force the version of library.

// package.json
{
	...,
	"scripts": {
		"preinstall": "npx npm-force-resolutions" // when use npm only, not for yarn
		...
	},
	"resolutions": {
            "fs-capacitor": "^6.2.0",
            "graphql-upload": "16.X.X" // WRITE YOUR VERSION
        },
	...
}

and it worked for me. I hope it can help you

@daunJung-dev
Copy link

Same issue was closed. #170

@NicoSan20
Copy link
Author

Hi there :)

Thank you so much for your fast replies.

@jaydenseric : It took me a little time because of the ESM, but I updated my project with the versions below :

  • node (18.12.1)
  • npm (9.2.0)
  • graphql (16.6.0)
  • graphql-upload (16.0.2)
  • apollo-server (3.11.1)

But the issue still remains.

@daunJung-dev : unfortunately I don't see the warning, but I still applied the changes you suggested, but it didn't fix the issue.

In the 2 solutions the issue remains the same, the apollo server hangs during the upload of the file which also takes a lot of time.

To be honest I'm a bit lost right now and I don't know what to do :( Thanks in advance for your help.

@jaydenseric
Copy link
Owner

@NicoSan20 are you sure these graphql-upload dependencies are installed in your node_modules at the correct versions?

"busboy": "^1.6.0",
"fs-capacitor": "^8.0.0",

You can run npm ls fs-capacitor and npm ls busboy to check.

Another thing to try is to promisify and await the file upload stream in your resolvers more like this example:

https://github.com/jaydenseric/apollo-upload-examples/blob/a02165ae774f5c5bd18017bc6df37512c5a5328e/api/storeUpload.mjs#L21-L44

I haven't used this finished utility before that the Apollo docs suggests, so not sure if it's a good idea or not:

const { finished } = require('stream/promises');

@NicoSan20
Copy link
Author

NicoSan20 commented Dec 16, 2022

@jaydenseric : Thank you for your answers. Yes, I can confirm that the dependencies are the same as you show.

After several hours of research, here are a few more details on what I see.

I thought the issue came when doing stream.pipe(out) in the resolver, but it's worse than that, because, I created an "empty" resolver like this and the issue still there :

upload: async (parent, { file }) => {
  return file;
},

What is interesting is that even like that, I can see the temporary file (capacitor-3a19ad4ce7baf3a8d6c30db070683b96.tmp) created by fs-capacitor being upload in /tmp and I can now confirm that as long as the file is uploading in /tmp, the apollo server is no longer available.

I also notice that if in the resolver I put an output log to see the file (simply like this : console.log(file)), the upload time is 10 times slower.

Just in case, the result of the log :

Promise {
  {
    filename: 'SamplePNGImage_3mbmb.png',
    mimetype: 'image/png',
    encoding: '7bit',
    createReadStream: [Function: createReadStream]
  }
}

I hope with this additional information it can help a little more on the resolution of this issue. From my part, I keep looking desperately :(

@jaydenseric
Copy link
Owner

jaydenseric commented Dec 17, 2022

What is interesting is that even like that, I can see the temporary file (capacitor-3a19ad4ce7baf3a8d6c30db070683b96.tmp) created by fs-capacitor being upload in /tmp

A couple of things to note, that might not at first be obvious about the way graphql-upload works.

It always creates a temp file for an incoming upload, even if no resolvers create a stream from it. graphql-upload processes the request before resolvers run, so it doesn't know what uploads are going to be used in what way by the resolvers. It's possible for one file upload to be used as an input argument for multiple mutations in one request; by having the upload buffer to a temp file multiple resolvers can create read streams from it (via fs-capacitor), instead of competing over consuming the single raw file upload read stream.

the apollo server is no longer available

I'm not sure what you mean by "no longer available".

Something else to note, is that the Express and Koa middleware exported by graphql-upload waits for the request to completely finish uploading before letting a response be sent.

The reason is because while HTTP standards allow a sever to respond before the browser has finished uploading the request, most browsers have bugs where they crash the network request in weird ways if a response comes back before the request has finished. If you don't like this default behavior of the middleware, you can always make your own to your taste using the exported processRequest function.

Always promising and await the completion of file uploads in resolvers, they can't just be ignored.

@NicoSan20
Copy link
Author

@jaydenseric : First, thank you for your time and your reply. Now I understand a little better how it works.

I'm not sure what you mean by "no longer available".

What I mean is that during the upload (during the creation of this temporary file via fs-capacitor), all clients who are connected to my website can no longer use it.

Also, on the client side, I use a websocket connection which breaks during the upload too.

So, could I have misconfigured something on the client side ? Should I search on the client side ? I'm currently using apollo-upload-client (17.0.0)

Just in case, here is my configuration :

const uploadLink = createUploadLink({
  uri: process.env.REACT_APP_GRAPHQL,
  credentials: 'include',
});

const wsLink = new WebSocketLink({
  url: process.env.REACT_APP_SUBSCRIPTIONS,
});

const terminatingLink = split(
  ({ query }) => {
    const definition = getMainDefinition(query);
    return definition.kind === 'OperationDefinition' && definition.operation === 'subscription';
  },
  wsLink,
  uploadLink
);

const errorLink = onError(({ graphQLErrors, networkError }) => {
  if (graphQLErrors) {
    console.log('graphQLErrors', graphQLErrors);
  }
  if (networkError) {
    console.log('networkError', networkError);
  }
});

const langLink = new ApolloLink((operation, forward) => {
  operation.setContext({
    headers: {
      lang: localStorage.getItem('i18n'),
    },
  });
  return forward(operation);
});

const link = ApolloLink.from([langLink, errorLink, terminatingLink]);

const cache = new InMemoryCache({
  typePolicies: {
    Query: {
      fields: {
        text: {
          merge(existing, incoming) {
            return incoming;
          },
        },
      },
    },
  },
});

const client = new ApolloClient({
  cache,
  link,
});

@NicoSan20
Copy link
Author

Last clarification, during the upload (during the creation of this temporary file via fs-capacitor), I cannot also send graphql request from Apollo Studio, which confirms that apollo server is no longer available during this process.

@jaydenseric
Copy link
Owner

Are you able to reproduce the issue using the Apollo upload examples app and API?

https://github.com/jaydenseric/apollo-upload-examples

@NicoSan20
Copy link
Author

@jaydenseric : Yes, I can easily reproduce the issue with your app and API.

For information, I ran your project on a virtual machine with the characteristics below :

  • Debian 11 (1vCPU & 8GB of memory)
  • node (v18.12.1)
  • npm (v9.2.0)

Here is what i see :

  • With File type, the issue is there, but it's going so fast that it's almost not visible
  • With FileList type, we can see the issue very well. When I upload a file of 3MB or more, during the upload, you can no longer go to your app (http://localhost:3000/) from another navigation page. Also, after some time your application crashes and shows the following error :
Unhandled Runtime Error
Error: Failed to fetch

Call Stack
new ApolloError
webpack-internal:///./node_modules/@apollo/client/errors/index.js (37:28)
Object.error
webpack-internal:///./node_modules/@apollo/client/core/QueryManager.js (154:129)
notifySubscription
webpack-internal:///./node_modules/zen-observable-ts/module.js (141:18)
onNotify
webpack-internal:///./node_modules/zen-observable-ts/module.js (180:3)
SubscriptionObserver.error
webpack-internal:///./node_modules/zen-observable-ts/module.js (233:5)
Object.eval [as error]
webpack-internal:///./node_modules/@apollo/client/utilities/observables/asyncMap.js (37:69)
notifySubscription
webpack-internal:///./node_modules/zen-observable-ts/module.js (141:18)
onNotify
webpack-internal:///./node_modules/zen-observable-ts/module.js (180:3)
SubscriptionObserver.error
webpack-internal:///./node_modules/zen-observable-ts/module.js (233:5)
eval
webpack-internal:///./node_modules/@apollo/client/utilities/observables/iteration.js (8:68)
Array.forEach
<anonymous>
iterateObserversSafely
webpack-internal:///./node_modules/@apollo/client/utilities/observables/iteration.js (8:25)
Object.error
webpack-internal:///./node_modules/@apollo/client/utilities/observables/Concast.js (44:90)
notifySubscription
webpack-internal:///./node_modules/zen-observable-ts/module.js (141:18)
onNotify
webpack-internal:///./node_modules/zen-observable-ts/module.js (180:3)
SubscriptionObserver.error
webpack-internal:///./node_modules/zen-observable-ts/module.js (233:5)
eval
webpack-internal:///./node_modules/apollo-upload-client/public/createUploadLink.js (230:22)

I specify that it is indeed the FileList type method that I also use in my project

If needed, The file I used (PNG image of 3MB) for the tests, comes from this web site https://www.sample-videos.com/download-sample-png-image.php

Could you tell me if you can also reproduce the issue with the 3MB file and the FileList type method ?

Thanks again for your time and support, I appreciate it.

@jaydenseric
Copy link
Owner

When I upload a file of 3MB or more, during the upload, you can no longer go to your app (http://localhost:3000/) from another navigation page. Also, after some time your application crashes and shows the following error

I just tried it on my machine, and can't reproduce this using that exact 3 MB PNG. Locally, the upload happens too fast to tell what's going on so I tried using Chrome dev tools to throttle network speed to 3G, selected the file (starting the GraphQL multipart request), then while that is uploading I opened the same http://localhost:3000/ URL in a fresh browser tab and it loaded fine.

@jaydenseric
Copy link
Owner

I should say, my machine is Apple M1 Max chip, 64 GB memory, with a lot of disk space.

@NicoSan20
Copy link
Author

@jaydenseric : Thank you for your comments.

So I redid a fresh installation of a new virtual server with the following characteristics :

  • Debian 11 with 4 vCPU and 16GB of memory
  • node (v18.12.1)
  • npm (v9.2.0)

Now for all my tests I use your API and app example (https://github.com/jaydenseric/apollo-upload-examples), But I still have the same issue.

With your API and app example, and using the FileList method, I get the always following error message :

Unhandled Runtime Error
Error: Failed to fetch

Call Stack
new ApolloError
webpack-internal:///./node_modules/@apollo/client/errors/index.js (37:28)
Object.error
webpack-internal:///./node_modules/@apollo/client/core/QueryManager.js (154:129)
notifySubscription
webpack-internal:///./node_modules/zen-observable-ts/module.js (141:18)
onNotify
webpack-internal:///./node_modules/zen-observable-ts/module.js (180:3)
SubscriptionObserver.error
webpack-internal:///./node_modules/zen-observable-ts/module.js (233:5)
Object.eval [as error]
webpack-internal:///./node_modules/@apollo/client/utilities/observables/asyncMap.js (37:69)
notifySubscription
webpack-internal:///./node_modules/zen-observable-ts/module.js (141:18)
onNotify
webpack-internal:///./node_modules/zen-observable-ts/module.js (180:3)
SubscriptionObserver.error
webpack-internal:///./node_modules/zen-observable-ts/module.js (233:5)
eval
webpack-internal:///./node_modules/@apollo/client/utilities/observables/iteration.js (8:68)
Array.forEach
<anonymous>
iterateObserversSafely
webpack-internal:///./node_modules/@apollo/client/utilities/observables/iteration.js (8:25)
Object.error
webpack-internal:///./node_modules/@apollo/client/utilities/observables/Concast.js (44:90)
notifySubscription
webpack-internal:///./node_modules/zen-observable-ts/module.js (141:18)
onNotify
webpack-internal:///./node_modules/zen-observable-ts/module.js (180:3)
SubscriptionObserver.error
webpack-internal:///./node_modules/zen-observable-ts/module.js (233:5)
eval
webpack-internal:///./node_modules/apollo-upload-client/public/createUploadLink.js (230:22)

On the API server console I also have the following error displayed :

Error [ERR_HTTP_REQUEST_TIMEOUT]: Request timeout
    at new NodeError (node:internal/errors:393:5)
    at onRequestTimeout (node:_http_server:780:30)
    at Server.checkConnections (node:_http_server:593:7)
    at listOnTimeout (node:internal/timers:564:17)
    at process.processTimers (node:internal/timers:507:7)

Failed to store upload: BadRequestError: Request disconnected during file upload stream parsing.

I also did the test on a fresh install of Ubuntu 22.04.1 (LTS) with 4 vCPU and 16GB of memory, but I have exactly the same issue as under Debian.

These tests have been done on standard installations and by downloading your API and app example. Then I went to the 'api' directory and run the command npm install, then npm run dev. I also did the same thing under the 'app' directory. Nothing else.

Is a step missing ? Am I forgetting something ?

I think this time I've reached the end of my skills :(

@NicoSan20
Copy link
Author

Hi guys ! Am I the only one who noticed this issue ?

If someone has some time, could you do a test on your side, just to confirm that I'm not the only one ? Here are the steps I did :

  1. Create a VM with 4 vCPU and 16GB of memory
  2. Simply install Debian or Ubuntu
  3. Install NodeJS (https://github.com/nodesource/distributions/blob/master/README.md#deb)
  4. Download API and app example provided by @jaydenseric (https://github.com/jaydenseric/apollo-upload-examples)
  5. Then go to 'api' directory and run the command npm install, then npm run dev. Do also the same under the 'app' directory.
  6. Then try to upload several images of 3 or 5 MB (https://www.sample-videos.com/download-sample-png-image.php)

I will appreciate it a lot... Thanks in advance for your help.

@jaydenseric
Copy link
Owner

Something to keep in mind about this error message:

Failed to store upload: BadRequestError: Request disconnected during file upload stream parsing.

The most common way people see a message like that is when the file upload streams are not promisified and awaited correctly and the resolvers return too soon, resulting in the GraphQL server sending a response before the client has finished streaming up all the files. This results in a disconnect, and a disconnect error on the file upload read stream that happened to be streaming at the time.

@NicoSan20
Copy link
Author

@jaydenseric : However I'm using your example on a freshly installed server. Why do I have this behavior and apparently not on your side ? That's the question I ask myself.

@jaydenseric
Copy link
Owner

2 weeks ago I tried pretty hard on my machine to get the examples to break the way you describe, and gave up but left it open on my computer overnight. The next day I tried again with the same sever and browser tab from yesterday and it seemed to have the problem, but as soon as I refreshed everything I couldn't get it do happen again.

I don't really have the availability to debug your reproduction steps with a virtual machine right now; the fact people haven't been raising issues like crazy about this gives me a fair bit of confidence there is something either wrong with Apollo Server/Client (I don't use it in my own work) or the examples code is a bit naive in some way.

@jaydenseric
Copy link
Owner

It would be good to update the Apollo examples API to Apollo Server v4 and see if the issue still happens:

https://github.com/jaydenseric/apollo-upload-examples/blob/a02165ae774f5c5bd18017bc6df37512c5a5328e/api/package.json#L23

@NicoSan20
Copy link
Author

@jaydenseric : Thank you for your reply.

As soon as I have a little time, I will do the test to switch to V4. I will keep you in touch about the result.

@NicoSan20
Copy link
Author

@jaydenseric : I switched your example to @apollo/server version 4.3.0, but the issue still the same :(

I will try to open a ticket on the apollo-server side, maybe they will have another approach.

@NicoSan20
Copy link
Author

@jaydenseric : Apparently until I can demonstrate that the issue comes from apollo-server, they won't do anything. Unfortunately, I'm not competent enough to find the exact cause of this issue.

Any new leads are good to take, so if you have any ideas, don't hesitate.

@jaydenseric
Copy link
Owner

Thanks for your work @NicoSan20, sorry you have been having such a hard time with this issue. I'll be implementing this package in a work project over the coming weeks and will want to be sure it works well for lots of simultaneous requests, so if there is an issue I'll likely encounter it and investigate.

@NicoSan20
Copy link
Author

@jaydenseric : Thank you for your reply. I will therefore keep an eye on this issue, because currently I'm sadly blocked on my project :( Thank you to you in advance for your help and next feedback.

@uranderu
Copy link

I don't know how constructive this is. But I'm using a fork called graphql-upload-minimal which also seems to have this/ a related issue. Maybe the same thing is happening here, where the server "hangs" because it can't throw an error? I also tried using this fork but I get the same result. I don't know but multiple forks having the same issue suddenly might indicate that something else is going on?

I tried turning it into a promise like you said above, but didn't work for me. I also couldn't get any logging to work. I'll try replicating the issue like @NicoSan20 described above.

@uranderu
Copy link

uranderu commented Jan 13, 2023

The example works on my machine. Both file and filelist. But since the issue has sneaked in somewhere in the last month, at least in my project, I wouldn't expect something else. The example repository hasn't been updated since September.

@NicoSan20
Copy link
Author

@uranderu : Have you tried to upload several images of 3 or 5 MB ? I used these files for my tests (https://www.sample-videos.com/download-sample-png-image.php)

@uranderu
Copy link

@NicoSan20 Tried it again with multiple images from that site. And that worked. I did try it on my local machine though, and that is Arch Linux.

@NicoSan20
Copy link
Author

@uranderu : Can you also confirm that during the upload you can browse the web site from another page? Thank you in advance for your feedback.

@koresar
Copy link

koresar commented Jan 24, 2023

Hi all.

The maintainer of the graphql-upload-minimal here.
I'm going to do a guess why this issue happens.

First of all - we can see that the consumption of the stream stops. Thus server and client hang (timeout).

This is a classic Node issue. Express.js/Connect.js (but not Koa) has it a lot. E.g.:

function requestHandler(req, res, next) {
  return; // <- accidental code, nobody subscribed to "data" event of req
}

Voila! Your request hung. Because nobody subscribed to the "data" event: req.on("data", ...) or req.pipe(...). https://stackoverflow.com/a/29111941/188475

Knowing the above, I was under impression that the fs-capacitor consumes the whole request stream regardless, simply because it subscribes to the "data" event of the stream here. Turns out - not always whole.

So here is my guess.
Node.js or OS (or else) pauses the streaming of data because all the available memory/disk/TCP buffer was consumed and needs draining.

Why it pauses?

Just imagine your Node HTTP server accepted 100 file upload requests simultaneously. Each file is 4.3GB. From my experience the Node.js will easily process all that thanks to streams! At any point of time Node.js holds a little (several kilobytes, say 8KB) buffer for each TCP socket. These 100 upload requests would consume 100*KB=800KB of memory only. So, even if your server has 128MB of RAM, it will still be able to process ONE HUNDRED of 4.3GB files! :)

However, to make things upload faster you need bigger RAM buffers, the Node.js would use as much memory as the OS allows it to! If Node.js server app has 128MB only then all the 128MB will be used for TCP buffers. After consuming all the RAM the OS will tell the client-side (browsers or curl) to pause sending more data. <- I'm guessing this is why your file upload hangs.

To reproduce the bug above one would need:

  • Not 32GB of RAM, but constrain your Node.js process to 0.5 GB RAM.
  • Not 5MB file, but 5GB file.
  • Not Koa, but Express.
  • Perhaps, also avoid using @apollo/server/plugin/drainHttpServer
  • (Optional, to make sure fs-capacitor is constrained too) Not 1TB of HDD, but 1GB of free space on your hard drive.

Potential bugfix as I see it

The request stream would need to be destroyed if the GraphQL resolver throws or forgets to consume the http request.

However, I have no idea where to put the req.destroy() line into the module codebase. Sorry.

@uranderu
Copy link

@NicoSan20 Just tried it with throttling my upload to GPRS and uploading multiple images from the site you referenced. Works flawlessly and I can see in the network tab that the POST request is still waiting for a response. So that works.

@NicoSan20
Copy link
Author

@koresar : Thank you very much for your answer, it gives me a little more hope.

@Jeffrey-Zutt
Copy link

Jeffrey-Zutt commented Mar 10, 2023

After updating @apollo/server from v3 to v4, upload requests started "hanging".

I found this little gem in the Apollo documentation:

https://www.apollographql.com/docs/apollo-server/security/cors/#graphql-upload

Apparently the @apollo/server now blocks any multipart/form-data requests.
Adding a special non-empty header Apollo-Require-Preflight fixes the issue for me.

@uranderu
Copy link

After updating @apollo/server from v3 to v4, upload requests started "hanging".

I found this little gem in the Apollo documentation:

https://www.apollographql.com/docs/apollo-server/security/cors/#graphql-upload

Apparently the @apollo/server now blocks any multipart/form-data requests. Adding a special non-empty header Apollo-Require-Preflight fixes the issue for me.

It has been a while and I'm no longer working on the project where this issue applied. So I can't verify this. But I am sure that I already added that header to my request. But maybe others can verify.

@SanNic20
Copy link

Hi @Jeffrey-Zutt, Hi @uranderu, I'm pretty busy these days, but I'll try adding the special non-empty header Apollo-Require-Preflight as recommended by @Jeffrey-Zutt and I will keep you in touch.

@SanNic20
Copy link

@Jeffrey-Zutt : Here is the explanation to your issue : Version 4 of @apollo/server uses by default "CSRF prevention feature" that's why you have to put the Apollo-Require-Preflight header for it to work. (Documentation)

Otherwise the issue is still there for me. I tried also with the updated example provided by @jaydenseric https://github.com/jaydenseric/apollo-upload-examples, but without success.

@koresar : Were you able to do anything about it ? You seem to have understood the problem very well.

@SanNic20
Copy link

You won't believe me, but I finally found the issue!

As I told you, I develop on a linux VM from my PC through Visual Studio Code. This way when I run my project, Visual Studio Code creates a port forwarding on localhost.

So when I do my tests on the application, I go through localhost which is redirected to my VM from Visual Studio Code and that's the problem.

Because if I do my tests directly on the IP address of my VM, I don't have any hangs and the upload is done correctly without freezing the server.

It was not more complicated than that :)

Sorry to have wasted your time and a big thanks to everyone who took the time to investigate and answer me.

@jaydenseric : You can now close this issue.

@jaydenseric
Copy link
Owner

@SanNic20 that's great news! What a relief to have an answer.

@koresar
Copy link

koresar commented Aug 9, 2023

Hello all.

The sibling module has fixed a possible cause of the bug. flash-oss#37 (comment)

Cause of the bug: the Express.js middleware was mishandling errors (connectivity or other types). Thus, the whole request was hanging. This is a DOS attack vector.

@jaydenseric would you like a PR like that submitted? I can fully explain why this is the correct fix.

@jaydenseric
Copy link
Owner

@koresar thanks for the heads-up; in response yesterday I have been doing maintenance preparation work in anticipation of working on this.

Don't worry about a PR for now; I'll put a fix together once I fully understand what the problem is. But as deep an explanation as you're able to share is welcomed!

To explain a bit of context about this:

// @ts-ignore Todo: Find a less hacky way to prevent sending a response
// before the request has ended.
response.send =
/** @param {Array<unknown>} args */
(...args) => {
requestEnd.then(() => {
response.send = send;
response.send(...args);
});
};

At the time (I don't know if browsers have improved in the years since) when a browser fetch request has a server response before the request has finished streaming (a valid thing to do in HTTP), instead of resolving the response instance it would just reject with crappy exceptions. This meant, if for example you had a form registering a user that has a file upload for the avatar, if your GraphQL resolvers throw a validation error about the username being taken while the avatar is still uploading, instead of the GraphQL client being able to cache and render GraphQL errors for the mutation you would just have a fetch error. To prevent this outcome, our GraphQL upload middleware always waits for the request to end before allowing the GraphQL response to be sent. The downside of course is that the user has to wait for the upload to redundantly finish for a failed operation to see the GraphQL errors, but the alternative is they would quickly see no detailed GraphQL errors which is worse.

We were not happy having to add these hacky workarounds, really browsers should respect the HTTP specs and not crash a fetch multipart request if there is an early response. We understood it's a little invasive to monkey patch the Express API to pull it off, but reasoned that if people don't like that particular behavior it's trivial for them to import processRequest and make custom middleware in only a few lines of code with their preferred behavior.

@koresar
Copy link

koresar commented Aug 11, 2023

Thanks for the much info.

That was a very thoughtful of you to implement such protection. Unfortunately it caused another bug for unstable connections.

Look like there is one reason for this hack:

  • not ideal fetch implementation in browsers which resulted in not ideal error messages for the end users.

Gooogling didn't help. But chatgipiteeing did help. It reminded me that the proper way to monkey-patch requests is via res.end = myThing, not via res.send = mything.

graphqlUploadExpressMiddleware((req, res, next) => {
  const originalEnd = res.end;
  res.end = function(data, encoding) {
    if (this._isPaused) {
      // Do not send the response yet
      this._dataToBeSent = data;
      this._encodingToBeSent = encoding;
    } else {
      // Send the response immediately
      originalEnd.call(this, data, encoding);
    }
  };

    res._isPaused = true;
    processRequest(request, response, processRequestOptions)
      .then((body) => {
        res._isPaused = false;
        req.body = body;
        next();
      })
      .catch((error) => {
        if (error.status && error.expose) response.status(error.status);
        next(error);
      });
  };
});

I haven't tested the above. Hope it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants