Apollo must wait to respond until request completes

[mike-marcacci]

It appears that all the apollo-server- packages suffer from a bug when using graphql-upload. It is essential to wait for an entire HTTP request to be consumed before responding, or you risk causing very hard-to-diagnose bugs, including locking a browser out of subsequent requests to the server.

The middleware that comes with graphql-upload does this, but was being bypassed in graphql-upload-express, which I fixed here. However, I noticed that the upload middleware bundled with Apollo fails to wait, and will experience these bugs even with that fix.

Please see this issue for context and details (specifically this comment) the accompanying demonstration repo put together with heroic effort by @juona.

---

This middleware in apollo-server-express might be able to borrow from this one in graphql-upload/express.

This middleware in apollo-server-koa might take from this in graphql-upload/koa.

[n1ru4l]

I can also confirm this issue on apollo-server-koa.

I can also confirm that applying the changes from https://github.com/jaydenseric/graphql-upload/blob/837bbdfb617c740bc19fb34ee6bf1c9553f688c9/src/graphqlUploadKoa.mjs#L29..L40

to apollo-server-koa this issue is resolved.

@mike-marcacci Nevertheless, I do not think that the server should process the whole request if he already determined that the user should not upload (e.g. rate limitings or amount of allowed files is exceeded). Is this a known limitation of graphql-upload, or is there a solution to this problem?

[mike-marcacci]

@n1mmy this is not really a limitation of graphql-upload, but rather HTTP itself (or at least all tested implementations including every major browser and load balancer). The problem is that if the connection is closed before the initiator (ie. the browser) or intermediate proxies have finished sending the entire payload, then the entire request is immediately treated as a failure.

The only correct way to handle this is to delay closing the connection until the entire request has been received. Now, when testing this with especially small payloads, you may not see this behavior since node (or the server OS) can buffer the remainder of the request such that entire payload has been sent even though it hasn't been received. This is what makes this issue so insidious: it often doesn't appear in tests, where we use small mock files, then rears its head nondeterministically in production.

You may note that in graphql-upload we wait for the request to finish before we begin sending the response (as opposed to just closing the connection), so there is technically room for optimization where multiplexing (ie. HTTP2) is possible. However, the scenario of an early return is a fairly uncommon case, and its payloads tend to be small (such as an error message), negating any real-world gains. Further, it becomes challenging to take over connection closing from the standard library because we do still want to allow connections to close prematurely for other reasons (such as timeouts) and we have no way of knowing the intent when .close() is called. Instead, we simply rely on the conventions of the framework (koa, express, etc) to delay sending of the response until we can be certain it's safe to do so.

[rnenjoy]

I wonder if this is the issue in having. I thought it was depending on file size, but maybe its not

[rnenjoy]

I maybe also have fixed my apollo-server-graphql by adding:

const finished = new Promise(resolve => req.on('end', resolve));

        const { send } = res;
        res.send = (...args) => {
            finished.then(() => {
                res.send = send;
                res.send(...args)
            });
        };

Above processFileUploads in ApolloServer.js

[n1ru4l]

Patch for apollo-server-koa@2.9.7:

diff --git a/node_modules/apollo-server-koa/dist/ApolloServer.js b/node_modules/apollo-server-koa/dist/ApolloServer.js
index 51a1e42..4ced9a4 100644
--- a/node_modules/apollo-server-koa/dist/ApolloServer.js
+++ b/node_modules/apollo-server-koa/dist/ApolloServer.js
@@ -35,6 +35,7 @@ var apollo_server_core_2 = require("apollo-server-core");
 exports.GraphQLExtension = apollo_server_core_2.GraphQLExtension;
 const fileUploadMiddleware = (uploadsConfig, server) => (ctx, next) => __awaiter(void 0, void 0, void 0, function* () {
     if (type_is_1.default(ctx.req, ['multipart/form-data'])) {
+        const finished = new Promise(resolve => ctx.req.on('end', resolve))
         try {
             ctx.request.body = yield apollo_server_core_1.processFileUploads(ctx.req, ctx.res, uploadsConfig);
             return next();
@@ -46,6 +47,8 @@ const fileUploadMiddleware = (uploadsConfig, server) => (ctx, next) => __awaiter
                 formatter: server.requestOptions.formatError,
                 debug: server.requestOptions.debug,
             });
+        } finally {
+            yield finished
         }
     }
     else {

[owengombas]

Same issue here, when I try to upload upload multiple files I randomly get an "Network Error: Failed to fetch" error...

[n1ru4l]

@OwenCalvin we finally decided to upload to AWS S3 directly. It reduces the server load and we now simply generate pre-signed URLs.

[vanyadymousky]

Hello, noticed there are no updates for almost a year - is this is hard to use compatibility with graphql-upload/koa or graphql-upload/express? Want to up this issue, it's pretty critical one.

[glasser]

Apollo Server 3 no longer has a built-in integration with graphql-upload.

If the issues still applies (eg, if manual integration with graphql-upload suffers from the same problem in a way that requires changes to Apollo Server to fix), let me know and we can reopen!