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

Add lifetimes to async traits that take args by reference #3061

Merged
merged 6 commits into from
Oct 13, 2023

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Oct 11, 2023

This PR adds lifetimes to the IdentityResolver, DnsResolver (renamed to ResolveDns), and EndpointResolver traits so that lifetime gymnastics aren't needed when implementing those traits. For example, IdentityResolver::resolve_identity takes &ConfigBag as an argument, which means you have to pull things out of the ConfigBag outside of any returned async block in order for the compiler to be satisfied. This change removes that consideration and makes implementing these traits a lot easier.


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

@jdisanti jdisanti requested review from a team as code owners October 11, 2023 21:58
@jdisanti jdisanti added needs-sdk-review breaking-change This will require a breaking change labels Oct 11, 2023
@github-actions
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.

Comment on lines 755 to 757
fn resolve_dns<'a>(&'a self, name: &'a str) -> DnsFuture<'a>
where
Self: 'a,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we say &'a self, then why do we also need the where bound? What's the difference between those two bounds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I initially added it because the async-trait crate adds it, but I can't think of a good reason to have it for our use-case, so I've removed it in 591e76f.

@github-actions
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.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

At first glance, it seems that a trait method accepting multiple references, with each having a different lifetime, allows for more flexibility for callers, but this PR is saying that's not necessarily the case? Specifically, in cases where these trait methods are used, it's more ergonomic for users if all input references have the same lifetime?

For example, IdentityResolver::resolve_identity takes &ConfigBag as an argument, which means you have to pull things out of the ConfigBag outside of any returned async block in order for the compiler to be satisfied.

Is it possible to elaborate on this with pseudo-code?

@jdisanti
Copy link
Collaborator Author

Having separate lifetimes per argument definitely gives more flexibility. I think there's a balance to be struck between flexibility and usability: going too flexible results in a very lengthy and complex function declaration, while going too simple makes it hard to implement the function body (and potentially callers of it).

I could do something like this to maximize flexibility:

fn thing<'a, 'b, 'c>(&'a self, &'b SomeArgument) -> SomeFuture<'c>
where
    'a: 'c,
    'b: 'c,
    Self: 'c

But then customers have to have all that noise in their implementations, and for not much gain. Since 'a and 'b both live as long as 'c, it's roughly equivalent to just using 'a, but may grant some extra flexibility to the caller. Since we are the caller (in the orchestrator), I don't think that matters that much.

To give an example of what triggered me doing this in the first place, I had this in the LazyCache:

impl ResolveCachedIdentity for LazyCache {
    fn resolve_cached_identity(
        &self,
        resolver: SharedIdentityResolver,
        config_bag: &ConfigBag,
    ) -> IdentityFuture {
        // ...

        IdentityFuture::new(async move {
            // ...
        })
    }
}

With the above, if I try to use config_bag inside of the async block, I'll get an error saying it doesn't live long enough. Additionally, if I try to use self in the async block, it has the same error because &self won't live as long as the future.

By adding the single lifetime, I can now reference both self and config_bag inside the async block without any issue.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

Thanks for going into details. Makes sense to me.

impl DnsResolver for TestDns {
fn resolve_dns(&self, name: String) -> DnsFuture {
DnsFuture::ready(Ok(self.addrs.get(&name).unwrap_or(&self.fallback).clone()))
impl ResolveDns for TestDns {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for verb + noun convention.

@github-actions
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 October 13, 2023 16:34
@github-actions
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 Oct 13, 2023
Merged via the queue into main with commit d293d1f Oct 13, 2023
40 of 41 checks passed
@jdisanti jdisanti deleted the jdisanti-async-trait-lifetimes branch October 13, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants