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

Documentation states maxSockets is limited to 50 #1959

Closed
iwt-janbrauer opened this issue Jan 26, 2021 · 8 comments · Fixed by #2024
Closed

Documentation states maxSockets is limited to 50 #1959

iwt-janbrauer opened this issue Jan 26, 2021 · 8 comments · Fixed by #2024
Labels
documentation This is a problem with documentation. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.

Comments

@iwt-janbrauer
Copy link

iwt-janbrauer commented Jan 26, 2021

Describe the issue with documentation
See https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/node-configuring-maxsockets.html.
It states:

When using the default of https, the SDK takes the maxSockets value from the globalAgent. If the maxSockets value is not defined or is Infinity, the SDK assumes a maxSockets value of 50.

But this is only true for the old version of the SDK. v3 does not have this logic. Thus it will use Infinity and quickly exhaust the limit.

To Reproduce (observed behavior)

  • Write a Lambda func with v3
  • Make a lot of requests to DynamoDB (30000 items / PutItem)
  • See Error: connect EMFILE 52.94.17.26:443 - Local (undefined:undefined) in the log output

Expected behavior
This shouldn't have happened, because the DynamoDB client should not run out of file handles.

Screenshots

{
    "errorType": "Runtime.UnhandledPromiseRejection",
    "errorMessage": "Error: connect EMFILE 52.94.17.26:443 - Local (undefined:undefined)",
    "reason": {
        "errorType": "Error",
        "errorMessage": "connect EMFILE 52.94.17.26:443 - Local (undefined:undefined)",
        "code": "EMFILE",
        "errno": "EMFILE",
        "syscall": "connect",
        "address": "52.94.17.26",
        "port": 443,
        "$metadata": {
            "attempts": 1,
            "totalRetryDelay": 0
        },
        "stack": [
            "Error: connect EMFILE 52.94.17.26:443 - Local (undefined:undefined)",
            "    at internalConnect (net.js:923:16)",
            "    at defaultTriggerAsyncIdScope (internal/async_hooks.js:364:12)",
            "    at GetAddrInfoReqWrap.emitLookup [as callback] (net.js:1066:9)",
            "    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:68:8)"
        ]
    },
    "promise": {},
    "stack": [
        "Runtime.UnhandledPromiseRejection: Error: connect EMFILE 52.94.17.26:443 - Local (undefined:undefined)",
        "    at process.<anonymous> (/var/runtime/index.js:35:15)",
        "    at process.emit (events.js:314:20)",
        "    at processPromiseRejections (internal/process/promises.js:209:33)",
        "    at processTicksAndRejections (internal/process/task_queues.js:98:32)"
    ]
}
@iwt-janbrauer iwt-janbrauer added the documentation This is a problem with documentation. label Jan 26, 2021
@trivikr
Copy link
Member

trivikr commented Feb 10, 2021

The default maxSockets of 50 was added in AWS SDK for JavaScript (v2) for performance tuning in v2.4.7 in PR aws/aws-sdk-js#1057

@trivikr
Copy link
Member

trivikr commented Feb 10, 2021

The Node.js sets Infinity for maxSockets by default.
Docs: https://nodejs.org/api/http.html#http_agent_maxsockets

@trivikr
Copy link
Member

trivikr commented Feb 10, 2021

I couldn't find internal discussion on how we arrived at the value of 50 for maxSockets five years ago. It could have been mainly done as Node.js switched from maxSockets=5 to maxSockets=Infinity when moving from 0.10 to 0.12 release.

The Node.js runtime updated maxSockets from 5 to Infinity in commit nodejs/node@9fc9b87 primarily to prevent deadlocks when more connections are made.

The reasons why v3 may switch to maxSockets default of 50:

  • Most customers of AWS SDK for JavaScript who tune performance of their applications are aware of this default from v2 Developer Guide, and might be expecting v3 to do the same.
  • Explicitly setting maxSockets is recommended in multiple blog posts on Node.js performance. The actual default value ranges depending on the type of application and type of hardware the backend is run on.
  • Node.js doesn't turn on HTTP keepAlive by default, but AWS SDK for JavaScript (v3) does. Having said that, this default behavior of Node.js is not HTTP-spec compliant and there is a new request from multiple maintainers of Node.js to change the behavior in future major versions.

The reasons why v3 may not switch to maxSockets default of 50:

  • The AWS SDK for JavaScript (v3) is generally available since December 2020, and this would be a breaking change for existing customers.
  • The value for maxSockets should be decided by type of application and the server it runs on, and not by SDK.
  • The main reason for adding maxSockets of 50 in v2 appears to be because the default value was changed in Node.js v0.12.0 from 5 to Infinity, both versions were supported in v2. The default for maxSockets has remained Infinity since then, and there hasn't been any major ask to update this value. The minimum Node.js version supported by AWS SDK for JavaScript (v3) is 10.x whose users would be accustomed to having maxSockets set to Infinity by default.

After listing these reasons, the AWS SDK for JavaScript (v3) is likely to stick with default value used by Node.js runtime for maxSockets. This issue will be discussed in one of the future post-scrums by the team, and an update will be provided here.

@trivikr
Copy link
Member

trivikr commented Feb 10, 2021

@iwt-janbrauer: Do you expect AWS SDK for JavaScript (v3) to set default value for maxSockets?
If yes, any reasons on why it should be done and what the value should be?

@trivikr trivikr added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Feb 10, 2021
@iwt-janbrauer
Copy link
Author

I don't have any further expectations other than the pro arguments you raised already.

I expect the documentation to be in line with what the code is actually doing, and I expect it to work out of the box (especially when using AWS Lambda as that is sold as a "it just works" solution).
Or if sticking to Infinity raise an exception that explicitly states I should take a look at the underlying HTTP agent settings.

@trivikr
Copy link
Member

trivikr commented Feb 11, 2021

During post-scrum discussion on 2/11, we decided to go ahead with default value of 50 for maxSockets for the following reasons:

  • SDKs are expected to set reasonable defaults for the user to ease usage for customers.
  • The default value for maxSockets=Infinity set by Node.js throws errors like EMFILE, too may open files documented in this issue.
  • The default value for maxSockets=50 set by AWS SDK for JavaScript (v2) seems reasonable, based on legacy code and suggestions in various blog posts on Node.js performance.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation This is a problem with documentation. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants