Improve the processRequest function.

Closes #272 .
jaydenseric · Nov 1, 2021 · a9980f2 · a9980f2
1 parent b479709
commit a9980f2
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 50 deletions.
diff --git a/changelog.md b/changelog.md
@@ -14,6 +14,11 @@
 - Test the `processRequest` function with a [GraphQL multipart request](https://github.com/jaydenseric/graphql-multipart-request-spec) that has no files.
 - Test the `processRequest` function with an unparsable multipart request.
 - Replaced the [`form-data`](https://npm.im/form-data) dev dependency with [`formdata-node`](https://npm.im/formdata-node), [`formdata-node`](https://npm.im/form-data-encoder), and [`node-abort-controller`](https://npm.im/node-abort-controller) and refactored tests to align with web standards.
+- Improved the `processRequest` function:
+  - Fixed ending requests from being handled incorrectly as aborting in edge cases, closing [#272](https://github.com/jaydenseric/graphql-upload/pull/272).
+  - Fixed read streams created via the resolved `Upload` scalar value `createReadStream` method:
+    - Not emitting the `error` event when the multipart request is aborted certain ways while the file is uploading.
+    - Emitting incorrect `error` event details for multipart request file field parse errors.
 
 ## 12.0.0
 

diff --git a/public/processRequest.js b/public/processRequest.js
@@ -54,7 +54,7 @@ module.exports = function processRequest(
   return new Promise((resolve, reject) => {
     let released;
     let exitError;
-    let currentStream;
+    let lastFileStream;
     let operations;
     let operationsPath;
     let map;
@@ -77,14 +77,24 @@ module.exports = function processRequest(
      * @ignore
      */
     const exit = (error) => {
+      // None of the tested scenarios cause multiple calls of this function, but
+      // it’t still good to guard against it happening in case it’s possible now
+      // or in the future.
+      // coverage ignore next line
       if (exitError) return;
+
       exitError = error;
 
       reject(exitError);
 
       parser.destroy();
 
-      if (currentStream) currentStream.destroy(exitError);
+      if (
+        lastFileStream &&
+        !lastFileStream.readableEnded &&
+        !lastFileStream.destroyed
+      )
+        lastFileStream.destroy(exitError);
 
       if (map)
         for (const upload of map.values())
@@ -101,42 +111,9 @@ module.exports = function processRequest(
       });
     };
 
-    /**
-     * Releases resources and cleans up Capacitor temporary files. Successive
-     * calls have no effect.
-     * @kind function
-     * @name processRequest~release
-     * @ignore
-     */
-    const release = () => {
-      if (released) return;
-      released = true;
-
-      if (map)
-        for (const upload of map.values())
-          if (upload.file) upload.file.capacitor.release();
-    };
-
-    /**
-     * Handles when the request is closed before it properly ended.
-     * @kind function
-     * @name processRequest~abort
-     * @ignore
-     */
-    const abort = () => {
-      exit(
-        createError(
-          499,
-          'Request disconnected during file upload stream parsing.'
-        )
-      );
-    };
-
     parser.on(
       'field',
       (fieldName, value, fieldNameTruncated, valueTruncated) => {
-        if (exitError) return;
-
         if (valueTruncated)
           return exit(
             createError(
@@ -248,10 +225,7 @@ module.exports = function processRequest(
     );
 
     parser.on('file', (fieldName, stream, filename, encoding, mimetype) => {
-      if (exitError) {
-        ignoreStream(stream);
-        return;
-      }
+      lastFileStream = stream;
 
       if (!map) {
         ignoreStream(stream);
@@ -263,11 +237,6 @@ module.exports = function processRequest(
         );
       }
 
-      currentStream = stream;
-      stream.on('end', () => {
-        currentStream = null;
-      });
-
       const upload = map.get(fieldName);
 
       if (!upload) {
@@ -297,7 +266,7 @@ module.exports = function processRequest(
       stream.on('error', (error) => {
         fileError = error;
         stream.unpipe();
-        capacitor.destroy(exitError);
+        capacitor.destroy(fileError);
       });
 
       const file = {
@@ -348,12 +317,24 @@ module.exports = function processRequest(
 
     parser.once('error', exit);
 
-    response.once('finish', release);
-    response.once('close', release);
+    response.once('close', () => {
+      released = true;
+
+      if (map)
+        for (const upload of map.values())
+          if (upload.file)
+            // Release resources and clean up temporary files.
+            upload.file.capacitor.release();
+    });
 
-    request.once('close', abort);
-    request.once('end', () => {
-      request.removeListener('close', abort);
+    request.once('close', () => {
+      if (!request.readableEnded)
+        exit(
+          createError(
+            499,
+            'Request disconnected during file upload stream parsing.'
+          )
+        );
     });
 
     request.pipe(parser);