Implement timeouts for LazyCachingCredentialsProvider #595

jdisanti · 2021-07-20T22:50:29Z

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

rcoh

overall implementation looks good. I think we need to work a bit more about how the cargo features are going to work out.

rcoh · 2021-07-21T17:28:40Z

aws/rust-runtime/aws-auth/Cargo.toml

@@ -19,3 +24,4 @@ http = "0.2.3"
 test-env-log = { version = "0.2.7", features = ["trace"] }
 tokio = { version = "1", features = ["macros", "rt", "rt-multi-thread", "test-util"] }
 tracing-subscriber = { version = "0.2.16", features = ["fmt"] }
+smithy-http = { path = "../../../rust-runtime/smithy-http", features = ["sleep-tokio"] }


you actually don't want to mention the feature here—mentioning it here makes it a hard dependency

when timeout-tokio is enabled, it will get pulled in

This should be fine since it's under [dev-dependencies].

rcoh · 2021-07-21T17:33:11Z

rust-runtime/smithy-http/src/sleep.rs

+/// Implementation of [`AsyncSleep`] for Tokio.
+#[cfg(feature = "sleep-tokio")]
+#[derive(Default)]
+pub struct TokioSleep;


#[non_exhaustive] to prevent people from constructing this directly

Good call! Will fix.

rcoh · 2021-07-21T17:35:56Z

aws/rust-runtime/aws-auth/src/provider/timeout.rs

+use std::pin::Pin;
+use std::task::{Context, Poll};
+
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]


#[non_exhaustive]

rcoh · 2021-07-21T17:45:48Z

aws/rust-runtime/aws-auth/src/provider/lazy_caching.rs

@@ -186,8 +202,9 @@ pub mod builder {
            );
            LazyCachingCredentialsProvider::new(
                SystemTimeSource,
-                self.refresh.expect("refresh provider is required"),
-                self.refresh_timeout.unwrap_or(DEFAULT_REFRESH_TIMEOUT),
+                self.sleep.expect("sleep_impl is required"),


if the tokio feature is enabled, we could consider defaulting to tokio-sleep here

rcoh · 2021-07-21T17:58:58Z

rust-runtime/smithy-http/Cargo.toml

@@ -7,6 +7,7 @@ license = "Apache-2.0"

 [features]
 bytestream-util = ["tokio/fs", "tokio-util/io"]
+sleep-tokio = ["tokio/time"]


I think we probably don't want this in smithy-http. Also, since this doesn't actually bring in the runtime, this isn't sufficient to actually work.

carllerche · 2021-07-21T20:58:29Z

aws/rust-runtime/aws-auth/src/provider.rs

+use std::time::Duration;
+
+// Re-export AsyncSleep and TokioSleep so that an explicit dependency on smithy-http isn't needed
+pub use smithy_http::sleep::AsyncSleep;


Worth noting, this adds a public API dependency on smithy-http, so breaking change releases will need to be coordinated.

carllerche · 2021-07-21T21:02:47Z

rust-runtime/smithy-http/src/sleep.rs

+/// Async trait with a `sleep` function.
+pub trait AsyncSleep: Send + Sync {
+    /// Returns a future that sleeps for the given `duration` of time.
+    fn sleep(&self, duration: Duration) -> Pin<Box<dyn Future<Output = ()> + Send + '_>>;


For future compatibility, you may want to consider defining struct Sleep(Pin<Box<...>>) and Sleep::new(impl Future + Send) (or whatever). That will give you some wiggle room in the future if you need to change the return type.

carllerche · 2021-07-21T21:07:58Z

aws/rust-runtime/aws-auth/src/provider/lazy_caching.rs

 pub struct LazyCachingCredentialsProvider {
    time: Box<dyn TimeSource>,
+    sleeper: Box<dyn AsyncSleep>,


Not a hard/fast rule, but based on my observations, usually w/ Rust, *er tends to be avoided as a suffix.

carllerche · 2021-07-21T21:19:59Z

aws/sdk/examples/sts/src/bin/credentials-provider.rs

+        // it can be used with different async runtimes. The sleep function will be used for timeouts.
+        // Since we're using Tokio in this example, we use the `TokioSleep` implementation, which
+        // requires the `timeout-tokio` feature in the `aws-auth` dependency.
+        .sleeper(TokioSleep::new())


I agree w/ @rcoh that, in the default case, this should not be specified by the user.

…timeouts

rcoh

LGTM, test failure is a broken doc link

rcoh · 2021-07-29T20:51:46Z

aws/rust-runtime/aws-auth/src/provider/lazy_caching.rs

@@ -202,8 +199,10 @@ pub mod builder {
            );
            LazyCachingCredentialsProvider::new(
                SystemTimeSource,
-                self.sleep.expect("sleep_impl is required"),
-                self.loader.expect("loader is required"),
+                self.sleep.unwrap_or_else(|| {


probably better to make this a fallible builder rather than hard crashing

Doesn't this fall under the category of programmer error? Or are you thinking there will be use-cases where sleep is set programmatically instead of hardcoded?

rcoh · 2021-07-29T20:53:12Z

aws/sdk/examples/sts/src/bin/credentials-provider.rs

-        // requires the `timeout-tokio` feature in the `aws-auth` dependency.
-        .sleeper(TokioSleep::new())
-        .loader(async_provide_credentials_fn(move || {
+        .load(async_provide_credentials_fn(move || {
            let client = client.clone();


maybe make a separate async fn for this so it's clear you don't have to embed the entire body in the async fn

If taking that approach, it's probably better to implement AsyncProvideCredentials on a struct instead, because you need a way to bind the STS Client to the function.

rcoh · 2021-07-29T20:55:01Z

rust-runtime/smithy-async/src/rt/sleep.rs

+use std::time::Duration;
+
+/// Async trait with a `sleep` function.
+pub trait AsyncSleep: std::fmt::Debug + Send + Sync {


hesitant about this Debug bound. I would probably drop it, I think

cc @carllerche for your take. I guess the alternative is manually implementing debug for things that contain this type, or leaving it as a generic parameter and deriving debug.

My reasoning here was that someone would be able to tell which sleep implementation they were using by debug printing it, which would result in TokioSleep if it were the Tokio implementation provided in this same file. I suspect implementations of this won't have any data associated with them.

I would be consistent w/ all other traits. It isn't unreasonable to require Debug when dealing mostly w/ trait objects. Most types implement Debug, so it shouldn't be much of a burden. However, if no other trait extends Debug, I probably wouldn't do it here either.

This is the first and only trait in the crate so far, so it will be consistent. This crate is intended to be used to provide async runtime agnostic abstractions, so having Debug on the traits to be able to tell which concrete implementation is being used is probably valuable, especially since these traits will be used as trait objects.

…timeouts

jdisanti added 4 commits July 20, 2021 15:47

Implement timeouts for LazyCachingCredentialsProvider

ee8875d

Rename refresh to reload

c43456a

Update CHANGELOG

49ff14e

Fix clippy

4c6d36d

jdisanti requested a review from rcoh July 20, 2021 23:45

rcoh reviewed Jul 21, 2021

View reviewed changes

carllerche reviewed Jul 21, 2021

View reviewed changes

jdisanti added 2 commits July 28, 2021 09:51

CR feedback

f2ed331

Merge remote-tracking branch 'upstream/main' into lazy-caching-creds-…

f27123e

…timeouts

jdisanti requested a review from rcoh July 29, 2021 20:19

rcoh approved these changes Jul 29, 2021

View reviewed changes

jdisanti added 2 commits August 2, 2021 10:51

Add note about panic on LazyCachedCredentialsProvider builder

e4a27e0

Merge remote-tracking branch 'upstream/main' into lazy-caching-creds-…

327eca5

…timeouts

jdisanti requested a review from rcoh August 2, 2021 17:52

rcoh approved these changes Aug 2, 2021

View reviewed changes

Fix doc comment code reference

85ee3cb

jdisanti merged commit 70999da into smithy-lang:main Aug 2, 2021

jdisanti deleted the lazy-caching-creds-timeouts branch August 2, 2021 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement timeouts for LazyCachingCredentialsProvider #595

Implement timeouts for LazyCachingCredentialsProvider #595

jdisanti commented Jul 20, 2021

rcoh left a comment

rcoh Jul 21, 2021

rcoh Jul 21, 2021

jdisanti Jul 22, 2021

rcoh Jul 21, 2021

jdisanti Jul 22, 2021

rcoh Jul 21, 2021

rcoh Jul 21, 2021

rcoh Jul 21, 2021

carllerche Jul 21, 2021

carllerche Jul 21, 2021

carllerche Jul 21, 2021

carllerche Jul 21, 2021

rcoh left a comment

rcoh Jul 29, 2021

jdisanti Jul 30, 2021

rcoh Jul 29, 2021

jdisanti Aug 2, 2021

rcoh Jul 29, 2021

rcoh Jul 29, 2021

jdisanti Jul 30, 2021

carllerche Aug 2, 2021

jdisanti Aug 2, 2021

Implement timeouts for LazyCachingCredentialsProvider #595

Implement timeouts for LazyCachingCredentialsProvider #595

Conversation

jdisanti commented Jul 20, 2021

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment