From e6ffbde84835d4e732688413d71fbbfa0facb45d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <mbajtoss@gmail.com>
Date: Tue, 12 Mar 2019 15:19:25 +0100
Subject: [PATCH] Fix handling of FormData errors

When a file cannot be uploaded (e.g. becase it does not exist), report
the error via callback/promise.

Before this change, FormData errors were reported via "error" event
on the request. In promise mode, such error was caught by `.then()`
handler in a way that shadowed the actual error reported by `.end()`.
In callback mode, such error was not caught at all and crashed the
process on an unhandled error.

With this change in place, FormData errors finish the request and pass
the error directly to the callback.
---
 lib/node/index.js      |  8 ++++++-
 lib/request-base.js    |  1 -
 test/node/multipart.js | 48 ++++++++++++++++++++++++++++--------------
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/lib/node/index.js b/lib/node/index.js
index 2f948a911..921382793 100644
--- a/lib/node/index.js
+++ b/lib/node/index.js
@@ -264,7 +264,13 @@ Request.prototype._getFormData = function() {
   if (!this._formData) {
     this._formData = new FormData();
     this._formData.on('error', err => {
-      this.emit('error', err);
+      debug('FormData error', err);
+      if (this.called) {
+        // The request has already finished and the callback was called.
+        // Silently ignore the error.
+        return;
+      }
+      this.callback(err);
       this.abort();
     });
   }
diff --git a/lib/request-base.js b/lib/request-base.js
index 81e06b0b1..e4346bb94 100644
--- a/lib/request-base.js
+++ b/lib/request-base.js
@@ -242,7 +242,6 @@ RequestBase.prototype.then = function then(resolve, reject) {
       console.warn("Warning: superagent request was sent twice, because both .end() and .then() were called. Never call .end() if you use promises");
     }
     this._fullfilledPromise = new Promise((innerResolve, innerReject) => {
-      self.on('error', innerReject);
       self.on('abort', () => {
         const err = new Error('Aborted');
         err.code = "ABORTED";
diff --git a/test/node/multipart.js b/test/node/multipart.js
index 6c0640a3d..e671dd4b1 100644
--- a/test/node/multipart.js
+++ b/test/node/multipart.js
@@ -73,31 +73,49 @@ describe("Multipart", () => {
     });
 
     describe("when a file does not exist", () => {
-      it("should emit an error", done => {
+      it("should fail the request with an error", done => {
         const req = request.post(`${base}/echo`);
 
         req.attach("name", "foo");
         req.attach("name2", "bar");
         req.attach("name3", "baz");
 
-        req.on("error", err => {
+        req.end((err, res) => {
+          assert.ok(!!err, "Request should have failed.");
+          err.code.should.equal("ENOENT");
           err.message.should.containEql("ENOENT");
           err.path.should.equal("foo");
           done();
         });
-
-        req.end((err, res) => {
-          if (err) return done(err);
-          assert(0, "end() was called");
-        });
       });
 
       it("promise should fail", () => {
-        request.post('nevermind')
-        .field({a:1,b:2})
-        .attach('c', 'does-not-exist.txt')
-        .then(() => assert.fail("It should not allow this"))
-        .catch(() => true);
+        return request.post(`${base}/echo`)
+          .field({a:1,b:2})
+          .attach('c', 'does-not-exist.txt')
+          .then(
+            res => assert.fail("It should not allow this"),
+            err => {
+              err.code.should.equal("ENOENT");
+              err.path.should.equal("does-not-exist.txt");
+            });
+      });
+
+      it("should report ECONNREFUSED via the callback", done => {
+        request.post('http://127.0.0.1:1') // nobody is listening there
+          .attach("name", "file-does-not-exist")
+          .end((err, res) => {
+            assert.ok(!!err, "Request should have failed");
+            err.code.should.equal("ECONNREFUSED");
+            done();
+          });
+      });
+      it("should report ECONNREFUSED via Promise", () => {
+        return request.post('http://127.0.0.1:1') // nobody is listening there
+          .attach("name", "file-does-not-exist")
+          .then(
+            res => assert.fail("Request should have failed"),
+            err => err.code.should.equal("ECONNREFUSED"));
       });
     });
   });
@@ -143,13 +161,11 @@ describe("Multipart", () => {
       request
         .post(`${base}/echo`)
         .attach("filedata", "test/node/fixtures/non-existent-file.ext")
-        .on("error", err => {
+        .end((err, res) => {
+          assert.ok(!!err, "Request should have failed.");
           err.code.should.equal("ENOENT");
           err.path.should.equal("test/node/fixtures/non-existent-file.ext");
           done();
-        })
-        .end((err, res) => {
-          done(new Error("Request should have been aborted earlier!"));
         });
     });
   });