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(node-http-handler): defer socket event listener attachment #1384

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Sep 1, 2024

aws/aws-sdk-js-v3#6423

This PR defers the registration of socket event listeners on individual requests outgoing from node:http(s) requests.

Attaching events creates a small performance overhead (0-2ms) and I believe they are unnecessary for requests that resolve in under 3 seconds for timeouts of greater than 6 seconds.

Here is a comparison of the use-case of a request before and after this PR:

Before PR

  • request is created
  • socket event listeners are attached
  • request is submitted
  • socket event triggers event listeners
    • some don't do anything because the timeouts involved are far higher than the request duration
    • sending a keepalive probe is useless if the request completes in e.g. under a second
  • request completes, event listeners are cleaned up

After PR

  • request is created
  • no event listeners register until the request duration exceeds 3 seconds or the configured timeout is less than 6 seconds, in which case they are still immediately registered.
    • avoids sending keepalive probe too early for short duration request
    • avoids adding other socket event listeners until needed
  • request is submitted
  • request completes, event listeners are cleaned up, or event listener deferment timeouts are cleared

@kuhe kuhe requested review from a team as code owners September 1, 2024 16:03
@kuhe kuhe requested a review from sugmanue September 1, 2024 16:03
@kuhe kuhe force-pushed the feat/socket branch 4 times, most recently from 1b03d5c to 527db30 Compare September 3, 2024 19:12
@kuhe kuhe requested review from milesziemer and removed request for sugmanue September 3, 2024 19:53
@kuhe kuhe merged commit c86a02c into smithy-lang:main Sep 5, 2024
11 checks passed
@kuhe kuhe deleted the feat/socket branch September 5, 2024 19:07
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.

3 participants