New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
forever not playing nicely with pool in node 0.12 #1511
Comments
…equest/request#1511" This reverts commit 0e6fe50.
@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. |
Here you go :) 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. |
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 |
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 |
As I said earlier your test is passing, but after using |
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 |
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) |
So what is the state of the |
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
alongsidepool: {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
andforever
affect the agent, they were never intended to be compatible with each other, maybe. We'll be moving torequest.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:
The text was updated successfully, but these errors were encountered: