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

Check that lambda_https inner service is ready before calling it #538

Merged
merged 3 commits into from
Oct 7, 2022
Merged

Check that lambda_https inner service is ready before calling it #538

merged 3 commits into from
Oct 7, 2022

Conversation

dcormier
Copy link
Contributor

@dcormier dcormier commented Oct 6, 2022

This allows for service backpressure.

Additionally:

Services are permitted to panic if call is invoked without obtaining Poll::Ready(Ok(())) from poll_ready.

Source

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

@dcormier
Copy link
Contributor Author

dcormier commented Oct 6, 2022

Alternatively, I could tweak the existing http-trait test to include checking that .poll_ready() is properly called.

@dcormier
Copy link
Contributor Author

dcormier commented Oct 6, 2022

lambda_runtime already does this properly, but should a test be added/updated to ensure it continues to do so?

@dcormier
Copy link
Contributor Author

dcormier commented Oct 6, 2022

lambda_extension does not do this properly.

Copy link
Contributor

@calavera calavera 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 opening this PR! Do you mind fixing lambda-extension too? I can do it myself later too.

@dcormier
Copy link
Contributor Author

dcormier commented Oct 6, 2022

I can take a swing at it. Want it in this PR or another one?

@dcormier
Copy link
Contributor Author

dcormier commented Oct 6, 2022

I added it here. I updated the existing integration test to account for this.

I feel like that's a nicer approach. Thoughts on me removing the http-ready integration test I added and making it a part of the preexisting http-trait integration test?

Also: Thoughts on me updating the existing runtime-trait integration tests to ensure lambda_runtime continues to be well behaved?

@calavera
Copy link
Contributor

calavera commented Oct 6, 2022

Thoughts on me removing the http-ready integration test I added and making it a part of the preexisting http-trait integration test?

That sounds like a good idea. We need to look into how we run those directly in PRs, but that's a problem for a different day XD

Thoughts on me updating the existing runtime-trait integration tests to ensure lambda_runtime continues to be well behaved?

Sounds great too 👍

@dcormier
Copy link
Contributor Author

dcormier commented Oct 6, 2022

Alright. That's all done. Thank you for the feedback.

This allows for service [backpressure](https://docs.rs/tower/0.4.13/tower/trait.Service.html#backpressure).

Additionally:

> Services are permitted to panic if `call` is invoked without obtaining `Poll::Ready(Ok(()))` from `poll_ready`.

[Source](https://docs.rs/tower/0.4.13/tower/trait.Service.html#be-careful-when-cloning-inner-services)
This allows for service [backpressure](https://docs.rs/tower/0.4.13/tower/trait.Service.html#backpressure).

Additionally:

> Services are permitted to panic if `call` is invoked without obtaining `Poll::Ready(Ok(()))` from `poll_ready`.

[Source](https://docs.rs/tower/0.4.13/tower/trait.Service.html#be-careful-when-cloning-inner-services)
This allows for service [backpressure](https://docs.rs/tower/0.4.13/tower/trait.Service.html#backpressure).

Additionally:

> Services are permitted to panic if `call` is invoked without obtaining `Poll::Ready(Ok(()))` from `poll_ready`.

[Source](https://docs.rs/tower/0.4.13/tower/trait.Service.html#be-careful-when-cloning-inner-services)
@calavera calavera merged commit fc51300 into awslabs:main Oct 7, 2022
@dcormier dcormier deleted the dc/ready branch October 7, 2022 12:37
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.

2 participants