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

Lazy initialize the default HTTP client #3262

Merged
merged 8 commits into from
Nov 30, 2023
Merged

Lazy initialize the default HTTP client #3262

merged 8 commits into from
Nov 30, 2023

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Nov 28, 2023

This change initializes the TLS trust certs at base client validation time rather than upon creation of the default HTTP client runtime plugin so that if the default is overridden, that initialization doesn't need to take place. This is especially helpful on MacOS where that initialization takes approximately 100 milliseconds.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jdisanti
Copy link
Collaborator Author

We could potentially switch to the regex-lite crate to make the first initialization faster as well.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@monken
Copy link

monken commented Nov 28, 2023

👀 You are working fast! Thank you!

@jdisanti
Copy link
Collaborator Author

Split the regex changes out of this PR into #3269

@jdisanti jdisanti changed the title Optimize client initialization time Lazy initialize the default HTTP client Nov 29, 2023
@jdisanti
Copy link
Collaborator Author

Since trusted cert initialization only occurs once per process and is only slow on MacOS, I'm currently of the opinion that we shouldn't merge this change. There may be some throw away work in the case where a client is overridden, but that amount is negligible. The primary benefit to optimizing it is for reducing AWS Lambda cold start time, but that is running Linux, and overriding the client there seems niche.

I could be persuaded otherwise though.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@rcoh
Copy link
Collaborator

rcoh commented Nov 29, 2023

Even if this isn't required from a performance perspective, you still have the issue where loading native certs doesn't even work. For example, I suspect WASM may be broken by always initializing this connector?

I think lazy-loading may be not worth it but I think we do need a solution for not loading one if one is already set.

This is also going to make a big difference in running CI on macs as well.

@jdisanti
Copy link
Collaborator Author

I believe WASM requires disabling the default hyper connector feature, so it actually still works in the present state.

I'll take a look at lazily loading the trusted certs without a behavior version change. It probably is worth it for the tests.

@jdisanti jdisanti marked this pull request as ready for review November 29, 2023 23:45
@jdisanti jdisanti requested a review from a team as a code owner November 29, 2023 23:45
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti jdisanti requested a review from a team as a code owner November 30, 2023 00:28
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

aws/rust-runtime/aws-config/src/imds/client.rs Outdated Show resolved Hide resolved
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti jdisanti enabled auto-merge November 30, 2023 19:42
@jdisanti jdisanti added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 30, 2023
@jdisanti jdisanti enabled auto-merge November 30, 2023 22:28
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti jdisanti added this pull request to the merge queue Nov 30, 2023
Merged via the queue into main with commit 9587dbc Nov 30, 2023
41 checks passed
@jdisanti jdisanti deleted the client-init branch November 30, 2023 23:38
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.

5 participants