Minimum throughput body timeouts Pt.1 #3068

Velfi · 2023-10-13T19:13:07Z

Motivation and Context

Description

This change adds a new body wrapper: The minimum throughput limit wrapper. It tracks the rate that data is being streamed from itself. If that rate falls below some configurable limit, it emits an error instead of the next chunk. This protects users from requests that start quickly but then slow down considerably.

I'd like to get this merged and then figure out the codegen/docs/examples/config part in a separate PR.

Testing

Tests are included.

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

aws/sdk/integration-tests/s3/tests/throughput-timeout.rs

github-actions · 2023-10-13T19:34:58Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

add more tests

aws/sdk/integration-tests/s3/tests/throughput-timeout.rs

github-actions · 2023-10-16T20:49:00Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-10-17T17:15:26Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

aws/sdk/integration-tests/s3/tests/throughput-timeout.rs

jdisanti · 2023-10-17T17:29:08Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

+        #[pin]
+        inner: InnerBody,
+        // A record of when and how much data was read
+        throughput_logs: VecDeque<(SystemTime, u64)>,


How large does this queue get in practice?

On my machine, a 100MB download with a tick rate of 1s produces 6700 logs. The download completes in only a few secs.

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

jdisanti · 2023-10-17T17:32:00Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

+        if this.throughput_logs.len() > LOG_WINDOW_SIZE {
+            this.throughput_logs.pop_front();
+        }


Seems like it should pop anything that's older than our time window, with the window size being a large upper bound. 16 is probably not high enough? Need to experiment with that number.

github-actions · 2023-10-19T21:55:36Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-10-19T23:06:26Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

jdisanti · 2023-10-19T23:11:39Z

rust-runtime/aws-smithy-async/src/rt/sleep.rs

@@ -83,7 +83,7 @@ pub fn default_async_sleep() -> Option<SharedAsyncSleep> {
 /// Future returned by [`AsyncSleep`].
 #[non_exhaustive]
 #[must_use]
-pub struct Sleep(Pin<Box<dyn Future<Output = ()> + Send + 'static>>);
+pub struct Sleep(Pin<Box<dyn Future<Output = ()> + Send + Sync + 'static>>);


Futures shouldn't be Sync.

After talking with Carl, I decided that this future should in fact be sync. While we could wrap this future in a mutex or similar to make it Sync, I'm opting to just add the bound. The reasoning is that omitting the Sync bound will cause more user pain than including it.

Requiring a future to be Sync can cause some issue when used with an async block (lang forum), but I don't necessarily see how that bites us here 🤔

At least, I think this PR needs a label breaking-change because Sleep::new now fails to take an async block for certain cases (playground - derived from the above lang forum).

remove `Sync` bound from `Sleep` fut update NUMBER_OF_LOGS_IN_ONE_KB const to be more accurate add trailer support to MTB fix SdkBody trailer handling

Velfi · 2023-10-20T20:42:59Z

~~This PR is currently blocked on some HTTP Body changes that @ysaito1001 will soon PR.~~

github-actions · 2023-10-20T20:59:16Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-10-23T16:22:14Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-10-23T19:30:37Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

Velfi · 2023-10-23T19:33:53Z

The current CI failures are slightly related to Yuki's work to feature gate stuff, I'll resolve them once he gets that work merged.

rcoh

Looks good—a few questions

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput/throughput.rs

rcoh · 2023-10-26T15:11:15Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

+        this.sleep_fut.replace(sleep_fut);
+
+        // Calculate the current throughput and emit an error if it's too low.
+        let actual_throughput = this.throughput_logs.calculate_throughput()?;


is it worth benchmarking this at some point? if we're getting a ton of data how much time do we spend calculating throughput? I wonder if we should only calculate it when the sleep expires, e.g.

Agreed, just not in this PR.

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

rcoh · 2023-10-26T15:19:00Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

+            }
+            Self::TimeTravel(_) => write!(
+                f,
+                "negative time has elapsed while reading the inner body, this is a bug"


it isn't really though—we're using SystemTime and not Instant. SystemTime doesn't guarantee monotonicity:

Distinct from the Instant type, this time measurement is not monotonic. This means that you can save a file to the file system, then save another file to the file system, and the second file has a SystemTime measurement earlier than the first. In other words, an operation that happens after another operation in real time may have an earlier SystemTime!

https://doc.rust-lang.org/std/time/struct.SystemTime.html

We should probably try to handle this gracefully or add some sort of InstantTimeSource in our bag of tricks

Agreed, just not in this PR.

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput.rs

github-actions · 2023-10-26T16:16:12Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-10-26T17:05:07Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

add feature gate to poll_next_trailers

github-actions · 2023-10-26T18:31:57Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

Velfi added 3 commits October 12, 2023 14:16

add updated minimum throughput body and tests

6052d0b

Merge branch 'main' into zhessler-minimum-throughput-body

10d7c6f

update minimum throughput body tests

299cdd1

Velfi force-pushed the zhessler-minimum-throughput-body branch from b1300d4 to 299cdd1 Compare October 13, 2023 19:14

Velfi commented Oct 13, 2023

View reviewed changes

aws/sdk/integration-tests/s3/tests/throughput-timeout.rs Outdated Show resolved Hide resolved

update minimum throughput body

61b3e8d

add more tests

Velfi commented Oct 16, 2023

View reviewed changes

aws/sdk/integration-tests/s3/tests/throughput-timeout.rs Outdated Show resolved Hide resolved

Velfi and others added 3 commits October 17, 2023 11:55

update MTB such that it works correctly when polled only once

f48be94

remove ByteStream::taken

24c5e7a

Merge branch 'main' into zhessler-minimum-throughput-body

900e9c8

Velfi marked this pull request as ready for review October 17, 2023 16:59

Velfi requested review from a team as code owners October 17, 2023 16:59

Velfi changed the title ~~Zhessler minimum throughput body~~ Minimum throughput body timeouts Pt.1 Oct 17, 2023

jdisanti reviewed Oct 17, 2023

View reviewed changes

Velfi marked this pull request as draft October 17, 2023 19:18

Velfi added 3 commits October 17, 2023 16:06

make MTB futuristic

4c5d4b8

Merge branch 'main' into zhessler-minimum-throughput-body

2c42b2a

update minimum throughput body tests

b322525

Velfi and others added 4 commits October 19, 2023 17:43

update minimum throughput body logging

0fe952e

fix clippy lints

66cfa44

remove use of deprecated alias

5b3c0d6

Merge branch 'main' into zhessler-minimum-throughput-body

9488a1f

jdisanti reviewed Oct 19, 2023

View reviewed changes

add http_body external type to allow list

98ce003

remove `Sync` bound from `Sleep` fut update NUMBER_OF_LOGS_IN_ONE_KB const to be more accurate add trailer support to MTB fix SdkBody trailer handling

Velfi and others added 2 commits October 23, 2023 11:01

add shrinking sine wave test

0761d13

Merge branch 'main' into zhessler-minimum-throughput-body

b3d8a32

make the Sleep future Sync again

43455b1

Velfi marked this pull request as ready for review October 23, 2023 19:12

Velfi added the needs-sdk-review label Oct 23, 2023

Velfi mentioned this pull request Oct 25, 2023

0.57 release tracking issue #3097

Closed

9 tasks

Velfi added the breaking-change This will require a breaking change label Oct 25, 2023

rcoh reviewed Oct 26, 2023

View reviewed changes

Merge branch 'main' into zhessler-minimum-throughput-body

85c7b1a

update MTB based on PR feedback

1fb22fc

fix doc link errors

f7da9a4

add feature gate to poll_next_trailers

rcoh approved these changes Oct 26, 2023

View reviewed changes

Velfi added this pull request to the merge queue Oct 26, 2023

Merged via the queue into main with commit 0f1f1a6 Oct 26, 2023
40 of 41 checks passed

Velfi deleted the zhessler-minimum-throughput-body branch October 26, 2023 20:37

jdisanti mentioned this pull request Nov 7, 2023

Question: What is the behavior of read_timeout in the HTTP client? #3145

Closed

Velfi removed the needs-sdk-review label Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimum throughput body timeouts Pt.1 #3068

Minimum throughput body timeouts Pt.1 #3068

Velfi commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023

github-actions bot commented Oct 16, 2023

github-actions bot commented Oct 17, 2023

jdisanti Oct 17, 2023

Velfi Oct 19, 2023

jdisanti Oct 17, 2023

github-actions bot commented Oct 19, 2023

github-actions bot commented Oct 19, 2023

jdisanti Oct 19, 2023

Velfi Oct 24, 2023

ysaito1001 Oct 24, 2023

Velfi commented Oct 20, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

Velfi commented Oct 23, 2023

rcoh left a comment

rcoh Oct 26, 2023

Velfi Oct 26, 2023

rcoh Oct 26, 2023

Velfi Oct 26, 2023

github-actions bot commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

Minimum throughput body timeouts Pt.1 #3068

Minimum throughput body timeouts Pt.1 #3068

Conversation

Velfi commented Oct 13, 2023 • edited Loading

Motivation and Context

Description

Testing

github-actions bot commented Oct 13, 2023

github-actions bot commented Oct 16, 2023

github-actions bot commented Oct 17, 2023

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 Oct 19, 2023

github-actions bot commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Velfi commented Oct 20, 2023 • edited Loading

github-actions bot commented Oct 20, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

Velfi commented Oct 23, 2023

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

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

Velfi commented Oct 13, 2023 •

edited

Loading

Velfi commented Oct 20, 2023 •

edited

Loading