fix: make sure the handler resolves in all cases (#507)

* fix error in processing file * fix error in processing file * add test * fix close * fix test * lint * revert change * Revert "revert change" This reverts commit 17ec56c. * node 14 :( * node 14 :( * add file * use filePath * delete foo * revert change * remove console.log * add test number * increase timeout * try fixing the error * try fixing the error * try reverting? * Revert "try reverting?" This reverts commit 35aca1e. * 90 * fix error * fix error * let request aborted destroy the file * simplify * simplify * ci
fastify · Mar 7, 2024 · 529276a · 529276a
1 parent 3f434e8
commit 529276a
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 13 deletions.
diff --git a/index.js b/index.js
@@ -230,27 +230,27 @@ function fastifyMultipart (fastify, options, done) {
     bb
       .on('field', onField)
       .on('file', onFile)
+      .on('end', cleanup)
+      .on('finish', cleanup)
       .on('close', cleanup)
-      .on('error', onEnd)
-      .on('end', onEnd)
-      .on('finish', onEnd)
+      .on('error', cleanup)
 
     bb.on('partsLimit', function () {
       const err = new PartsLimitError()
       onError(err)
-      process.nextTick(() => onEnd(err))
+      process.nextTick(() => cleanup(err))
     })
 
     bb.on('filesLimit', function () {
       const err = new FilesLimitError()
       onError(err)
-      process.nextTick(() => onEnd(err))
+      process.nextTick(() => cleanup(err))
     })
 
     bb.on('fieldsLimit', function () {
       const err = new FieldsLimitError()
       onError(err)
-      process.nextTick(() => onEnd(err))
+      process.nextTick(() => cleanup(err))
     })
 
     request.pipe(bb)
@@ -376,18 +376,15 @@ function fastifyMultipart (fastify, options, done) {
       currentFile = null
     }
 
-    function onEnd (err) {
-      cleanup()
-
-      ch(err || lastError)
-    }
-
     function cleanup (err) {
       request.unpipe(bb)
-      // in node 10 it seems that error handler is not called but request.aborted is set
+
       if ((err || request.aborted) && currentFile) {
         currentFile.destroy()
+        currentFile = null
       }
+
+      ch(err || lastError || null)
     }
 
     return parts

diff --git a/test/multipart.test.js b/test/multipart.test.js
@@ -634,3 +634,58 @@ test('should not miss fields if part handler takes much time than formdata parsi
   await once(res, 'end')
   t.pass('res ended successfully')
 })
+
+test('should not freeze when error is thrown during processing', async function (t) {
+  t.plan(2)
+  const app = Fastify()
+
+  app
+    .register(multipart)
+
+  app
+    .post('/', async (request, reply) => {
+      const files = request.files()
+
+      for await (const { file } of files) {
+        try {
+          const storage = new stream.Writable({
+            write (chunk, encoding, callback) {
+            // trigger error:
+              callback(new Error('write error'))
+            }
+          })
+
+          await pump(file, storage)
+        } catch {}
+      }
+
+      return { message: 'done' }
+    })
+
+  await app.listen()
+
+  const { port } = app.server.address()
+
+  const form = new FormData()
+  const opts = {
+    hostname: '127.0.0.1',
+    port,
+    path: '/',
+    headers: form.getHeaders(),
+    method: 'POST'
+  }
+  const req = http.request(opts)
+
+  try {
+    form.append('upload', fs.createReadStream(filePath))
+    form.pipe(req)
+  } catch {}
+
+  const [res] = await once(req, 'response')
+  t.equal(res.statusCode, 200)
+  res.resume()
+  await once(res, 'end')
+  t.pass('res ended successfully!')
+
+  await app.close()
+})