Skip to content
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

Respect the global agent maxSocket property #224

Closed
wants to merge 3 commits into from
Closed

Conversation

nscott
Copy link

@nscott nscott commented Jan 31, 2014

Currently if someone uses an AWS service and they use SSL, http(s).globalAgent.maxSockets is ignored entirely when this new agent is created. You also can't configure maxSockets by passing in config, as that is also ignored in this case.

By default, node pools its connections. That means if you have a shared agent, and you have more than 5 connections open (because the default maxSockets is 5), you're going to block until a connection is available for use. This is particularly bad when you're trying to rapidly pull and push from something like SQS.

The simplest test is to make a queue consumer for SQS. Create 10 workers that will continually try eating off the queue, and then try sending a message with the current library. The response will take a very long time to return.

See https://gist.github.com/nscott/8735628 for an example in CoffeeScript. Run it with the current aws-sdk, and then run it with this revision. You'll see that sendMessage()'s callback hits very quickly, as opposed to waiting a very long time. receiveMessage() can also pick up on those messages faster.

A temporary, injectable solution is to overwrite the prototype. See the coffescript at https://gist.github.com/nscott/8735708.

@lsegal
Copy link
Contributor

lsegal commented Jan 31, 2014

Unfortunately this patch won't actually work because we only ever create the sslAgent object one time on the first request. This means that if you adjust https.globalAgent.maxSockets mid-way through your program, you will get unexpected results-- that change will not be reflected in the SDK. I would rather not change the agent behavior to be less consistent. Fortunately there is a way to delegate the property with Object.defineProperty, so I will work on a patch based on yours that can properly delegate maxSockets (and other settings).

That said, note that you can configure maxSockets in the SDK, either by creating your own agent, or by passing the globalAgent into your client. The SDK has an httpOptions configuration value that accepts an agent object:

// use global agent
AWS.config.httpOptions = { agent: require('https').globalAgent };

// alternatively, create your own
var agent = new https.Agent();
agent.maxSockets = 10;

// optionally pass it into the client rather than globally
var s3 = new AWS.S3({httpOptions: {agent: agent}});

Note that there are reasons not to use the global agent, specifically in 0.8.x, where the rejectUnauthorized setting defaults to false, which opens you up to man-in-the-middle attacks. This is the reason the SDK does not use the globalAgent by default.

@nscott
Copy link
Author

nscott commented Jan 31, 2014

Uhg, maybe I'm using an outdated version of the API. I tried passing my own agent through and it never worked... Well, that would indeed fix this problem. Sorry for the trouble, and thanks.

@nscott nscott closed this Jan 31, 2014
lsegal added a commit that referenced this pull request Feb 1, 2014
This change allows the SDK to inherit the maxSockets property
from the global agent in Node.js so that users do not have to
pass in a custom agent to increase maxSockets. Instead, this
change allows users to do the following to increase maxSockets:

```javascript
require('https').globalAgent.maxSockets = 20;

var s3 = new AWS.S3();
for (var i = 0; i < 20; i++) {
  s3.getObject(params, function (err, data) { /* ... */ });
}
```

Fixes #224
@lsegal
Copy link
Contributor

lsegal commented Feb 1, 2014

I added the above change to make the SDK respect the https.globalAgent.maxSockets setting through delegation so you should not have to pass in a custom agent at all. This will be available in the next 2.x RC release, but not 1.x, since this might not be backwards compatible.

@nscott
Copy link
Author

nscott commented Feb 6, 2014

Thanks, I really appreciate that @lsegal!

@mhart mhart mentioned this pull request Feb 14, 2014
lsegal added a commit that referenced this pull request Mar 26, 2014
lsegal added a commit that referenced this pull request Apr 24, 2014
@lock
Copy link

lock bot commented Sep 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants