Server streaming body #1023

guymguym · 2022-01-02T15:43:25Z

Signed-off-by: Guy Margalit [email protected]

Motivation and Context

Support streaming body in server requests and responses.

Description

Testing

Generated s3 server codegen and tested get-object/put-object with streams.

Checklist

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

CC @crisidev @david-perez

guymguym · 2022-01-02T16:04:16Z

Hmm, I wonder why hyper::Body::wrap_stream is not found when building codegen-server-test

error[E0599]: no function or associated item named `wrap_stream` found for struct `Body` in the current scope
    --> rest_json/rust-server-codegen/src/operation_ser.rs:1742:59
     |
1742 |         let payload = aws_smithy_http_server::body::Body::wrap_stream(output.blob);
     |                                                           ^^^^^^^^^^^ function or associated item not found in `Body`

error[E0599]: no function or associated item named `wrap_stream` found for struct `Body` in the current scope
    --> rest_json/rust-server-codegen/src/operation_ser.rs:1771:59
     |
1771 |         let payload = aws_smithy_http_server::body::Body::wrap_stream(output.blob);
     |                                                           ^^^^^^^^^^^ function or associated item not found in `Body`

error[E0599]: no function or associated item named `wrap_stream` found for struct `Body` in the current scope
    --> rest_json/rust-server-codegen/src/operation_ser.rs:1800:59
     |
1800 |         let payload = aws_smithy_http_server::body::Body::wrap_stream(output.blob);
     |                                                           ^^^^^^^^^^^ function or associated item not found in `Body`

crisidev · 2022-01-03T12:17:35Z

Hmm, I wonder why hyper::Body::wrap_stream is not found when building codegen-server-test

error[E0599]: no function or associated item named `wrap_stream` found for struct `Body` in the current scope
    --> rest_json/rust-server-codegen/src/operation_ser.rs:1742:59
     |
1742 |         let payload = aws_smithy_http_server::body::Body::wrap_stream(output.blob);
     |                                                           ^^^^^^^^^^^ function or associated item not found in `Body`

error[E0599]: no function or associated item named `wrap_stream` found for struct `Body` in the current scope
    --> rest_json/rust-server-codegen/src/operation_ser.rs:1771:59
     |
1771 |         let payload = aws_smithy_http_server::body::Body::wrap_stream(output.blob);
     |                                                           ^^^^^^^^^^^ function or associated item not found in `Body`

error[E0599]: no function or associated item named `wrap_stream` found for struct `Body` in the current scope
    --> rest_json/rust-server-codegen/src/operation_ser.rs:1800:59
     |
1800 |         let payload = aws_smithy_http_server::body::Body::wrap_stream(output.blob);
     |                                                           ^^^^^^^^^^^ function or associated item not found in `Body`

wrap_stream is available only with the stream feature. You will need to add the feature to aws-smithy-http-server Cargo.toml.

crisidev

This is amazing @guymguym !! I left a couple of comments, nothing major :)

...tware/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationHandlerGenerator.kt

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

...tware/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationHandlerGenerator.kt

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

rust-runtime/aws-smithy-http/src/byte_stream.rs

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

rust-runtime/aws-smithy-http-server/src/lib.rs

david-perez

I'd like to merge this PR this week.

I did another pass and only found one more nitpick.

I'm approving. Can you please merge and resolve conflicts?

david-perez · 2022-01-31T19:22:07Z

...tware/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationHandlerGenerator.kt

@@ -132,13 +134,18 @@ class ServerOperationHandlerGenerator(
        } else {
            symbolProvider.toSymbol(operation.outputShape(model)).fullName
        }
+        val streamingBodyTraitBounds = if (operation.inputShape(model).hasStreamingMember(model)) {
+            "\n B: Into<#{SmithyHttpServer}::ByteStream>,"


You can remove the \n and interpolate the variable in the next line.

@david-perez the reason I did this was that the output is usually not including this streaming member and then it generates an empty line, which is not causing any compilation issues, but is weird to watch...

We run "cargo fmt" at the end of the codegen, so the weird look should go away.. Is it not?

Nope, the empty line is still there. Perhaps because it's just an empty line then cargo fmt will not remove it.

is there a better way to generate an optional line?

Not that I know of. Maybe @rcoh knows.

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

crisidev · 2022-02-01T10:58:14Z

I'll give this another pass this morning as well.

crisidev

Looks good!

crisidev · 2022-02-01T12:24:16Z

...tware/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationHandlerGenerator.kt

@@ -132,13 +134,18 @@ class ServerOperationHandlerGenerator(
        } else {
            symbolProvider.toSymbol(operation.outputShape(model)).fullName
        }
+        val streamingBodyTraitBounds = if (operation.inputShape(model).hasStreamingMember(model)) {
+            "\n B: Into<#{SmithyHttpServer}::ByteStream>,"


We run "cargo fmt" at the end of the codegen, so the weird look should go away.. Is it not?

guymguym · 2022-02-01T13:23:21Z

.../amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt

@@ -525,14 +525,14 @@ class ServerProtocolTestGenerator(
            FailingTest(RestJson, "RestJsonSupportsNegativeInfinityFloatInputs", Action.Response),
            FailingTest(RestJson, "RestJsonStreamingTraitsWithBlob", Action.Request),
            FailingTest(RestJson, "RestJsonStreamingTraitsWithNoBlobBody", Action.Request),
-            FailingTest(RestJson, "RestJsonStreamingTraitsWithBlob", Action.Response),


@david-perez @crisidev I was not sure what to do here - should I remove these tests from the expect-fail list as these now seem to pass...?

Yep, if it passes it can be removed from the failing list.

@guymguym I manually verified the now-passing tests and they look good.

I also had a look at the remaining failing request streaming protocol tests and pushed a new commit to this branch, Add blob streaming support for server request protocol tests, to get rid of them.

The only remaining failing protocol test for streaming is RestJsonStreamingTraitsRequireLengthWithBlob, but that is due to us not setting Content-Length appropriately, so I've cut #1146 to track that.

Let me know if you agree with the fix or have any questions.

That's great @david-perez. Thank you for making it so easy!

guymguym · 2022-02-01T13:46:45Z

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

-            if (parsedValue != null) {
-                withBlock("input = input.${member.setterName()}(", ");") {
-                    parsedValue(this)
+        if (streamingMember == null) {


I wanted to highlight this condition I added - I had to check it so that with streaming there will be no bindings parsers generated. The weird thing is just the flow of this function - the conditions seems a bit detached from one another but I wasn't sure if there's a way to simplify:

if (parser != null) { ... } else if (streamingMember != null) { ... } if (streamingMember == null) { ... }

@guymguym Thanks for calling this out. We still need to generate HTTP bindings for blob streaming (e.g. HTTP header bindings). This also led me to discover there's a bug in Smithy that is now being fixed over at smithy-lang/smithy#1075

I have pushed a new commit to this branch, Move blob streaming handling to httpPayload binding generation, that refactors this part and generates the stream handling when we handle @httpPayload.

Let me know if you agree with the fix or have any questions.

Ah now I see. I think my problem was probably that the previous headers code that I used was not very accurate, but since your PR to implement server response headers that might have fixed this issue. I will test a bit later and report back if I still see any issues in the S3 server codegen, but it looks good to me. Thanks!

guymguym · 2022-02-01T13:57:39Z

@david-perez I'm done rebasing and updating. Couple questions for you in the comments above. Thanks

crisidev · 2022-02-02T10:14:39Z

Wonderful work @guymguym!! @rcoh, when you have some time, would you mind reviewing this from a client codegen perspective? We'd like to merge this. There is still some work to do to have a good streaming experience, but this PR is paving the way!

david-perez · 2022-02-03T15:53:03Z

One last thought: given that we decided on using aws_smithy_http::byte_stream::ByteStream, I don't see any value in aws_smithy_http_server re-exporting ByteStream. We should just use ByteStream directly and work on #1125 to hide SdkBody from its public API.

Signed-off-by: Guy Margalit <[email protected]>

guymguym · 2022-02-03T16:12:08Z

done @david-perez

guymguym · 2022-02-03T16:28:54Z

.../amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt

+            FailingTest(RestJson, "RestJsonSupportsNaNFloatInputs", Action.Response),
+            FailingTest(RestJson, "RestJsonSimpleScalarProperties", Action.Response),
+            FailingTest(RestJson, "RestJsonSupportsInfinityFloatInputs", Action.Response),
+            FailingTest(RestJson, "RestJsonSupportsNegativeInfinityFloatInputs", Action.Response),


@david-perez These seem to fail in CI - did I accidentally add these changes when rebasing from main?

Yes, earlier today I had pushed to this branch a commit merging from main 907c0f3 (which removes these now-passing tests), so that you wouldn't have to deal with conflicts.

But you didn't fetch it (?), and you force-pushed the latest commit.

In general once a PR is open I think it's best to not rebase and force-push, only merge from main. Because AFAIK GitHub's UI won't show the previous history, only the rewrriten one, and that might confuse other people looking at the PR. The downside is that merging makes the history non-linear, but we always squash before merging, so I think it's a small price to pay for having an entire's PR history.

Anyway, these tests can be removed from the failing list.

got it. thanks. pushed a commit to remove those.

I think it's good now

rcoh

Approved on the client side

david-perez · 2022-02-03T18:26:45Z

@guymguym Thank you!

guymguym requested review from a team as code owners January 2, 2022 15:43

crisidev suggested changes Jan 3, 2022

View reviewed changes

david-perez reviewed Jan 3, 2022

View reviewed changes

guymguym force-pushed the s3d-streaming branch from ac3eb3c to b87b7e9 Compare January 3, 2022 16:06

rcoh requested changes Jan 3, 2022

View reviewed changes

rust-runtime/aws-smithy-http/src/byte_stream.rs Show resolved Hide resolved

david-perez reviewed Jan 3, 2022

View reviewed changes

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt Outdated Show resolved Hide resolved

guymguym force-pushed the s3d-streaming branch from b87b7e9 to 58ba708 Compare January 3, 2022 16:35

david-perez mentioned this pull request Jan 14, 2022

Guidance about Accept header for servers smithy-lang/smithy#1050

Open

guymguym commented Jan 16, 2022

View reviewed changes

rust-runtime/aws-smithy-http-server/src/lib.rs Outdated Show resolved Hide resolved

guymguym mentioned this pull request Jan 16, 2022

rust-server-codegen: add support for @httpPayload in requests #1060

Merged

guymguym force-pushed the s3d-streaming branch 2 times, most recently from 80580c8 to d57232d Compare January 20, 2022 07:17

guymguym force-pushed the s3d-streaming branch from d57232d to 44d1379 Compare January 25, 2022 21:21

david-perez mentioned this pull request Jan 27, 2022

Make aws_smithy_http::byte_stream::ByteStream not expose SdkBody #1125

Open

guymguym force-pushed the s3d-streaming branch from 44d1379 to a4ff63f Compare January 27, 2022 19:27

guymguym changed the title ~~[Server] streaming body + response headers~~ Server streaming body Jan 27, 2022

david-perez approved these changes Jan 31, 2022

View reviewed changes

david-perez added the server Rust server SDK label Feb 1, 2022

guymguym force-pushed the s3d-streaming branch from a4ff63f to 538d063 Compare February 1, 2022 11:32

crisidev approved these changes Feb 1, 2022

View reviewed changes

guymguym force-pushed the s3d-streaming branch 2 times, most recently from 25e8ede to 9792c9c Compare February 1, 2022 13:22

guymguym commented Feb 1, 2022

View reviewed changes

guymguym force-pushed the s3d-streaming branch from 9792c9c to bc571b3 Compare February 1, 2022 13:38

guymguym commented Feb 1, 2022

View reviewed changes

guymguym force-pushed the s3d-streaming branch 2 times, most recently from 3722ea5 to d5e4174 Compare February 1, 2022 22:21

guymguym force-pushed the s3d-streaming branch 2 times, most recently from 0033f7e to 9b268db Compare February 2, 2022 18:43

guymguym and others added 4 commits February 3, 2022 18:10

Server streaming body

5ac9f1e

Signed-off-by: Guy Margalit <[email protected]>

Move blob streaming handling to httpPayload binding generation

5b6ef98

Add blob streaming support for server request protocol tests

f463cee

Avoid re-exporting ByteStream in server module

2ca7cc0

guymguym force-pushed the s3d-streaming branch from 3f6c996 to 2ca7cc0 Compare February 3, 2022 16:11

guymguym commented Feb 3, 2022

View reviewed changes

remove passing tests

bfa1dee

rcoh approved these changes Feb 3, 2022

View reviewed changes

david-perez merged commit f76bc15 into smithy-lang:main Feb 3, 2022

guymguym deleted the s3d-streaming branch February 3, 2022 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server streaming body #1023

Server streaming body #1023

guymguym commented Jan 2, 2022 •

edited

Loading

guymguym commented Jan 2, 2022

crisidev commented Jan 3, 2022

crisidev left a comment

david-perez left a comment

david-perez Jan 31, 2022

guymguym Feb 1, 2022

crisidev Feb 1, 2022

guymguym Feb 1, 2022

guymguym Feb 1, 2022

david-perez Feb 2, 2022

crisidev commented Feb 1, 2022

crisidev left a comment

crisidev Feb 1, 2022

guymguym Feb 1, 2022

crisidev Feb 1, 2022

david-perez Feb 2, 2022 •

edited

Loading

guymguym Feb 2, 2022

guymguym Feb 1, 2022

david-perez Feb 2, 2022 •

edited

Loading

guymguym Feb 2, 2022

guymguym commented Feb 1, 2022

crisidev commented Feb 2, 2022

david-perez commented Feb 3, 2022

guymguym commented Feb 3, 2022

guymguym Feb 3, 2022

david-perez Feb 3, 2022

guymguym Feb 3, 2022

guymguym Feb 3, 2022

rcoh left a comment

david-perez commented Feb 3, 2022

Server streaming body #1023

Server streaming body #1023

Conversation

guymguym commented Jan 2, 2022 • edited Loading

Motivation and Context

Description

Testing

Checklist

guymguym commented Jan 2, 2022

crisidev commented Jan 3, 2022

crisidev left a comment

Choose a reason for hiding this comment

david-perez 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

crisidev commented Feb 1, 2022

crisidev 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

david-perez Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guymguym commented Feb 1, 2022

crisidev commented Feb 2, 2022

david-perez commented Feb 3, 2022

guymguym commented Feb 3, 2022

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

david-perez commented Feb 3, 2022

guymguym commented Jan 2, 2022 •

edited

Loading

david-perez Feb 2, 2022 •

edited

Loading

david-perez Feb 2, 2022 •

edited

Loading