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

test: override connect_timeout and read_timeout in clients #2333

Closed
wants to merge 3 commits into from

Conversation

Oliboy50
Copy link

@Oliboy50 Oliboy50 commented Feb 8, 2023

Motivation and Context

A simple PR to reproduce the fact that connect_timeout is not properly applied on a DynamoDB client constructed from a SDK config that already has a real http_connector set (e.g. when users run aws_config::load_from_env().await).

ℹ️ The bug is also present for read_timeout (but I don't know how to write an integration test for that).
ℹ️ The bug is not present for operation_timeout and operation_attempt_timeout (probably because they are not set on the http_connector).

Description

Only added an integration test, because I don't know how to fix this bug.

Testing

Checklist

  • waiting for a maintainer to run CI workflows and see that the new test fails
  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@Oliboy50 Oliboy50 changed the title test: connect_timeout and read_timeout bug test: override connect_timeout and read_timeout in clients Feb 9, 2023
@Oliboy50 Oliboy50 marked this pull request as ready for review February 10, 2023 20:19
@Oliboy50 Oliboy50 requested a review from a team as a code owner February 10, 2023 20:19
@rcoh
Copy link
Collaborator

rcoh commented Feb 13, 2023

yes thanks for adding this test! I had spotted this issue in #2151 . We'll try to get a fix out soon

@Velfi
Copy link
Contributor

Velfi commented Feb 13, 2023

Hey @Oliboy50, the issue here is not so much that connectors are broken but that this has a bad user-experience.

When any type that implements the SmithyConnector trait is passed to a config's http_connector setter method, it's converted into an HttpConnector::Prebuilt and can't accept any more configuration. Configuring a connector and creating it are the same thing.

However, you can pass an HttpConnector::ConnectorFn. This allows the smithy client to create new connectors as needed, and those connectors will respect configuration set at a later "stage" than the connector itself. For example, if you set a prebuilt connector in the SdkConfig, then it wouldn't pick up configuration from the service-specific config.

Here's an example of what creating and passing a ConnectorFn looks like:

let connector_fn =
    |connector_settings: &ConnectorSettings, sleep_impl: Option<Arc<dyn AsyncSleep>>| {
        let mut conn = hyper_ext::Adapter::builder()
            .connector_settings(connector_settings.clone());
        conn.set_sleep_impl(sleep_impl);

        Some(DynConnector::new(
            conn.build(
                hyper_rustls::HttpsConnectorBuilder::new()
                    .with_webpki_roots()
                    .https_or_http()
                    .enable_http1()
                    .build(),
            ),
        ))
    };

let conf = SdkConfig::builder()
    .http_connector(HttpConnector::ConnectorFn(Arc::new(connector_fn)))

    .sleep_impl(default_async_sleep().unwrap())
    .build();

let client = aws_sdk_dynamodb::Client::from_conf(
        aws_sdk_dynamodb::config::Builder::from(&conf)
            .timeout_config(
                TimeoutConfig::builder()
                    .connect_timeout(Duration::from_millis(connect_timeout_value))
                    .build(),
            )
            .build(),
    );

Now, the 100ms timeout will work. Let me know if you have any questions about this. I'll try to think of a more friendly API.

@Oliboy50
Copy link
Author

Oliboy50 commented Feb 14, 2023

thanks for the explanation 👍

actually, as a simple user, I would not even think of building my own ConnectorFn as you shown here... nor using any other http_connector than the one set by default (because I trust you to pick the best one by default)...

simply using aws_config::load_from_env().await then set different timeout configs at each service level is the way to go IMO

so I'll wait for the issue linked by @rcoh to be fixed 👌

BTW, because of how accurate it is, I really love the operation_* timeouts much more than connect/read timeouts, so I'd love to see this issue fixed too ❤️

BTW2, would you want me to close this PR now or keep it open until the issue is fixed?

@Velfi
Copy link
Contributor

Velfi commented Feb 14, 2023

would you want me to close this PR now or keep it open until the issue is fixed?

I'll close this PR because we have @rcoh's issue. Thank you for bringing more attention to this issue.

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