Remove `futures_core::stream::Stream` from public API #2910

ysaito1001 · 2023-08-09T01:12:58Z

Motivation and Context

Addresses boxes 2 and 5 in #2413

Description

This PR removes the futures_core::stream::Stream trait from public API, as enforced by our tool check-external-types. In our codebase, we previously relied on the Stream trait for the following use cases:

calling .next().await to retrieve items from a stream
calling .collect().await to dump elements into a collection
passing types to functions/methods accepting impl Stream

Removing the Stream trait from the use cases above affected the following types:

aws_smithy_async::fn_steram::{FnStream, TryFlatMap}
aws_smithy_http::byte_stream::ByteStream
aws_smithy_http::event_stream::sender::{EventStreamSender, MessageStreamAdapter}

The primary approach taken in this PR involves

defining async fn next(&mut self) -> XXX for types that used to implement the impl Stream
implementing a .collect stream extension method for FnStream since it was previously used in its example
using FnStream at its core when types needed to hold/pass/return Stream trait objects
using FnStream to replace usages of a stream! macro from async_stream
introducing new-types HyperBodyWrapEventStream and HyperBodyWrapByteStream for those that were previously passed to hyper::body::Body::wrap_stream, which requires impl Stream, but no longer implement the trait.

Testing

Ported existing tests (many of which used a stream! macro from the async-stream crate) to use FnStream.

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 · 2023-08-11T20:20:20Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2023-08-11T21:37:53Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

This commit appends `release-2023-08-03` to `NOTABLE_SDK_RELEASE_TAGS`. Elements in that `Vec` should be sorted in an ascending order for a function `enabled_feature` to work correctly. The change has been verified by the following (each executed from canary-runner directory) ``` cargo run -- build-bundle \ --sdk-release-tag release-2023-08-03 \ --canary-path ../canary-lambda \ --manifest-only --musl && \ cd ../canary-lambda && \ cargo check ``` ``` cargo run -- build-bundle \ --sdk-release-tag release-2023-05-24 \ --canary-path ../canary-lambda \ --manifest-only --musl && \ cd ../canary-lambda && \ cargo check ``` ``` cargo run -- build-bundle \ --sdk-release-tag release-2023-12-14 \ --canary-path ../canary-lambda \ --manifest-only --musl && \ cd ../canary-lambda && \ cargo check

github-actions · 2023-08-16T02:48:25Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2023-08-30T19:12:24Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

This commit addresses #2910 (comment)

github-actions · 2023-08-30T21:17:42Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

...ftware/amazon/smithy/rust/codegen/core/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

...lin/software/amazon/smithy/rust/codegen/core/smithy/customizations/SmithyTypesPubUseExtra.kt

This commit addresses #2910 (comment)

github-actions · 2023-08-31T04:44:29Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti

LGTM

unexge · 2023-09-05T09:29:12Z

...ithy/rust/codegen/server/python/smithy/generators/PythonServerEventStreamWrapperGenerator.kt

+                    let stream = #{FnStream}::new(|tx| {
+                        Box::pin(async move {
+                            while let #{Some}(item) = stream.next().await {
+                                tx.send(item).await.expect("send should succeed");


Is it safe to panic here?

david-perez · 2023-09-06T10:33:00Z

CHANGELOG.next.toml

+message = """
+The `futures_core::stream::Stream` trait has been removed from public API. The methods that were made available through the `Stream` trait have been removed from `FnStream`, `TryFlatMap`, ByteStream`, `EventStreamSender`, and `MessageStreamAdapter`. However, we have preserved `.next()` and `.collect()` to continue supporting existing call sites in `smithy-rs` and `aws-sdk-rust`, including tests and rustdocs. If we need to support missing stream operations, we are planning to do so in an additive, backward compatible manner.
+
+If your code uses a `stream!` macro from the `async_stream` crate to generate stream data, it needs to be replaced by `aws_smithy_async::future::fn_steram::FnStream`. See https://github.com/awslabs/smithy-rs/discussions/2952 for more details.


fn_steram -> fn_stream.

david-perez · 2023-09-06T10:33:21Z

CHANGELOG.next.toml

@@ -84,3 +84,21 @@ message = "Generate a region setter when a model uses SigV4."
 references = ["smithy-rs#2960"]
 meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
 author = "jdisanti"
+
+[[aws-sdk-rust]]


These entries support Markdown. We should link to the relevant Rust items e.g. https://docs.rs/aws-smithy-async/latest/aws_smithy_async/future/fn_stream/struct.FnStream.html

david-perez · 2023-09-06T10:36:15Z

aws/sdk/integration-tests/transcribestreaming/tests/test.rs

-            yield Ok(AudioStream::AudioEvent(AudioEvent::builder().audio_chunk(Blob::new(chunk)).build()));
-        }
-    };
+    let input_stream = FnStream::new(|tx| {


Out of curiosity. From https://docs.rs/aws-smithy-async/latest/aws_smithy_async/future/fn_stream/struct.FnStream.html:

Because the stream is 1-bounded, the function will not proceed until the stream is read.

Why is the channel 1-bounded? My understanding is that the user sends stream events to the channel and the client polls and dequeues them to send them through the network. What would be bad about allowing the channel to be unbounded?

FnStream is an implementation of generators–the network isn't involved in all use cases. By bounding it, you all the caller to avoid using more memory than is necessary

rust-runtime/inlineable/src/hyper_body_wrap_stream.rs

david-perez · 2023-09-06T11:08:35Z

rust-runtime/aws-smithy-async/src/future/fn_stream.rs

@@ -6,12 +6,14 @@
 //! Utility to drive a stream with an async function and a channel.

 use crate::future::rendezvous;
-use futures_util::StreamExt;


nit: I guess we could mention in the changelog that the type now doesn't implement this or TryStream, TryStreamExt either, for completion.

david-perez · 2023-09-06T11:15:38Z

...ware/amazon/smithy/rust/codegen/server/python/smithy/protocols/PythonServerProtocolLoader.kt

+            // `aws_smithy_http_server_python::types::ByteStream` already implements
+            // `futures::stream::Stream`, so no need to wrap it in a futures' stream-compatible


Why the asymmetry though? I'd prefer it if we made aws_smithy_http_server_python::types::ByteStream more similar to aws_smithy_http::byte_stream::ByteStream and have both not implement Stream, and wrap it there too in a HyperBodyWrapByteStream.

The generator code would also become simpler; we wouldn't need to draw distinctions based on what we're generating.

The asymmetry comes from the fact that HyperBodyWrapByteStream is only capable of wrapping aws_smithy_http::byte_stream::ByteStream; even if we dropped impl Stream for aws_smithy_http_server_python::types::ByteStream, it could not be wrapped in HyperBodyWrapByteStream.

For this reason, if something already implements the Stream trait, it's better to leave it as-is since we don't need to wrap it in a new-type to enable the trait.

david-perez · 2023-09-06T11:33:57Z

rust-runtime/aws-smithy-http/src/event_stream/sender.rs

+    // `FnStream` does not have a `Sync` bound but this struct needs to be `Sync`
+    // as demonstrated by a unit test `event_stream_sender_send_sync`.
+    // Wrapping `input_stream` with a `Mutex` will make `EventStreamSender` `Sync`.
+    input_stream: Mutex<FnStream<Result<T, E>>>,


Why does EventStreamSender need to be Sync? i.e. why does that test exist?

david-perez · 2023-09-06T11:49:17Z

examples/pokemon-service-common/src/lib.rs

                                    crate::error::CapturePokemonEventsError::InvalidPokeballError(
                                        crate::error::InvalidPokeballError {
                                            pokeball: pokeball.to_owned()
                                        }
                                    )
-                                );


Related to Burak's comment, I'm having trouble understanding when the receiver can disconnect and this can panic. Can we document that somewhere?

If the receiver can indeed drop, we shouldn't have examples where we panic, and instead teach users in the example how best to handle the disconnection.

If the receiver can't really drop, we shouldn't expose a fallible interface.

david-perez · 2023-09-06T11:50:31Z

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

@@ -48,7 +48,8 @@
 //!
 //! ### Stream a ByteStream into a file
 //! The previous example is recommended in cases where loading the entire file into memory first is desirable. For extremely large
-//! files, you may wish to stream the data directly to the file system, chunk by chunk. This is posible using the `futures::Stream` implementation.
+//! files, you may wish to stream the data directly to the file system, chunk by chunk.
+//! This is possible using the [`.next()`](crate::byte_stream::ByteStream::next).


Suggested change

//! This is possible using the [`.next()`](crate::byte_stream::ByteStream::next).

//! This is possible using the [`.next()`](crate::byte_stream::ByteStream::next) method.

This commit addresses #2910 (comment). Now that `HyperBodyWrapByteStream` is also used outside the context of `hyper::body::Body::wrap_stream` (passed to `tokio_util::io::StreamReader`), the module has been renamed to `futures_stream_adapter`. Furthermore, `HyperBodyWrapByteStream` and `HyperBodyWrapEventStream` have been renamed to `FuturesStreamCompatByteStream` and `FuturesStreamCompatEventStream` respectively.

rcoh

need more discussion here about how we will do this with paginators. I think we should probably split out some smaller PRs—I don't think FnStream is the interface we want to be generally exposing to people—it's not really use friendly and has a confusing name.

When stream is used as an input type (e.g. transcribe streaming) I think we may want to attempt to retain some amount of compatibility with the Stream trait, possibly via the smithy conversions / compatibility crate (or our own trait?)

I don't think that making it a concrete struct is really the right option here.

Can we make this PR much smaller to start piecing off individual chunks?

rcoh · 2023-09-07T20:03:30Z

aws/sdk/integration-tests/transcribestreaming/tests/test.rs

-            yield Ok(AudioStream::AudioEvent(AudioEvent::builder().audio_chunk(Blob::new(chunk)).build()));
-        }
-    };
+    let input_stream = FnStream::new(|tx| {


FnStream is an implementation of generators–the network isn't involved in all use cases. By bounding it, you all the caller to avoid using more memory than is necessary

rcoh · 2023-09-07T20:07:28Z

...in/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/PaginatorGenerator.kt

-                pub fn send(self) -> impl #{Stream}<Item = #{item_type}> + #{Unpin} {
+                /// _Note:_ No requests will be dispatched until the stream is used
+                /// (e.g. with [`.next().await`](aws_smithy_async::future::fn_stream::FnStream::next)).
+                pub fn send(self) -> #{fn_stream}::FnStream<#{item_type}> {


hmmm...not sure I like this. It's pretty constraining and also exposes a weird name to customers. I think we should wrap this in a newtype so we aren't publicly exposing the FnStream internals

Can we make this PR much smaller to start piecing off individual chunks?

We can create a smaller PR just to remove impl Stream from aws-smithy-async and but keep it for event streaming (that'll take care of Burak & David's questions around FnStream since the code around event stream will be unchanged).

The above send methods needs to return a new-type hiding FnStream.

github-actions · 2023-09-08T03:03:27Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2023-09-08T03:59:24Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

ysaito1001 · 2023-09-08T18:29:39Z

Closing this PR for the reason #2910 (comment).

…eam::ByteStream` (#2983) ## 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](#2910 (comment)) 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](https://github.com/awslabs/smithy-rs/blob/cb79a68e3c38d1e62d3980d5e7baedc1144bacc7/rust-runtime/aws-smithy-http-server-python/src/types.rs#L343) (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 - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [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._

ysaito1001 added 7 commits August 8, 2023 17:17

Remove futures_core::stream::Stream from smithy-async

6b308e4

Remove futures_core::stream::Stream from smithy-http

9a27075

Remove futures_core::stream::Stream from Paginator

f5b440c

Remove futures_core::stream::Stream from integration tests

2bcf9b5

Merge branch 'main' into ysaito/remove-futures-stream-from-public-api

6e31754

Fix links in docs

181440d

Fix codegen-server-test:test

f00d605

ysaito1001 added the breaking-change This will require a breaking change label Aug 9, 2023

ysaito1001 added 2 commits August 10, 2023 22:06

Fix tests in CI

3e05c47

Update pokemon example to use FnStream

8e90987

smithy-lang deleted a comment from github-actions bot Aug 11, 2023

ysaito1001 added 3 commits August 11, 2023 13:45

Fix more server tests

492e8bf

Fix test name that checks both Send and Sync

2c7541b

Fix event-streaming tests in pokemon-service

f7e8a69

smithy-lang deleted a comment from github-actions bot Aug 11, 2023

ysaito1001 added 2 commits August 11, 2023 14:44

Let ser_* return concreate types instead of impl Stream

f3dae46

Port event_stream_input_ergonomics test

3aae470

ysaito1001 added 3 commits August 11, 2023 15:46

Remove unused dependency from aws-smithy-async

3944bb3

Update pokemon-service-test to use FnStream

fd70e49

Remove unused tokio_stream::StreamExt from canary-lambda

ae047e5

smithy-lang deleted a comment from github-actions bot Aug 11, 2023

Tell udeps futures_util is used

50fe74c

ysaito1001 added 2 commits August 15, 2023 18:51

Update latest for canary-lambda

ccf9d06

Add dependency on aws-smithy-async

f8c0c40

ysaito1001 added 3 commits August 30, 2023 15:12

Return size hint as is by http_body::Body::size_hint

0f5f6be

This commit addresses #2910 (comment)

Add an comment FnStream is Send not Sync

d2e4411

This commit addresses #2910 (comment)

Add more comments as to why generator can be set to None

a778886

This commit addresses #2910 (comment)

ysaito1001 requested a review from jdisanti August 30, 2023 20:46

Merge branch 'main' into ysaito/remove-futures-stream-from-public-api

292a51b

jdisanti reviewed Aug 30, 2023

View reviewed changes

...ftware/amazon/smithy/rust/codegen/core/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt Outdated Show resolved Hide resolved

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt Outdated Show resolved Hide resolved

jdisanti reviewed Aug 30, 2023

View reviewed changes

...lin/software/amazon/smithy/rust/codegen/core/smithy/customizations/SmithyTypesPubUseExtra.kt Outdated Show resolved Hide resolved

ysaito1001 added 3 commits August 30, 2023 22:31

Convert StreamPayloadSerializer to interface

ab57dce

This commit addresses #2910 (comment)

Add comments to data class for ServerHttpBoundProtocolSection

3db0525

This commit addresses #2910 (comment)

Use ServiceShape.hasEventStreamOperations

91163de

This commit addresses #2910 (comment)

ysaito1001 requested a review from jdisanti August 31, 2023 04:16

jdisanti approved these changes Aug 31, 2023

View reviewed changes

jdisanti removed the needs-sdk-review label Aug 31, 2023

unexge reviewed Sep 5, 2023

View reviewed changes

david-perez reviewed Sep 6, 2023

View reviewed changes

rcoh requested changes Sep 7, 2023

View reviewed changes

Merge branch 'main' into ysaito/remove-futures-stream-from-public-api

35a1fca

Fix undeclared crate or module for feature-gated types

852211f

ysaito1001 closed this Sep 8, 2023

ysaito1001 mentioned this pull request Sep 12, 2023

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

Merged

2 tasks

Velfi removed the needs-server-review label Oct 19, 2023

ysaito1001 deleted the ysaito/remove-futures-stream-from-public-api branch November 11, 2023 02:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `futures_core::stream::Stream` from public API #2910

Remove `futures_core::stream::Stream` from public API #2910

ysaito1001 commented Aug 9, 2023 •

edited

Loading

github-actions bot commented Aug 11, 2023

github-actions bot commented Aug 11, 2023

github-actions bot commented Aug 16, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 31, 2023

jdisanti left a comment

unexge Sep 5, 2023

david-perez Sep 6, 2023

david-perez Sep 6, 2023

david-perez Sep 6, 2023

rcoh Sep 7, 2023

david-perez Sep 6, 2023

david-perez Sep 6, 2023

david-perez Sep 6, 2023

ysaito1001 Sep 7, 2023

david-perez Sep 6, 2023

david-perez Sep 6, 2023

david-perez Sep 6, 2023

rcoh left a comment

rcoh Sep 7, 2023

rcoh Sep 7, 2023

ysaito1001 Sep 7, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

ysaito1001 commented Sep 8, 2023

		// `aws_smithy_http_server_python::types::ByteStream` already implements
		// `futures::stream::Stream`, so no need to wrap it in a futures' stream-compatible

	//! This is possible using the [`.next()`](crate::byte_stream::ByteStream::next).
	//! This is possible using the [`.next()`](crate::byte_stream::ByteStream::next) method.

Remove futures_core::stream::Stream from public API #2910

Remove futures_core::stream::Stream from public API #2910

Conversation

ysaito1001 commented Aug 9, 2023 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Aug 11, 2023

github-actions bot commented Aug 11, 2023

github-actions bot commented Aug 16, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 31, 2023

jdisanti 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

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

rcoh 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

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

ysaito1001 commented Sep 8, 2023

Remove `futures_core::stream::Stream` from public API #2910

Remove `futures_core::stream::Stream` from public API #2910

ysaito1001 commented Aug 9, 2023 •

edited

Loading