graphql-upload: Calling .destroy() when uploading multiple files does not work correctly

This escalated from a question I asked last week (https://github.com/jaydenseric/graphql-upload/issues/151). It’s since been closed and my comment possibly went unnoticed, hence a new issue.

Using:

graphql-upload 8.0.6 apollo-server-express 2.0.5 or 2.6.2 - same behaviour with both, either way most likely not an Apollo issue

Mutation schema:

testUploads(uploads: [Upload]): Boolean

Resolver:

testUploads: (_, { uploads }) => {
  console.log("Mutation called");

  return Promise.all([
    uploads[0].then(file => {
      console.log("Upload 0 resolved");
      const stream = file.createReadStream();
      stream.resume(); // LINE 1
    }),
    uploads[1].then(file => {
      console.log("Upload 1 resolved");
      const stream = file.createReadStream();
      stream.resume(); // LINE 2
    })
  ]).then(() => true);
}

I am calling this code via the Insomnia client, using a manually formed multipart request:

{
  "operationName": null,
  "variables": { "uploads": [null, null] },
  "query": "mutation ($uploads: [Upload]) {  testUploads(uploads: $uploads)  }"
}

{ "0": ["variables.uploads.0"], "1": ["variables.uploads.1"] }

I am uploading two different files, their sizes do not seem to have any influence over this.

Here’s the odd behaviour:

It does not matter what I have in LINE 1 - could be stream.resume(), stream.destroy(), a stream.pipe(process.stdout), or it could be commented out entirely (i.e. a stream is created via createReadStream() but nothing is ever done with it!), this has no influence to how the code works.

Now, LINE 2 can also be just about anything - a stream.resume(), stream.pipe(process.stdout), or commented out. Whatever the combination of LINE 1 and LINE 2, the code appears to work fine, i.e. the mutation gets called each time, the uploads resolve and I get a response with the value true, as expected. I don’t see any increase in the memory usage. Perhaps the unused uploads are piling up somewhere in the filesystem, but right now that’s beyond my concerns.

There is one important exception, however: if LINE 2 is stream.destroy(), then this happens:

The first time I call the mutation everything seems to be working fine - both uploads resolve and I get my response.
However, the second time I call it, the request just hangs - it does not even reach my HTTP loggers. I suspect it doesn’t even hit Express itself, so not sure what is actually going on.
Here’s the fun part. If I cancel the request and then fire another one - it works okay again.
If I then fire a fourth request, you guessed it - it hangs.

From hereon, every other request succeeds, and every other hangs.

The behaviour seems to at least partially defy statements previously made by @mike-marcacci:

The bug only manifests itself when I use stream.destroy() on the second upload, so in some way the order of upload processing (or whatever this is) does matter.
If I do nothing with both streams, there really is no problem, so it doesn’t seem like I have to do anything with the streams at all, even after they have been created via createReadStream().

Turns out a properly working stream.destroy() is more important than I first thought. The new .pipeline() method plus the original pipe library rely on using destroy(). Also, while I could fully work around this problem by dropping .pipeline() and using stream.resume() to waste faulty streams, it’s quite a bit of work + thinking in a complex stream processing system and I believe it may not be too efficient and is not appropriate semantically.

Thank you for your continued work and support!

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 17 (1 by maintainers)

Commits related to this issue

Use express response methods We should not bypass the express conventions here. Doing so prevents other middleware from working as intended. This includes `graphql-upload`, which is currently unab... — committed to mike-marcacci/graphql-server by mike-marcacci 5 years ago

Most upvoted comments

@juona RE fs-capacitor, both are true. The reason that library exists at all is that it’s possible that your resolvers need to use uploads in a different order than they’re sent in the HTTP payload, and it’s also possible for you to use the same upload variable multiple times in your GraphQL query.

Because of this, we have to potentially buffer whole uploads. Of course, we can’t rely on these fitting into memory, and so temporary files are used instead. If your resolvers are available to consume the uploads as they arrive, they receive the content without waiting for the whole thing to be buffered; it also allows a new read stream to be attached at any point in its lifecycle (which will start by consuming data already buffered to the file). The library is very good about garbage collecting these, and makes sure to use the temporary files directory, so any files that were not removed by the library (due to, say, a power failure) are removed by the OS.

mike-marcacci on Jun 14, 2019

Hello there,

Thank you @mike-marcacci so much for the investigation and the detailed explanation and actions taken. It’s not the first time you had to deal with problems @apollo-server, sorry about that. I was kinda hoping the defect would be somewhere in the upload lib, as seeing how you two work, I could’ve probably expected a quick solution : ]

One more question. You mentioned that with fs-capacitor you buffer the uploads into the file system. You did mean to use the word “buffer”, right? I.e. you are not really saving the whole files to the file system and then reading from them again, you are merely buffering the stream?

I never really gave any thought to this, but now I was wondering why fs-capacitor is not helping mitigate this problem, and that is exactly why, yes? Hope this is not a tremendously stupid question.

P. S. I have already learned to always use my own instance of graphql-upload.

P. P. S. Now I am one of the 0.1% too. Thanks!

juona on Jun 14, 2019

I’ve opened a PR to fix this w/ Apollo.

mike-marcacci on Jun 14, 2019

@juona thanks again so much for the intensely diligent sleuthing and fantastic demo repo. I was able to find the exact problem, which happens to be this line in apollo-server-express.

Here’s what’s happening:

The request begins; node processes the headers; graphql-upload processes the map and creates upload promises.
Apollo is called and in turn calls the resolvers.
The resolvers finish before the request stream has finished (an error might have occurred, an upload might be intentionally ignored, etc).
Apollo bypasses express and instead of callingres.send(graphqlResponse), they use the raw ServerResponse stream methods res.write(graphqlResponse); res.end().

This causes node to try to send the response over the connection while the request is still pending, and puts the HTTP transaction in a state that is handled very poorly by node, Chrome, and most other browsers & load balancers.

This is precisely why we’ve forced express to wait before actually sending the response.

Apollo should be following the express patterns, rather than using the escape hatch when it isn’t necessary; if they did need to use the ServerResponse methods directly, they should at least do it the right way, and wait for the request to finish.

A Mini Rant About HTTP w/ Node

All the tutorials you’ve ever read are wrong. They depend on a race condition that is never met in a simple “hello world” example but inevitably manifest as impossibly illusive behavior like the one in this issue. Whenever you write a response, you should really be doing the following, while consuming the request stream:

function send(data) {
  if (request.complete) {
    response.end(data);
  } else {
    request.on("end", () => response.end(data));
    request.resume();
  }
}

This is still necessary when using any of the major HTTP frameworks, including koa and express, although they make it possible to encapsulate this behavior in middleware (which is what we do here in this repo).

I’ll bet that fewer than 0.1% of node devs realize that this is the case, but it is. 🤷🏻‍♂️

mike-marcacci on Jun 14, 2019