client hangs after failed upload

[bhamon]

I'm using mercurius + mercurius-upload with a basic mutation resolver that reads an input file to store it to a S3 bucket.
If the resolver fails all the subsequent requests to fastify hang indefinitely.

You can find a minimal reproduction code here: https://github.com/bhamon/bug-graphql-upload

To reproduce the issue you should use Chrome + upload a file of about 300 KB (smaller files doesn't fail).
The issue can be reproduced with other clients too, but depending on the client and the uploaded file the results varies a lot. The client must reuse the same HTTP stream in order for the problem to occurs.

The issue as already been reported here for Apollo server and a corrective PR has been issued.

I'm not very familiar with Fastify internals so I can only assume that the problem should be the same.

[bhamon]

instructions:

• launch the server (node .)
• open a browser and navigate to http://localhost:8888
• select a file and click on the test button
• in the Network tab you should see the second request that hangs

Please note that the first failing request is the expected behavior to reproduce the issue.

[mcollina]

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[aarishmkhan]

Hey, I can work on this issue. Please assign this issue to me

[Jawell]

Do we have any progress?

[mcollina]

Do we have any progress?

I have not seen a PR addressing this yet.

[Jawell]

I investigated that it is not necessary to have an error on the first request. I used Firefox and 487kb file
This bug is reproduced even if the first request was successful. The problem occurs exactly when the first request has a file. If you remove form.append("file", domSelector.files[0]);, then everything will be fine

Snippet to respond true from mutation
Try to use it with and without append file
https://github.com/bhamon/bug-graphql-upload

// index.js

...

const schema = gql`
  scalar Upload

  type Query {
    dummy: Boolean
  }

  type Mutation {
    dummyTrue: Boolean
    dummy(file: Upload!): Boolean
  }
`;

const resolvers = {
  Upload: gqlUpload,
  Query: {
    dummy: () => true,
  },
  Mutation: {
    dummyTrue: () => true,
    dummy: async (_, { file }) => {
      throw new Error("dummy");
    },
  },
};
...

// index.html

...
form.append(
  "operations",
   JSON.stringify({
   query: gql`
     mutation dummy {
       dummyTrue
     }
     `,
     variables: { file: null },
  })
);
...

[Jawell]

Also, if you remove second request and leave first request with append file, you will see that hangs is existed every second time

This is only first request (dummyTrue mutation) with append file. I have a hangs every second time

[Jawell]

So, now I tried REST API - only fastify with @fastify/multipart, the result is the same. I think the problem in fastify package, not in GraphQL API or mercurius
A request hangs every second time, when I use multipart data

[Jawell]

Hey @mcollina we have a progress, could you check ticket in fastify, that I mentioned?

Here the last investigation:

@Jawell

I tried to use graphql-upload and mercurius (GraphQL adapter over fastify, I think you know). I'm still facing the problem. But hangs problem was fixed in graphql-upload and I didn't have problems using Apollo + Express + graphql-upload without stream consuming (the same mutation and schema as in this example). So, is it fastify "feature"?

@climba03003

It probably mercurius question more than a fastify question.
graphql-upload is actually dumping the file into file system but @fastify/multipart by default inherit the same behavior from busboy which is user must control the behavior explicitly. As a graphql plugin if something is works in one side but not another, I suggest you to file an issue in mercurius.

[mcollina]

I took a quick skim and this is an issue is most relevant to the mercurius-upload module, not mercurius itself:

1. mercurius-upload module is not using @fastify/multipart parsing package, but rather a custom multipart handling of graphql-upload
2. mercurius-upload module is just a tiny wrapper on top of graphql-upload
3. mercurius-upload has not been updated to the latest Mercurius and it's stuck on v10.

This module is missing the equivalent of this line, i.e. we should wait for the request to be fully consumed before sending a response. Currently mercurius-upload is missing this information.

I've seen both @SimenB and @PabloSzx involved in that package, so maybe can they help.

[Jawell]

Thank you for reply
I used graphql-upload with mercurius and I'm still facing the problem. Everything is fine with graphql-upload + Apollo + express. I think that Apollo or express are handled that behavior. Another problem that I can't use mercurius-upload because this package is made for fastify v3, I use v4

[SimenB]

3. mercurius-upload has not been updated to the latest Mercurius and it's stuck on v10.

We use mercurius-upload with Mercurius v11 successfully, FWIW (and also with @apollo/server and @as-integrations/fastify). Haven't attempted to upgrade to v12, but OP uses v11, so probably won't make a difference.

This module is missing the equivalent of this line, i.e. we should wait for the request to be fully consumed before sending a response.

Adding that should be somewhat straightforward, unless I'm missing something. I'd assume an await stream.promises.finished(request.raw) (or possibly await events.once(request.raw, 'end')?) should work? Seems weird to me that processRequest doesn't wait internally, but 🤷 It's handled in both the express and koa middlewares that ship with it, so probably something I'm missing.

Another problem that I can't use mercurius-upload because this package is made for fastify v3, I use v4

This is incorrect.

mercurius-upload/index.ts

Line 36 in 3a91963

fastify: '>= 4.x',

I see the readme is wrong, though: #12

[mcollina]

I can see something like that should work @SimenB.

@Jawell would you like to try fixing this?

[Jawell]

Sure, I'll try to use this advice, stay in touch. You can assign me

[mcollina]

I think you should place an await events.once(request.raw, 'end') inside an onSend Fastify hook.

[Jawell]

So, I tried this and it would be enough:

  fastify.addHook('onSend', async function (request) {
    if (!request.mercuriusUploadMultipart) {
      return
    }

    await promises.finished(request.raw)
  })

I'll create PR with tests later today. Or you want to use events.once? But I didn't catch what is events

[SimenB]

But I didn't catch what is events

require('events')

[climba03003]

It is a simple function and I would avoid promise most of the time.
You can use the done callback without async-await.

[SimenB]

Closed via #14

[PabloSzx]

Closed via #14

[sammwafy]

The same error persists after switching to graphql-upload-minimal.
Changing the import of processRequest to be from graphql-upload or graphql-upload-ts resolved the issue for me.

[amazzoccone]

Same error as @sammwafy 😞 . Fix it as mentioned:

import processRequest from 'graphql-upload/processRequest.mjs'

Would be great to revert usage of graphql-upload-minimal or fix implementation.

[ceopaludetto]

Same as @sammwafy, replacing graphql-upload-minimal with graphql-upload-ts just works. In my case a guard in NestJS was throwing an error and mercurius just hanged forever, replacing via a patch did the work. The diff in the file mercurius-upload/dist/index.js

diff --git a/dist/index.js b/dist/index.js
index 702d96f368943c6030dd1f801b66efb47ac7553a..35640696bf121ece6997f149264edf8d47366d31 100644
--- a/dist/index.js
+++ b/dist/index.js
@@ -30,7 +30,7 @@ exports.mercuriusUpload = void 0;
 const util = __importStar(require("util"));
 const stream_1 = __importDefault(require("stream"));
 const fastify_plugin_1 = __importDefault(require("fastify-plugin"));
-const graphql_upload_minimal_1 = require("graphql-upload-minimal");
+const graphql_upload_minimal_1 = require("graphql-upload-ts");
 const finishedStream = util.promisify(stream_1.default.finished);
 const mercuriusGQLUpload = (fastify, options, done) => {
     fastify.addContentTypeParser('multipart', (req, _payload, done) => {

You also need to replace every usage of graphql-upload-minimal to graphql-upload-ts in your application (scalars and types)

[capJavert]

This is indeed an issue even if latest code was to be released #24 (comment) it still hangs without @ceopaludetto patch.

I did however used the base graphql-upload package instead of ts one.