feat(lambda-http): accept http_body::Body in responses #466

nmoutschen · 2022-04-14T13:19:24Z

Issue #, if available:

Description of changes:

Allows using any http_body::Body implementation in the response for lambda_http.

⚠️ At the moment, this PR removes the ability to return a Response<String> or any into value that implements Into<aws_lambda_events::encodings::Body>.

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.

calavera · 2022-04-19T15:19:22Z

lambda-http/src/response.rs

        let (parts, body) = self.into_parts();
-        Response::from_parts(parts, body.into())
+
+        let fut = async { Response::from_parts(parts, body.into_body().await) };


why does this have to be a future?

Here, this is due to the .await on into_body(), which is needed to support http_body::Body implementations that stream the body back, as we need to collect the entire payload before we can return a Lambda response.

Is it possible to collect partial payloads? Thinking HTTP Ranges or terminate early if a piece of data has been seen in the response as it streams instead of waiting for the whole payload? Not sure if this use case should be available through this API or just limited to a custom hack, but it would serve my interests.

@brainstorm Not immediately for this purpose, since here we just use this to transform it into a aws_lambda_events::encoding::Body enum, which we needs to generate the right response for API Gateway/ALB.

That said, if you're calling an API that returns an http_body::Body implementation, or some form of asynchronous stream, you could always implement that yourself in your function handler.

nmoutschen · 2022-05-05T12:33:55Z

Marking this one as ready to review as there's nothing outstanding here and would help on smithy-lang/smithy-rs#1338

calavera

LGTM

calavera · 2022-05-05T17:02:07Z

@nmoutschen will this fix the problem described in #438 ?

david-perez · 2022-05-06T09:14:40Z

lambda-http/src/lib.rs

+    fut_req: Pin<Box<dyn Future<Output = Result<R, E>> + 'a>>,
+    fut_res: Option<ResponseFuture>,


nit: since only one of these will be polled at a time, it might be better to model it as an enum with two variants.

I need some help here @calavera, @eduardomourar or @david-perez

The PR as it is now simply doesn't work. The lambdas using lambda_http get called (I can see my info logs with the event) but never return the response and time out.

I believe this poll function is at the core of the issue: https://github.com/awslabs/aws-lambda-rust-runtime/pull/466/files#diff-c62692e6b817476306b4d6966f541eec77a1768674e4dad5cf7f10c4faababa4R106-R119

I observed that the value of self.fut_res gets set then nothing else happens, the poll function never gets executed again with the value of self.fut_res set, it actually looks like it gets called once then that's it.

Ok bear with me I think I know why this isn't working as-is: https://tokio.rs/tokio/tutorial/async#wakers

When a future returns Poll::Pending, it must ensure that the waker is signaled at some point. Forgetting to do this results in the task hanging indefinitely.

Forgetting to wake a task after returning Poll::Pending is a common source of bugs.

Looks like by returning Poll::Pending from Poll::Ready and doing nothing else we are breaking the contract we also need to take an additional step

Before returning Poll::Pending, we called cx.waker().wake_by_ref(). This is to satisfy the future contract. By returning Poll::Pending, we are responsible for signaling the waker.

The code should be:

Poll::Ready(Ok(resp)) => { self.fut_res = Some(resp.into_response()); cx.waker().wake_by_ref() Poll::Pending }

And I've confirmed now that this works correctly. Continuing on with testing.

@hugobast Good catch! This is indeed a sharp edge of async Rust.

Typically only leaf futures are the ones that interact with wakers. Most futures uphold the poll contract by returning Poll::Pending only if some other future also returned Poll::Pending; in this way, it trivially fulfills the contract of poll since the inner future must follow that same contract.

In this particular case, we can do the same by polling the future that we just put in self.fut_res. That is, do here what is done in the first arm of the if statement too.

I think it's clearer if we model this as an enum, since we're only polling one of the futures at a time, and simply do self.poll() when the request future completes and we now want to poll the response future.

(Note though that your code is perfectly fine in terms of correctness)

@david-perez something like this? hugobast@d58a1bc

that looks good to me

david-perez · 2022-05-06T09:19:11Z

lambda-http/src/response.rs

+    fn into_response(self) -> ResponseFuture {
+        Box::pin(async move {
+            Response::builder()
+                .status(self.0.try_into().expect("unable to transform status code"))


This will panic if the conversion fails. I'm not sure if this convenience blanket impl is worth panicking. At the very least it should be documented.

david-perez · 2022-05-06T09:31:22Z

lambda-http/src/response.rs

+            // Can safely unwrap here as we do type validation in the 'if' statement
+            Box::pin(ready(*any_self.downcast::<Body>().unwrap()))
+        } else {
+            Box::pin(async move { Body::from(to_bytes(self).await.expect("unable to read bytes from body").to_vec()) })


This will always use the impl From<Vec<u8>> for Body, which always creates the Body::Binary variant. Is this correct? The body might just contain UTF-8-encoded text.

See https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-payload-encodings-workflow.html, which makes my head hurt.

ALB integration use content-encoding and content-type headers to determine if the body should be base64 encoded. See it here: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/lambda-functions.html#receive-event-from-load-balancer

Perhaps we could add utility methods to convert hyper::Response or reqwest::Reponse to http::Response<lambda_http::Body>. These methods can convert the Body to correct lambda_http::Body variant based on the same rules used by ALB.

This means we should probably work on Response not Body.

nmoutschen · 2022-05-09T11:53:10Z

@calavera Yep, I've also included a new example to fix that.

Fair points @david-perez ! I'm a bit busy with the Stockholm summit this week, but will update this PR either by the end of this week or the next with those changes.

Regarding panicking, this is to mimic the current behaviour, but while I'm making those changes, I might as well return a Result instead.

For the Body::Binary variant, yep, I was thinking of trying to transform into a String first, and only use Vec as a default. In the case of Lambda, it will always return a string (either proper string, or base64 encoded payload). Using Body::Binary basically does base64 encoding and set isBase64Encoded to true (see https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html#api-gateway-simple-proxy-for-lambda-output-format). My assumption was that Body::Binary would be handled correctly, but I might have to do a few more tests there to validate this just to ensure we don't break anything.

david-perez · 2022-05-09T17:03:03Z

@nmoutschen Are you sure that always constructing the Body::Binary variant is correct? If I understand this table correctly, it effectively means that we're always in the last 6 rows of the second table.

My assumption was that Body::Binary would be handled correctly

My understanding is that if the body contains regular UTF-8 encoded text, and we construct the Body::Binary variant for it, we will base64-encode it before handing it off to the Lambda service. If the Lambda is behind an API Gateway and the client has e.g. application/json in its Accept header, then API Gateway will base64-decode it before responding. So things work nicely in this case. This case corresponds to row 10 of the second table. What about the other cases?

eduardomourar · 2022-06-11T10:41:16Z

Could this be merged and released, please?

calavera · 2022-06-11T19:17:53Z

Could this be merged and released, please?

@eduardomourar unfortunately this has not been tested. @david-perez's comment still stands:

My understanding is that if the body contains regular UTF-8 encoded text, and we construct the Body::Binary variant for it, we will base64-encode it before handing it off to the Lambda service. If the Lambda is behind an API Gateway and the client has e.g. application/json in its Accept header, then API Gateway will base64-decode it before responding. So things work nicely in this case. This case corresponds to row 10 of the second table. What about the other cases?

Moreover, Nicolas has left AWS and he's no longer interested in completing this PR, which is understandable.

If you'd like to see these changes in a release, you're very welcome to help us verify that the changes work for all API GW responses. For now, we don't have much time to verify it ourselves.

hugobast · 2022-06-15T18:20:38Z

Could this be merged and released, please?

@eduardomourar unfortunately this has not been tested. @david-perez's comment still stands:

My understanding is that if the body contains regular UTF-8 encoded text, and we construct the Body::Binary variant for it, we will base64-encode it before handing it off to the Lambda service. If the Lambda is behind an API Gateway and the client has e.g. application/json in its Accept header, then API Gateway will base64-decode it before responding. So things work nicely in this case. This case corresponds to row 10 of the second table. What about the other cases?

Moreover, Nicolas has left AWS and he's no longer interested in completing this PR, which is understandable.

If you'd like to see these changes in a release, you're very welcome to help us verify that the changes work for all API GW responses. For now, we don't have much time to verify it ourselves.

I'm more than happy to carry this forward, I'm having difficulties running the integration tests (probably due to my M1 Mac), I digress. What I am curious to know @calavera is what kind of test you are looking for, I thought I would add something to the existing integration test and run it as many times as there are API GW permutations to validate that we cover all responses from https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-payload-encodings-workflow.html. It does seem toilsome to do though, am I on the right track? is there something easier I can do to test?

calavera · 2022-06-15T18:51:14Z

@hugobast thanks a lot for looking into this! I don't really have any better ideas to be honest.

hugobast · 2022-06-21T21:44:08Z

@calavera here's the PR, happy to discuss #491 please also check this commit that I have decided to drop since the API Gateway configuration to perform the tests per the table lead me into a dead-end (I could be wrong and I'm happy to pick it back up) hugobast@83fa77d

calavera · 2022-06-21T23:42:03Z

Thanks @hugobast ! I'm going to close this PR in favor of yours, we can continue the conversation there.

feat!(lambda-http): accept http_body::Body in responses

d101df5

nmoutschen mentioned this pull request Apr 15, 2022

Add string implementations for http_body::Body? hyperium/http-body#57

Closed

fix(lambda-http): fix examples and docs

d17ac31

nmoutschen changed the title ~~feat!(lambda-http): accept http_body::Body in responses~~ feat(lambda-http): accept http_body::Body in responses Apr 19, 2022

doc(lambda-http): add examples

eac9281

nmoutschen mentioned this pull request Apr 19, 2022

Can not use TraceLayer from tower_http crate #438

Closed

calavera reviewed Apr 19, 2022

View reviewed changes

brainstorm mentioned this pull request Apr 22, 2022

Implemented APIGateway to Rust Lambda through lambda_http brainstorm/s3-rust-noodles-bam#4

Merged

eduardomourar mentioned this pull request May 5, 2022

feat(http-server): support service on aws lambda smithy-lang/smithy-rs#1338

Closed

2 tasks

nmoutschen marked this pull request as ready for review May 5, 2022 12:33

calavera approved these changes May 5, 2022

View reviewed changes

david-perez reviewed May 6, 2022

View reviewed changes

hugobast mentioned this pull request Jun 21, 2022

feat(lambda-http): accept http_body::Body in responses #491

Merged

2 tasks

calavera closed this Jun 21, 2022

calavera deleted the response-body branch June 21, 2022 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda-http): accept http_body::Body in responses #466

feat(lambda-http): accept http_body::Body in responses #466

nmoutschen commented Apr 14, 2022 •

edited

Loading

calavera Apr 19, 2022

nmoutschen Apr 19, 2022

brainstorm Apr 20, 2022 •

edited

Loading

nmoutschen Apr 20, 2022

nmoutschen commented May 5, 2022

calavera left a comment

calavera commented May 5, 2022

david-perez May 6, 2022

hugobast Jun 17, 2022

hugobast Jun 17, 2022

david-perez Jun 17, 2022 •

edited

Loading

hugobast Jun 20, 2022 •

edited

Loading

calavera Jun 21, 2022

david-perez May 6, 2022

david-perez May 6, 2022

bnusunny Jun 17, 2022 •

edited

Loading

nmoutschen commented May 9, 2022

david-perez commented May 9, 2022

eduardomourar commented Jun 11, 2022

calavera commented Jun 11, 2022

hugobast commented Jun 15, 2022

calavera commented Jun 15, 2022

hugobast commented Jun 21, 2022

calavera commented Jun 21, 2022

		fut_req: Pin<Box<dyn Future<Output = Result<R, E>> + 'a>>,
		fut_res: Option<ResponseFuture>,

feat(lambda-http): accept http_body::Body in responses #466

feat(lambda-http): accept http_body::Body in responses #466

Conversation

nmoutschen commented Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainstorm Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmoutschen commented May 5, 2022

calavera left a comment

Choose a reason for hiding this comment

calavera commented May 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

hugobast Jun 20, 2022 • edited Loading

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

bnusunny Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

nmoutschen commented May 9, 2022

david-perez commented May 9, 2022

eduardomourar commented Jun 11, 2022

calavera commented Jun 11, 2022

hugobast commented Jun 15, 2022

calavera commented Jun 15, 2022

hugobast commented Jun 21, 2022

calavera commented Jun 21, 2022

nmoutschen commented Apr 14, 2022 •

edited

Loading

brainstorm Apr 20, 2022 •

edited

Loading

david-perez Jun 17, 2022 •

edited

Loading

hugobast Jun 20, 2022 •

edited

Loading

bnusunny Jun 17, 2022 •

edited

Loading