Feature: Stalled stream protection #3202

Velfi · 2023-11-14T21:33:17Z

See the upgrade guide for this feature to learn more.

The breaking change is to the MinimumThroughputBody::new method.

Motivation and Context

#1562

Description

awslabs/aws-sdk-rust#956

Testing

I added several tests

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.

…am-protection

github-actions · 2023-11-14T21:50:50Z

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-11-15T18:35:56Z

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.

update external types add defaults for stalled stream protection test stalled stream protection defaults

…am-protection

github-actions · 2023-11-15T21:03:16Z

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-11-15T21:56:08Z

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.

aws/rust-runtime/aws-config/src/lib.rs

aws/rust-runtime/aws-types/src/sdk_config.rs

aws/sdk/integration-tests/s3/Cargo.toml

aws/sdk/integration-tests/s3/tests/stalled-stream-protection.rs

...otlin/software/amazon/smithy/rust/codegen/client/smithy/generators/OperationCustomization.kt

aws/rust-runtime/aws-types/src/sdk_config.rs

...y/rust/codegen/client/smithy/generators/config/StalledStreamProtectionConfigCustomization.kt

rust-runtime/aws-smithy-runtime/src/client/stalled_stream_protection.rs

Co-authored-by: John DiSanti <[email protected]>

…am-protection

github-actions · 2023-11-16T17:21:44Z

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.

aws/rust-runtime/aws-config/src/lib.rs

rcoh · 2023-11-15T16:03:55Z

aws/sdk/integration-tests/s3/tests/stalled-stream-protection.rs

+// This test doesn't work because we can't count on `hyper` to poll the body,
+// regardless of whether we schedule a wake. To make this functionality work,
+// we'd have to integrate more closely with the orchestrator.
+//
+// I'll leave this test here because we do eventually want to support stalled
+// stream protection for uploads.


does this work if the body is polled at least once?

I think it would have to be polled twice:

once to set the start of polling time used to calculate throughput

once more to see that throughput is below the minimum

Cases where it gets checked once only won't be marked as failed.

aws/rust-runtime/aws-config/src/lib.rs

...y/rust/codegen/client/smithy/generators/config/StalledStreamProtectionConfigCustomization.kt

rcoh · 2023-11-16T17:29:02Z

...y/rust/codegen/client/smithy/generators/config/StalledStreamProtectionConfigCustomization.kt

+                    ${section.newLayerName}.store_put(
+                        #{StalledStreamProtectionConfig}::new_enabled().grace_period(#{Duration}::from_secs(5)).build()
+                    );


this is the default right? I think we should be setting this in the default_runtime_plugin, right, @jdisanti?

We need to make sure this doesn't override what the customer is setting, as well.

Oh, good catch. This definitely needs to be a defaults runtime plugin.

The test_stalled_stream_protection_for_downloads_can_be_disabled ensures that the customer's config isn't overridden.

rcoh · 2023-11-16T17:31:29Z

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

+            // Check the grace period future to see if it needs creating.
+            let mut grace_period_fut = this
+                .grace_period_fut
+                .take()
+                .unwrap_or_else(|| this.async_sleep.sleep(this.options.grace_period()));
+            if let Poll::Ready(()) = pin!(&mut grace_period_fut).poll(cx) {
+                // The grace period has ended!
+                return Poll::Ready(Some(Err(Box::new(Error::ThroughputBelowMinimum {
+                    expected: self.options.minimum_throughput(),
+                    actual: actual_throughput.unwrap(),
+                }))));
+            };
+            this.grace_period_fut.replace(grace_period_fut);
        } else {
-            poll_res
+            // Ensure we don't have an active grace period future if we're not
+            // currently below the minimum throughput.
+            let _ = this.grace_period_fut.take();
        }
+


what's this change for? bugs discovered?

This adds support for grace periods. We didn't have it before this PR. I added them b/c John thought they'd be helpful.

respond to PR comments

github-actions · 2023-11-16T18:46:54Z

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-11-16T20:08:03Z

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-11-16T21:10:36Z

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.

jdisanti

LGTM. Would be good to have @rcoh look also.

Velfi · 2023-11-16T22:19:02Z

I need to fix some broken tests before merging this. Namely, turning off the protection for some old tests.

rcoh

approved with some non-blocking comments

rcoh · 2023-11-17T00:13:05Z

aws/sdk/integration-tests/dynamodb/tests/retries-with-client-rate-limiting.rs

@@ -65,6 +67,7 @@ async fn test_adaptive_retries_with_no_throttling_errors() {

    let http_client = StaticReplayClient::new(events);
    let config = aws_sdk_dynamodb::Config::builder()
+        .stalled_stream_protection(StalledStreamProtectionConfig::new_disabled())


why do we have to disable this in tests?

is it because we're overriding sleep and the sleep is sleeping instantly?

rcoh · 2023-11-17T00:22:42Z

rust-runtime/aws-smithy-runtime-api/src/client/stalled_stream_protection.rs

+}
+
+impl StalledStreamProtectionConfig {
+    /// Create a new config that enables stalled stream protection.


"the default grace period will be used"

rcoh · 2023-11-17T00:23:59Z

rust-runtime/aws-smithy-runtime/src/client/defaults.rs

+        .with_config(layer("default_stalled_stream_protection_config", |layer| {
+            layer.store_put(
+                StalledStreamProtectionConfig::new_enabled()
+                    .grace_period(Duration::from_secs(5))


this is the default anyway—I'm a little worried about the default coming from two places because someone might think they're changing it but it won't actually change anything.

maybe the answer is a comment next to the default in options.rs?

rcoh · 2023-11-17T00:25:15Z

rust-runtime/aws-smithy-runtime/src/client/stalled_stream_protection.rs

+        cfg: &mut ConfigBag,
+    ) -> Result<(), BoxError> {
+        if self.enable_for_request_body {
+            if let Some(cfg) = cfg.load::<StalledStreamProtectionConfig>() {


nit: naming this cfg makes me think it's a ConfigBag

…-stalled-stream-protection

- Fix a bug where SSP wasn't being forwarded from aws-config - Fix a bunch of tests

…-stalled-stream-protection

github-actions · 2023-11-17T02:55:49Z

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.

fix bad merge

github-actions · 2023-11-17T12:36:52Z

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.

Velfi added 3 commits November 8, 2023 15:12

checkpoint

79bca6c

implement stalled stream protection for downloads

5569064

Merge remote-tracking branch 'origin/main' into zhessler-stalled-stre…

041a6dd

…am-protection

Velfi requested review from a team as code owners November 14, 2023 21:33

Velfi and others added 3 commits November 15, 2023 12:02

fix import that needed a feature gate

bc0965c

Merge branch 'main' into zhessler-stalled-stream-protection

bf2040a

fix doc test

48a42f2

Velfi added the breaking-change This will require a breaking change label Nov 15, 2023

Velfi added 4 commits November 15, 2023 13:39

add CHANGELOG

cb63698

update external types add defaults for stalled stream protection test stalled stream protection defaults

add grace period support

d5e3cee

remove feature gate

6cae978

Merge remote-tracking branch 'origin/main' into zhessler-stalled-stre…

b7bce2f

…am-protection

Velfi added the needs-sdk-review label Nov 15, 2023

Velfi added 2 commits November 15, 2023 15:24

add allowed external type to aws-types

d653f42

add creds to integration tests

46920fa

jdisanti reviewed Nov 16, 2023

View reviewed changes

Velfi and others added 4 commits November 16, 2023 09:24

Apply suggestions from code review

3e08632

Co-authored-by: John DiSanti <[email protected]>

Merge remote-tracking branch 'origin/main' into zhessler-stalled-stre…

5a15468

…am-protection

remove config suffix from stalled stream function names

b1f4149

remove blob gate on stalled stream protection interceptor

6d9a7e1

rcoh reviewed Nov 16, 2023

View reviewed changes

Velfi and others added 2 commits November 16, 2023 12:26

move stalled stream protection to runtime API crate

a760a26

respond to PR comments

Merge branch 'main' into zhessler-stalled-stream-protection

3dac27c

Velfi and others added 2 commits November 16, 2023 13:47

fix external types

f78c4f0

Merge branch 'main' into zhessler-stalled-stream-protection

25f9819

fix dynamoDB retry tests

6a52c90

jdisanti approved these changes Nov 16, 2023

View reviewed changes

Velfi marked this pull request as draft November 16, 2023 22:18

rcoh approved these changes Nov 17, 2023

View reviewed changes

rcoh added 3 commits November 16, 2023 19:48

Merge branch 'main' of github.com:smithy-lang/smithy-rs into zhessler…

ed660ac

…-stalled-stream-protection

Fix bugs and tests

3ca242d

- Fix a bug where SSP wasn't being forwarded from aws-config - Fix a bunch of tests

Fix timestream

a0ab75a

rcoh marked this pull request as ready for review November 17, 2023 02:35

Merge branch 'main' of github.com:smithy-lang/smithy-rs into zhessler…

64f994a

…-stalled-stream-protection

rcoh enabled auto-merge November 17, 2023 03:21

rcoh added this pull request to the merge queue Nov 17, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 17, 2023

rcoh added 2 commits November 17, 2023 07:14

Merge branch 'main' into zhessler-stalled-stream-protection

c03f39b

Update CHANGELOG.next.toml

b257a71

fix bad merge

rcoh added this pull request to the merge queue Nov 17, 2023

Velfi removed the needs-sdk-review label Nov 17, 2023

Merged via the queue into main with commit 768237a Nov 17, 2023
40 of 41 checks passed

rcoh deleted the zhessler-stalled-stream-protection branch November 17, 2023 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Stalled stream protection #3202

Feature: Stalled stream protection #3202

Velfi commented Nov 14, 2023 •

edited

Loading

github-actions bot commented Nov 14, 2023

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 16, 2023

rcoh Nov 15, 2023

Velfi Nov 16, 2023

rcoh Nov 16, 2023

rcoh Nov 16, 2023

jdisanti Nov 16, 2023

Velfi Nov 16, 2023

rcoh Nov 16, 2023

Velfi Nov 16, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

jdisanti left a comment

Velfi commented Nov 16, 2023

rcoh left a comment

rcoh Nov 17, 2023

rcoh Nov 17, 2023

rcoh Nov 17, 2023

rcoh Nov 17, 2023

rcoh Nov 17, 2023

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

Feature: Stalled stream protection #3202

Feature: Stalled stream protection #3202

Conversation

Velfi commented Nov 14, 2023 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Nov 14, 2023

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 16, 2023

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

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

jdisanti left a comment

Choose a reason for hiding this comment

Velfi commented Nov 16, 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

Choose a reason for hiding this comment

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

Velfi commented Nov 14, 2023 •

edited

Loading