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

Remove futures_core::stream::Stream from aws_smithy_http::byte_stream::ByteStream #2983

Merged
merged 31 commits into from
Sep 28, 2023

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Sep 12, 2023

Motivation and Context

Removes futures_core::stream::Stream from aws_smithy_http::byte_stream::ByteStream

Description

This PR is part of our ongoing effort, #2413. We remove the futures_core::stream::Stream trait from aws_smithy_http::byte_stream::ByteStream. To continue support existing places that relied upon ByteStream implementing the Stream trait, we provide explicit .next, .try_next, and .size_hints methods on that type. As part of it, a doc-hidden type FuturesStreamCompatByteStream, which implements Stream, has been added to aws_smithy_http to cater to those who need a type implementing Stream (see discussion why doc-hidden).

Another place we need to update is codegen responsible for rendering stream payload serializer, and this affects the server. The regular server and the python server have slightly different rendering requirements, since the former uses aws_smithy_http::byte_stream::ByteStream (that does not implement the Stream trait) and latter uses its own ByteStream (that does implement Stream). We use ServerHttpBoundProtocolCustomization to handle the difference: StreamPayloadSerializerCustomization and PythonServerStreamPayloadSerializerCustomization.

Testing

No new behavior has been added, relied on the existing tests in CI.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

seems pretty reasonable. I might consider putting the impl Stream bridge in the types-convert crate instead so customers can easily use it if they want

@ysaito1001 ysaito1001 marked this pull request as ready for review September 20, 2023 20:43
@ysaito1001 ysaito1001 requested review from a team as code owners September 20, 2023 20:43
@ysaito1001 ysaito1001 requested review from david-perez and removed request for a team September 20, 2023 20:43
@ysaito1001
Copy link
Contributor Author

seems pretty reasonable. I might consider putting the impl Stream bridge in the types-convert crate instead so customers can easily use it if they want

The impl Stream bridge is currently done for FuturesStreamCompatByteStream and it's meant to be used directly by codegen to pass ByteStream (in the case of python-server, it's own aws_smithy_http_server_python::types::ByteStream) to hyper::body::Body::wrap_stream for stream payload serializers.

For external customers' use cases for aws_smithy_http::ByteStream, we may not need the impl Stream bridge, i.e. next, try_next, collect, and into_async_read should do?

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

Awesome work on this!! I love how narrow this PR ended up being

Base automatically changed from ysaito/remove-futures-stream-from-smithy-async to main September 27, 2023 21:39
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

The method in question is used by the python server's custom `ByteStream`
so it needs to be `pub`.
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001 ysaito1001 added this pull request to the merge queue Sep 28, 2023
Merged via the queue into main with commit 33cd698 Sep 28, 2023
40 of 41 checks passed
@ysaito1001 ysaito1001 deleted the ysaito/remove-futures-stream-from-byte-stream branch September 28, 2023 20:17
@@ -393,7 +423,9 @@ impl ByteStream {
/// # }
/// ```
pub fn into_async_read(self) -> impl tokio::io::AsyncRead {
Copy link
Contributor

Choose a reason for hiding this comment

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

Earlier we could do this wrapping ourselves & since tokio_util::io::StreamReader implements AsyncBufRead, we would get a buffered reader for free. Would it be acceptable to change this to:

Suggested change
pub fn into_async_read(self) -> impl tokio::io::AsyncRead {
pub fn into_async_read(self) -> impl tokio::io::AsyncBufRead {

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps you would want a separate into_async_buf_read?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have any buffering in our implementation. You should be able to wrap the ByteStream with a new-type and implement Stream for it to get the same functionality you used to have. The Stream impl should be a simple call through to the poll_next and size_hint methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know. What I'm saying is that in this function, you're wrapping bytestream in StreamReader from tokio, which implements AsyncBufRead. So you could also return impl AsyncBufRead from this function.
Otherwise, one has to wrap this in a BufReader for no reason, since it is not possible to wrap ByteStream in StreamReader easily (without wrapping & implementing Stream manually).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see what you're saying. Yeah, I don't see why we couldn't have two methods. You're welcome to submit a PR to add that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it makes more sense to change the signature of the existing function as I suggested above. It won't be a breaking change at all since AsyncBufRead is a super trait of AsyncRead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good.

github-merge-queue bot pushed a commit that referenced this pull request Nov 10, 2023
## Motivation and Context
The tokio `StreamReader` implements `AsyncBufRead`, but we're returning
`AsyncRead` currently. If a user needs an `AsyncBufRead`, then they've
to wrap the returned value in tokio `BufReader` for no reason. Since
`ByteStream` doesn't implement `Stream` anymore, one has to either wrap
the returned value unnecessarily or write a wrapper similar to this
function.
See
#2983 (comment).

## Description
Simply changed the return type to say `impl AsyncBufRead`. Since
`AsyncBufRead` is a super-set of `AsyncRead`, this is not a breaking
change.

## Testing
The code example tests it.

## Checklist
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

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

---------

Co-authored-by: Russell Cohen <[email protected]>
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.

6 participants