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

feat: add checked socket exhaustion warning when throughput is slow #1164

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Feb 15, 2024

Addresses aws/aws-sdk-js-v3#3560

This PR adds a warning:

@smithy/node-http-handler:WARN socket usage at capacity=<X> and <Y> additional 
requests are enqueued. 
See https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/node-configuring-maxsockets.html

Where X is the configured maxSockets for the Node http(s) agent, and Y is the number of open requests.

This warning is emitted in the following situation:

  • a request is taking more than socketAcquisitionWarningTimeout ?? connectTimeout + requestTimeout to complete.
    • indicates delay in acquiring a socket
  • no similar warning has been emitted in the last 15 seconds.
    • prevent log spam
  • the number of active sockets is equal to maxSockets
    • indicates socket saturation
  • the number of enqueued requests is greater than or equal to 2x maxSockets
    • indicates socket saturation

Notes:

  • it's easy to reach the maxSocket limit even in normal usage. This is why we can't immediately emit the warning on reaching the limit.
  • this is why the PR uses a timeout function, and only when there is a backed up queue of requests both in timing and in request count is a warning emitted.

@kuhe kuhe requested review from a team as code owners February 15, 2024 19:36
@kuhe kuhe requested a review from haydenbaker February 15, 2024 19:36
@kuhe kuhe force-pushed the feat/sockets branch 3 times, most recently from 0c8b150 to a47f1c3 Compare February 15, 2024 20:11
Comment on lines +37 to +44
/**
* Delay before the NodeHttpHandler checks for socket exhaustion,
* and emits a warning if the active sockets and enqueued request count is greater than
* 2x the maxSockets count.
*
* Defaults to connectionTimeout + requestTimeout or 3000ms if those are not set.
*/
socketAcquisitionWarningTimeout?: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before, can we mark this with @internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatives are to not let this timeout be configurable, or have a different opt-in to the warning.

Since users are dealing with a mysterious process exit, having the log output without opt-in I think would be better for their debugging experience.

@kuhe kuhe merged commit 1e23f96 into smithy-lang:main Feb 19, 2024
7 checks passed
@kuhe kuhe deleted the feat/sockets branch February 19, 2024 17:45
@siebrand
Copy link

This causes generalui/s3p#89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants