forever not playing nicely with pool in node 0.12

[summer4096]

It seems like the forever option isn't documented (anymore?) so I'm not sure how intended this behavior is, or if it's the result of using a deprecated feature.

We've used the forever: true alongside pool: {maxSockets: 1024} in the past. This has worked well, but recently we've found that it throws an error in node 0.12.

Here's a failing test I wrote for that: faradayio@abe8b55

It's possible that since both pool and forever affect the agent, they were never intended to be compatible with each other, maybe. We'll be moving to request.forever({maxSockets: 1024}).

Should request throw a warning if the user tries to combine these options? Are they supposed to work together?

Here's the error that occurs when I try to combine them in 0.12:

TypeError: Cannot read property 'length' of undefined
    at ForeverAgent.<anonymous> (/redacted/request/node_modules/forever-agent/index.js:22:34)
    at ForeverAgent.emit (events.js:110:17)
    at Socket.onFree (_http_agent.js:202:10)
    at Socket.emit (events.js:104:17)
    at _http_client.js:453:14
    at process._tickCallback (node.js:355:11)

[summer4096]

See request/forever-agent#27

[simov]

@devtristan it's great that you wrote a failing test, create a PR with it here, so that anyone can see it and test it.

[summer4096]

Here you go :)

#1516

And the test results: https://travis-ci.org/request/request/builds/56143997

Using request/forever-agent#27 causes this test to pass, although forever-agent doesn't have tests or travis integration so I'm not sure how to demonstrate that.

[simov]

I'm not sure how your failing test is related to request/forever-agent#27

Your test is passing either way, but for some reason the server's close event is not being fired and times out Assert: "Test file timed out: test-pool.js"

[summer4096]

I don't know why it fails on 0.10, that may be another issue entirely.

The pull request on forever-agent fixes this issue. When I run the tests using the forked forever-agent from the pull request, they pass.

What's going on, as far as I can tell, is that the host comes in as an object instead of a string in recent versions of node and in iojs. Forever-agent looks up connections by a key starting with [object Object] and tries to get the length without first checking for existence. Since the object Object key hasn't been created, it throws the error. I'm not exactly sure why this seems to only happen when a pool is specified.

[simov]

As I said earlier your test is passing, but after using forever:true the test times out, when trying to close the server. Similar to what happens here request/forever-agent#12 so while the test times out it is not passing. What should we do about it?

[summer4096]

Oh, it is. The exception is thrown asynchronously. I pushed a new commit that makes it fail properly - https://travis-ci.org/request/request/builds/56937948.

I would suggest using Agent.destroy but it's new and not available in older versions of node.

I've been using this

[simov]

This is how your test should look like:

tape('forever', function(t) {
  var r = request({
    url: 'http://localhost:6767',
    forever: true,
    pool: {maxSockets: 1024}
  }, function(err, res, body) {
    // explicitly shut down the agent
    if (r.agent.destroy === typeof 'function') {
      r.agent.destroy()
    } else {
      // node < 0.12
      Object.keys(r.agent.sockets).forEach(function (name) {
        r.agent.sockets[name].forEach(function (socket) {
          socket.end()
        })
      })
    }

    t.equal(err, null)
    t.equal(res.statusCode, 200)
    t.equal(body, 'asdf')

    var agent = res.request.agent
    t.equal(agent.maxSockets, 1024)
    t.end()
  })
})

Can you update your test with the code from above and squash the commits into one? No need for using domain there.

Now this test not only fails, but it also passes with the fix from request/forever-agent#27 (i.e. no timeout)

[vvo]

So what is the state of the forever option? How does one uses request/request with keepalived connections in Node 0.10/0.12/iojs?