Fix request with Node.js 0.12.x ($100)

[nylen]

I'm nominating this fix for a $100 bounty. Fellow contributors, what do you think?

[mikeal]

Is request broken or are the tests just having issues? I fixed all but one test on io.js 1.2 and the fixes were entirely in the tests.

[nylen]

I've been seeing some Error: socket hang up and ECONNRESET, like what's in the HTTPS tests. Haven't had time to look into it.

[akshayp]

@nylen @mikeal I haven't been able to dig into this deeply yet but it looks like the ECONNRESET errors happen for me when the forever agent is being used. I'll post back with a reproducible test case when I have one

[rvagg]

If you want a reproducible test case you could look at the test suite in st which I can't make pass on 0.12 or io.js because of this. Run node test/multi-mount.js and you'll see it.

@othiym23 mentioned that there was a problem with npm and io.js that I have a feeling may be related to this, pulling him in here to confirm or deny.

[rvagg]

Actually .. I think this is simply just the new maxSockets so users need to be told to manage that themselves now that it's Infinity instead of 5.

Try:

const http = require('http');
const port = 1337;
const clients = 500;

var done = 0;
var agent = http.Agent()
// agent = http.Agent({ maxSockets: 100 })

var server = http.createServer(function (req, res) {
  res.write('.');
  console.log('waiting 100 @ %d', done)
  setTimeout(function () {
    console.error('ended')
    res.end('.');
  }, 100);
}).listen(port, function () {
  for (var i = 0; i < clients; i++) {
    (function (i) {
      console.log('requesting @ %d', i)
      http.get({ port: port, agent: agent }, function (req) {
        req.resume()
        req.on('end', function () {
          console.log('done %d', done - 1)
          if (++done === clients)
            process.exit(0);
        });
      })
    }(i))
  }
})

[akshayp]

@rvagg In my case I'm using the forever option which never works presumably because the HTTP Agent API changed quite a bit with node 0.12. eg:

var request = require('./index').forever({maxSockets: 1});
request('http://www.yahoo.com', function (error, response, body) {
    if (error) {
        console.log(error);
    } else {
        console.log(response.statusCode);
    }
});

leads to

{ [Error: connect ENOTSOCK] code: 'ENOTSOCK', errno: 'ENOTSOCK', syscall: 'connect' }

vs on node 0.10 it logs

200

[mikeal]

We should deprecate forever, really we should just noop it.

[akshayp]

👍 @mikeal. It'd be good to just use the default http agent on node 0.12 for people that are using the forever option as an interim solution and then deprecate in the future since it looks like request maintains compatibility for atleast 2 node versions

[othiym23]

The failure in npm is due to a strange interaction between the http[s].Agent and the way that mochi (the Erlang HTTP library used by CouchDB) handles error responses on Connection: Keep-Alive connections. mochi returns the error response and then does something that leads the node Agent to believe that the connection is still open, even though mochi has closed it. This leads to ECONNRESET and EPIPE responses that should cause that agent connection to be marked as dead and not used, but for some reason, this isn't happening, nor are the errors properly getting propagated to a place where npm-registry-client or request can handle them. Obviously, there are some unknowns on that, and I have yet to produce a reduced test case I can use to open an issue, but I think this problem is solely in Node.js / io.js core, and not in request.

It would be very nice to get this fixed for the sake of npm's tests, although it's not a production issue because all of npm's CouchDB instances being behind the CDN, haproxy, or pound, all of which do not have this problem.

[BBB]

@nylen Hopefully PR #1442 fixes the issues with the https tests

[kanongil]

@othiym23 I have experienced production issues with node 0.11.x and CouchDB that seem related. Are you tracking this issue somewhere?

[othiym23]

@kanongil I should be, but I haven't had the time to produce a reduced test case / definitively characterize the underlying bug. @isaacs and I talked it through a couple weeks ago and were both fairly sure that it was a problem with the Agent, but I need to do two things:

1. Grab some sessions with wireshark under Node 0.10 and Node 0.12 and see what's happening differently between them, and
2. read through the Agent code and see if I can see where it's jumping the shark.

1 is made more difficult by the fact that the tests that are failing for npm are fairly tightly coupled to each other and to a specific CouchDB app (npm-registry-couchapp), but mostly I just need a few uninterrupted hours to hammer on this and then I will probably have a PR for one or both of io.js and Node.

[mikeal]

We should remove that warning in the README. There is only one known issue (that i can find) on 0.12.x and it's in the SSL handling. The warning should be modified to point out the known issue rather than state that we're broken entirely on 0.12.x. Most of the failing tests are issues with the tests themselves.

[fiznool]

@mikeal 👍 - it's quite unnerving to see the README warning, especially as there isn't much context behind it.

[morficus]

+1 for updating the warning in the README to add more context (or at the very least link to the issue explaining it).

[nickmccurdy]

@mikeal 👍

[akshayp]

The Readme should probably still point out that forever agent is the only thing that doesn't work with node 0.12

[simov]

Anyone, see this #1461 and https://travis-ci.org/request/request/builds/53174289

[stuartpb]

For those of us who aren't privy to whatever out-of-band discussion explained what the failing test is, what's going on? What "known issue" is this? Does #1442 fix it, whatever it was?

[simov]

Yes, totally forgot about this one, fixed here #1468