Emit FormData error events as Request error events

[rwky]

As per #1336 created a clean request which passes tests.

Below is a stack trace (from 1st October 2014 sorry it's old!) caused by uploading an image to the Facebook API using request. The trace is caught by a node domain otherwise it'd be ignored.

Error: write ECONNRESET Error: write ECONNRESET
at errnoException (net.js:904:11)
at afterWrite (net.js:720:19)
 ---------------------------------------------
at Readable.on (_stream_readable.js:707:33)
at pipe (tls.js:1455:10)
at exports.connect (tls.js:1352:19)
at Agent.createConnection (https.js:79:14)
at Agent.createSocket (http.js:1293:16)
at Agent.addRequest (http.js:1269:23)
at new ClientRequest (http.js:1416:16)
at exports.request (https.js:123:10)
 ---------------------------------------------
at Stream.pipe (stream.js:57:10)
at CombinedStream.pipe (/node_modules/request/node_modules/form-data/node_modules/combined-stream/lib/combined_stream.js:63:25)
at end (/node_modules/request/request.js:415:22)
at /node_modules/request/request.js:444:11
at /node_modules/request/node_modules/form-data/lib/form_data.js:273:5
at /node_modules/request/node_modules/form-data/node_modules/async/lib/async.js:254:17
at done (/node_modules/request/node_modules/form-data/node_modules/async/lib/async.js:135:19)
at /node_modules/request/node_modules/form-data/node_modules/async/lib/async.js:32:16

As you can see there is an ECONNRESET in combined_stream which is passed up to the form-data module, however this error is not then passed up to request, unless domains are in place it is just lost.

This change will pass the error up to request so users can act on the 'error' event, in my case I'd retry the request. The only impact this should have on existing applications is they emit error events more often if the application uses form-data instead of ignoring it.

The alternative option would be to in the application would be to listen to the 'error' event on the form object created from request.form() which I'm happy to do but I thought since it's used by request it'd be good if it could be sent to the callback.

I would write a test case for it but I've no idea how to trigger an ECONNRESET in combine_stream intentionally, if anyone knows how I'll gladly write a test case!

[rwky]

The tests pass when I run them locally, looks like something is up with Travis?

[simov]

@rwky I think any error emitted from combined-stream/form-data would do it, not necessarily ECONNRESET

[coveralls]

Coverage decreased (-0.42%) when pulling a661e80 on rwky:formdata-error into 7208fd4 on request:master.

[rwky]

@simov would you be happy merging this in if I added tests? I can rummage through form-data and see if there's an error I can trigger easily.

[coveralls]

Coverage decreased (-0.16%) when pulling 03c5298 on rwky:formdata-error into 7208fd4 on request:master.

[simov]

First of all I forgot to thank you for the thorough description of that task. Personally I don't think re-emitting the form-data errors there would do any harm, because we're not adding any opinion to it.

As for the test it is needed just to state that this behavior is expected, also the Travis build should pass, no idea what's the problem.

[nylen]

It would be nice to have a test for this, but if it's too hard then we'll take it without one.

The Travis failure appears to be because the Travis network has exceeded its GitHub request limit for the day. I'll rerun the build again tomorrow.

[nylen]

Still getting that same Travis failure. It's not a problem with this PR. I'll address this in a separate issue and try to get the person who submitted our browser tests involved.

@rwky have you had a chance to further look into a test case?

[rwky]

I've added a simple test case, fortunately form-data has an easy way of generating an error by passing an array as a value to the append method. Looks like Travis is behaving again 😄

[simov]

Looking good to me, besides the port number.

[rwky]

I copied the url from the form-data tests, it never gets called since the error is generated before request sends any data.

[nylen]

#1342 fixed that issue with the Travis build (not applied here, of course, but it was just an intermittent issue).

@simov @rwky we need to abort the request when we receive an error from form-data. Right now, it will still complete in the background, just with who-knows-what being sent as the body.

[nylen]

@rwky re your latest comment, we're in the process of standardizing test ports to 6767, and the runTest function was a harness for running multiple tests, which isn't needed here. I also prefixed 'form-data: ' to the error message to make it more clear where these errors are coming from.

Can you cherry-pick this commit into this PR: nylen/request@d6bf426

[rwky]

@nylen looks good to me I've cherry picked that commit.

[nylen]

Okay then. I'll merge this tomorrow if no one objects or beats me to it. Thanks for contributing and for following up.

[rwky]

No worries happy to help. Do you have an idea when it will land in NPM?

[nylen]

I'd like to get #1344 in before a new release too. are you good with waiting at most a week for that to land?

[rwky]

Yep I'm happy to wait, no rush I'm currently using git+https url in package.json from my own repo so it's not a problem, just like to know when I can check back to switch to the official package.

[nylen]

👍 I'll let you know when it's out.

[nylen]

@rwky version 2.52.0 is out on npm with this change.

[nylen]

@rwky use 2.53.0 instead (see #1395, #1396).

[rwky]

@nylen Excellent thanks!