ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.57k stars 1.33k forks source link

Multipart request (formData) can raise double callback #747

Open djaibi opened 8 years ago

djaibi commented 8 years ago

Hi,

I have encountered a problem with supertest, in the case we do request with attach (multipart) it will use a formData instance and this instance will be pipped to req : https://github.com/visionmedia/superagent/blob/master/lib/node/index.js#L1017.

But if for some reason the file is not completely uploaded and we send back a response (because of an error for example). This part of the code will be reached:

https://github.com/visionmedia/superagent/blob/master/lib/node/index.js#L862

And self.callback will be called . The connection will be closed, so req will raise an error event

[Error: write ECONNRESET] code: 'ECONNRESET', errno: 'ECONNRESET', syscall: 'write' (https://github.com/visionmedia/superagent/blob/master/lib/node/index.js#L731).

In the end the callback is called a second time.

Maybe I'm doing something wrong, Is this behaviour intended ?

mleanos commented 8 years ago

I'm experiencing the same behavior. I've described it here, https://github.com/visionmedia/supertest/issues/230, and it may be related to this as well, https://github.com/visionmedia/supertest/issues/258

In my case, I am using .attach with a request, and this particular test checks to make sure a User is logged in, before attempting to upload the file; this properly send back a 400 response in the form of res.status(400).send(... This is what I want my API method to do. However, it does appear that since the response is sent back so quickly, the attached file isn't loaded into memory (shooting from the hip here, as I don't know much about how the attach feature works), thus causing the ECONNRESET error.

I have tested this with varying file sizes. A file size of somewhere between 44 KB & 50 KB will exhibit this behavior. A file size of 44 KB never has this issue. Thus, my conclusion is that superagent's .attach method hasn't completed what it is doing, due to the file size, before I send back the response to my test.

I hope this helps in determining the issue. I realize that I have no need to use the .attach method with this use case, and I have since removed it from the test in question. However, I think this is definitely a bug somewhere in superagent's attach method, or somewhere else down the line as it gets propagated through the assertion process.

sherodtaylor commented 8 years ago

This is still happening when I use attach. It calls back twice one with res defined and then another with undefined res.

    superagent
      .post(`${domain}/api/bitcoinaddresses`)
      .attach('bitcoin_addresses', files, filename)
      .end(function(err, res) {
        cb(err, res, res.body);
      });
fractalawareness commented 7 years ago

I get the same exact behavior as mleanos described even in the v.2.0.0

kornelski commented 7 years ago

What about v2.2?

timendez commented 6 years ago

I am still experiencing this problem in v3.0.0, notably, I am using two .attach functions

kornelski commented 6 years ago

Can you provide more information? The attach functions don't set up any callbacks, so there may be other options/server response headers that cause it.

timendez commented 6 years ago

Oh, woops, I am using supertest, not superagent. Do you still want to see my code?

kornelski commented 6 years ago

Only if you can reproduce the bug without supertest involved. Otherwise report the problem to supertest.