Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

v2.25.1: globalPool not used when maxSockets are set. #617

Closed
nemtsov opened this Issue · 4 comments

4 participants

@nemtsov

If the pool is provided in v2.25.1, due to the added check in de8508e, the globalPool will not be used, unlike in the previous versions.

Here's a test:

function send(i, pool) {
  console.time(i)
  require('request')({
      url: 'http://localhost:3000/?delay=1000'
    , pool: pool
  }, function (err, res, body) {
    console.timeEnd(i)
  })
}

for (var pool, i = 0; i < 10; i++) {
  //pool = {maxSockets: 5}
  send(i, pool)
}

Due to the global pool, the requests will be chunked into two groups (of 5, the default pool size) 1000ms each; as expected.

0: 1098ms
1: 1018ms
2: 1019ms
3: 1020ms
4: 1019ms
5: 2019ms
6: 2021ms
7: 2022ms
8: 2022ms
9: 2023ms

If you uncomment the line with the maxSockets, I expect to the exactly the same behavior, but instead get:

0: 1096ms
1: 1019ms
2: 1020ms
3: 1020ms
4: 1021ms
5: 1021ms
6: 1022ms
7: 1021ms
8: 1022ms
9: 1022ms

I'd submit this as a PR, undoing de8508e, but I don't understand the motivation behind that commit enough to recommend that as a solution.

@Cauldrath

The description of pool in the documentation states: A hash object containing the agents for these requests. If omitted this request will use the global pool which is set to node's default maxSockets.

The main problem before was that if you set the size of the global pool elsewhere, then ran this code with the line uncommented, the maxSockets of the global pool would be set to 5. I'm not sure what your expected behavior would be for the socket pool maxSocket count if it did use the global pool, especially in cases where the maxSocket value varied.

If you want to set the maxSockets of the global pool, the proper way is to change http.globalAgent.maxSockets to whatever your desired value is. Then don't specify a pool for your request and it will use that.

So this is what your code would look like:

require('http').globalAgent.maxSockets = 5;

function send(i) {
  console.time(i)
  require('request')({
      url: 'http://localhost:3000/?delay=1000'
  }, function (err, res, body) {
    console.timeEnd(i)
  })
}

for (var pool, i = 0; i < 10; i++) {
  send(i)
}
@nemtsov

The main problem before was that if you set the size of the global pool elsewhere, then ran this code with the line uncommented, the maxSockets of the global pool would be set to 5. I'm not sure what your expected behavior would be for the socket pool maxSocket count if it did use the global pool, especially in cases where the maxSocket value varied.

That makes sense. On one hand, it does prevent the user from inadvertently updating globalAgent.maxSockets from request; and that's great. On the other, it changes the behavior for existing clients of the lib, who only set the {maxSockets: n} (expecting to get some pool with n number of sockets in it) and don't set their own agents in the pool; making them create/use a new agent per request instance.

If that's desired and the implications understood, this issue can be closed—I'll change the way I see pool now before upgrading to v2.25.x

@mikeal
Owner

this shit is all terrible at the node core level. luckily it's getting refactored in the current release and @isaacs promised me that it'll also be available as a standalone module much like new streams. when that happens, all of request will move to that module so that even older versions of request won't use the old pool.

@isaacs

As it turns out, refactoring out the http agent is pretty much impossible to do in the manner of streams2, since there is so much reliance on the particulars of node's networking layer for performance.

It wouldn't be too hard to write a standalone module that polyfilled for Request's usage, and everything Request does is fully supported by the changed code, since the fancy keepalive stuff is all opt-in anyway. But if you want the benefits, it's going to have to be a code change on your side, I'm afraid.

@mikeal mikeal closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.